{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6684","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6684/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6684/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6684/events","html_url":"https://github.com/cockroachdb/cockroach/pull/6684","id":154871428,"number":6684,"title":"storage/engine: reuse iterators for the lifetime of rocksDBBatch","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":1,"created_at":"2016-05-14T19:31:51Z","updated_at":"2016-05-16T03:26:30Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/6684","html_url":"https://github.com/cockroachdb/cockroach/pull/6684","diff_url":"https://github.com/cockroachdb/cockroach/pull/6684.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/6684.patch"},"body":"`rocksDBBatch` caches a prefix and normal (non-prefix) iterator upon\nfirst use and reuses them for the lifetime of the batch. This avoids a\ncgo call to create an iterator on every request. Note that this change\nonly affects benchmarks which are doing random inserts and thus not able\nto take advantage of the \"blind\" put optimization.\n\nname                            old time/op    new time/op    delta\nTrackChoices1_Cockroach-32         651µs ± 4%     634µs ± 3%  -2.59%  (p=0.000 n=20+20)\nTrackChoices10_Cockroach-32        203µs ± 2%     197µs ± 3%  -2.89%  (p=0.000 n=19+20)\nTrackChoices100_Cockroach-32       143µs ± 4%     138µs ± 3%  -4.09%  (p=0.000 n=20+20)\nTrackChoices1000_Cockroach-32      139µs ± 5%     134µs ± 4%  -3.40%  (p=0.000 n=20+20)\nInsertDistinct1_Cockroach-32      2.49ms ± 3%    2.45ms ± 5%  -1.44%  (p=0.028 n=19+20)\nInsertDistinct10_Cockroach-32     2.04ms ± 3%    1.96ms ± 2%  -4.03%  (p=0.000 n=20+20)\nInsertDistinct100_Cockroach-32    2.41ms ± 3%    2.35ms ± 4%  -2.17%  (p=0.000 n=19+20)\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/6684)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/219341233","html_url":"https://github.com/cockroachdb/cockroach/pull/6684#issuecomment-219341233","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6684","id":219341233,"user":{"login":"bdarnell","id":160562,"avatar_url":"https://avatars.githubusercontent.com/u/160562?v=3","gravatar_id":"","url":"https://api.github.com/users/bdarnell","html_url":"https://github.com/bdarnell","followers_url":"https://api.github.com/users/bdarnell/followers","following_url":"https://api.github.com/users/bdarnell/following{/other_user}","gists_url":"https://api.github.com/users/bdarnell/gists{/gist_id}","starred_url":"https://api.github.com/users/bdarnell/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bdarnell/subscriptions","organizations_url":"https://api.github.com/users/bdarnell/orgs","repos_url":"https://api.github.com/users/bdarnell/repos","events_url":"https://api.github.com/users/bdarnell/events{/privacy}","received_events_url":"https://api.github.com/users/bdarnell/received_events","type":"User","site_admin":false},"created_at":"2016-05-16T03:26:30Z","updated_at":"2016-05-16T03:26:30Z","body":"\n\n\n\n\n\nReviewed 2 of 2 files at r1.\nReview status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.\n\n---\n\n*[storage/engine/rocksdb.go, line 521 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6684#-KHrYryjEUDTgGpuU-s1:-KHrYrykfnJvSu4XJVia:30524065) ([raw file](https://github.com/cockroachdb/cockroach/blob/dfd13a69e99e637f0747d3f0bef0dd87fd8665d2/storage/engine/rocksdb.go#L521)):*\n> ```Go\n> \n> func (r *rocksDBBatchIterator) Close() {\n> \t// rocksDBBatchIterator.Close() is a no-op. The iterator is left open until\n> ```\n\nI'd be more comfortable if we added a boolean `open` flag to `rocksDBBatchIterator` (to be set in NewIterator and cleared in Close) to guard against any attempt to reuse an iterator while another is open.\n\nAs the MVCCDeleteRange change above shows, sometimes we have more than one iterator, and it seems like an arbitrary restriction to allow one prefix and one non-prefix. \n\n---\n\n*[storage/engine/rocksdb.go, line 546 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6684#-KHr_AzzDiBZwMlhYZda:-KHr_AzzDiBZwMlhYZdb:1238269991) ([raw file](https://github.com/cockroachdb/cockroach/blob/dfd13a69e99e637f0747d3f0bef0dd87fd8665d2/storage/engine/rocksdb.go#L546)):*\n> ```Go\n> func (r *rocksDBBatch) Close() {\n> \tif i := &r.prefixIter.rocksDBIterator; i.iter != nil {\n> \t\tC.DBIterDestroy(i.iter)\n> ```\n\nComment that we are deliberately calling C.DBIterDestroy directly instead of going through prefixIter.rocksDBIterator.Close to avoid putting our iterator in the pool.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/cockroachdb/cockroach/6684)*\n<!-- Sent from Reviewable.io -->\n"}}