{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6861","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6861/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6861/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6861/events","html_url":"https://github.com/cockroachdb/cockroach/pull/6861","id":156359690,"number":6861,"title":"storage/engine: add rocksDBBatchBuilder","user":{"login":"petermattis","id":6201335,"avatar_url":"https://avatars.githubusercontent.com/u/6201335?v=3","gravatar_id":"","url":"https://api.github.com/users/petermattis","html_url":"https://github.com/petermattis","followers_url":"https://api.github.com/users/petermattis/followers","following_url":"https://api.github.com/users/petermattis/following{/other_user}","gists_url":"https://api.github.com/users/petermattis/gists{/gist_id}","starred_url":"https://api.github.com/users/petermattis/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/petermattis/subscriptions","organizations_url":"https://api.github.com/users/petermattis/orgs","repos_url":"https://api.github.com/users/petermattis/repos","events_url":"https://api.github.com/users/petermattis/events{/privacy}","received_events_url":"https://api.github.com/users/petermattis/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":23,"created_at":"2016-05-23T20:28:46Z","updated_at":"2016-05-31T00:26:55Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/6861","html_url":"https://github.com/cockroachdb/cockroach/pull/6861","diff_url":"https://github.com/cockroachdb/cockroach/pull/6861.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/6861.patch"},"body":"Building batches in Go is significantly faster than performing a cgo call:\n~55ns/op vs ~400ns/op. The Go batches do not support indexing, so flush to\na C++ batch whenever a read of the batch is being performed. I experimented\nwith adding indexing to the Go side, but all attempts were significantly\nslower than flushing to C++.\n\nAdded an `engine.Batch` interface with a `FreezeFlushes` method which is\ncalled to avoid flushing a batch in order to write the raft applied index\nand mvcc stats.\n\nFixes #6289.\nFixes #6739.\n\n```\nname                            old time/op  new time/op  delta\nTrackChoices1_Cockroach-32       597µs ± 5%   577µs ± 3%   -3.37%  (p=0.000 n=19+20)\nTrackChoices10_Cockroach-32      153µs ± 5%   138µs ± 4%  -10.02%  (p=0.000 n=20+20)\nTrackChoices100_Cockroach-32    96.9µs ± 7%  80.5µs ± 5%  -16.95%  (p=0.000 n=20+19)\nTrackChoices1000_Cockroach-32   92.0µs ± 7%  73.7µs ± 6%  -19.94%  (p=0.000 n=20+19)\nInsertDistinct1_Cockroach-32    2.04ms ± 9%  1.84ms ± 8%   -9.62%  (p=0.000 n=20+20)\nInsertDistinct10_Cockroach-32   1.36ms ± 5%  1.23ms ± 4%  -10.09%  (p=0.000 n=19+20)\nInsertDistinct100_Cockroach-32  2.03ms ±15%  1.67ms ± 7%  -17.92%  (p=0.000 n=20+19)\nKVInsert1_SQL-32                 519µs ± 4%   528µs ± 5%   +1.88%  (p=0.024 n=20+20)\nKVInsert10_SQL-32                926µs ± 6%   906µs ± 3%   -2.13%  (p=0.028 n=20+20)\nKVInsert100_SQL-32              3.89ms ± 8%  3.53ms ± 8%   -9.14%  (p=0.000 n=20+19)\nKVInsert1000_SQL-32             29.4ms ±11%  27.5ms ± 8%   -6.64%  (p=0.000 n=20+20)\nKVInsert10000_SQL-32             356ms ±10%   340ms ±11%   -4.41%  (p=0.013 n=19+19)\nKVUpdate1_SQL-32                 757µs ± 9%   742µs ± 9%     ~     (p=0.253 n=20+20)\nKVUpdate10_SQL-32               1.31ms ± 7%  1.27ms ± 6%   -2.96%  (p=0.001 n=20+19)\nKVUpdate100_SQL-32              5.44ms ± 7%  5.24ms ± 7%   -3.74%  (p=0.006 n=20+20)\nKVUpdate1000_SQL-32             43.5ms ± 6%  42.0ms ± 9%   -3.62%  (p=0.007 n=20+20)\nKVUpdate10000_SQL-32             508ms ±16%   452ms ±14%  -10.97%  (p=0.000 n=20+18)\n```\n\n<!-- Reviewable:start -->\n---\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"35\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/cockroachdb/cockroach/6861)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/222569069","html_url":"https://github.com/cockroachdb/cockroach/pull/6861#issuecomment-222569069","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6861","id":222569069,"user":{"login":"petermattis","id":6201335,"avatar_url":"https://avatars.githubusercontent.com/u/6201335?v=3","gravatar_id":"","url":"https://api.github.com/users/petermattis","html_url":"https://github.com/petermattis","followers_url":"https://api.github.com/users/petermattis/followers","following_url":"https://api.github.com/users/petermattis/following{/other_user}","gists_url":"https://api.github.com/users/petermattis/gists{/gist_id}","starred_url":"https://api.github.com/users/petermattis/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/petermattis/subscriptions","organizations_url":"https://api.github.com/users/petermattis/orgs","repos_url":"https://api.github.com/users/petermattis/repos","events_url":"https://api.github.com/users/petermattis/events{/privacy}","received_events_url":"https://api.github.com/users/petermattis/received_events","type":"User","site_admin":false},"created_at":"2016-05-31T00:26:55Z","updated_at":"2016-05-31T00:26:55Z","body":"\n\n\n\n\n\nReview status: all files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.\n\n---\n\n*[storage/replica.go, line 1682 \\[r11\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6861#-KJ1GnfeCbMf9iJBsXQG:-KJ3AM0kAcZVYK8yqc7d:-1183050200) ([raw file](https://github.com/cockroachdb/cockroach/blob/616c4b98224eba638a2adeb365922b0cb8184809/storage/replica.go#L1682)):*\n<details><summary><i>Previously, bdarnell (Ben Darnell) wrote…</i></summary>\n> I'm pretty sure we never get here with any engine besides `engine.Batch` (if we did, we'd break atomicity since we rely on being able to discard a partially-written batch on error), so we could make the methods around here pass Batches instead of Engines and get rid of this check. Don't block the PR for it, but it's something to consider when we clean up these interfaces. \n</details>\nYep, we always have an `engine.Batch`. I'd like to plumb that through as we clean up these interfaces.\n\n---\n\n*[storage/engine/batch.go, line 83 \\[r6\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6861#-KIhvfesmK3vmXXJQmLY:-KJ3Byp82dRe9e7oNslA:1008488657) ([raw file](https://github.com/cockroachdb/cockroach/blob/e18aca090f7f686de20cce7c86069786f50c4cef/storage/engine/batch.go#L83)):*\n<details><summary><i>Previously, bdarnell (Ben Darnell) wrote…</i></summary>\n> Ah, I missed that the big-endian versions are for MVCCKey encoding, not the rocksdb batch encoding. Comments would be nice: this varint encoding is the one used by rocksdb and is incompatible with the one in util/encoding, while the fixed-length encodings are our own. It's unfortunate that we can't easily share these with util/encoding. \n</details>\nI'll add a bunch of comments when I add the additional tests @tschottdorf is asking for.\n\n---\n\n*[storage/engine/batch.go, line 100 \\[r6\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6861#-KIixzRUOW1ZPUHc9oBE:-KJ3BCIUgCaHkZdpaVdm:240733536) ([raw file](https://github.com/cockroachdb/cockroach/blob/e18aca090f7f686de20cce7c86069786f50c4cef/storage/engine/batch.go#L100)):*\n<details><summary><i>Previously, bdarnell (Ben Darnell) wrote…</i></summary>\n> TestBatchBuilder tests that we can construct keys and apply batches, but it doesn't test that we're encoding the data correctly. For example, if you swapped `putUint32` to little-endian, I'm pretty sure TestBatchBuilder wouldn't notice. Something else in the test suite would probably blow up, but that's going to be a difficult failure to diagnose. That test needs to read the data back and make sure it got what it thought it wrote. \n</details>\n`TestBatchBuilder` builds a batch representation using both the old C++ code and the new Go code and then compares the two representations. I just verified that changing `putUint32` caused the test to fail.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/cockroachdb/cockroach/6861)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"petermattis"}}