save a key comparison in block seeks #6646

ajkr · 2020-04-03T22:37:39Z

This saves up to two key comparisons in block seeks. The first key
comparison saved is a redundant key comparison against the restart key
where the linear scan starts. This comparison is saved in all cases
except when the found key is in the first restart interval. The
second key comparison saved is a redundant key comparison against the
restart key where the linear scan ends. This is only saved in cases
where all keys in the restart interval are less than the target
(probability roughly 1/restart_interval).

Test Plan:

ran readrandom with mostly default settings and counted key comparisons
using PerfContext.

before: user_key_comparison_count = 30370310
after: user_key_comparison_count = 28881965

setup command:

$ TEST_TMPDIR=/dev/shm/dbbench ./db_bench -benchmarks=fillrandom,compact -write_buffer_size=1048576 -target_file_size_base=1048576 -max_bytes_for_level_base=4194304 -max_background_jobs=12 -level_compaction_dynamic_level_bytes=true -num=10000000

benchmark command:

$ TEST_TMPDIR=/dev/shm/dbbench/ ./db_bench -use_existing_db=true -benchmarks=readrandom -disable_auto_compactions=true -num=10000000 -compression_type=none -reads=1000000 -perf_level=3

facebook-github-bot

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

ajkr · 2020-04-03T22:57:20Z

edit: this problem is fixed.

huh, after ~30 minutes stress test failed. not sure yet if related.

$ python ./tools/db_crashtest.py blackbox --simple --max_key=1000000 --value_size_mult=33 --write_buffer_size=1048576 --target_file_size_base=1048576 --max_bytes_for_level_base=4194304
...
***Iterator diverged from control iterator which has value 000000000000270F000000000000012B00000000000002B1 total_order_seek: 0 auto_prefix_mode: 0 S 00000000000022D700000000000001027878 PPNPNPNNNP; total_order_seek: 0 auto_prefix_mode: 0 STL PPPNNNN***

facebook-github-bot · 2020-04-03T23:01:14Z

@ajkr has updated the pull request. Re-import the pull request

facebook-github-bot

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-04-04T00:08:06Z

@ajkr has updated the pull request. Re-import the pull request

facebook-github-bot · 2020-04-04T04:59:55Z

@ajkr has updated the pull request. Re-import the pull request

ajkr · 2020-04-04T05:01:03Z

alright, known problems are fixed so ready for review.

facebook-github-bot

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-04-06T00:21:29Z

@ajkr has updated the pull request. Re-import the pull request

facebook-github-bot · 2020-04-06T00:23:33Z

@ajkr has updated the pull request. Re-import the pull request

facebook-github-bot

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-04-11T22:19:08Z

@ajkr has updated the pull request. Re-import the pull request

facebook-github-bot · 2020-04-11T22:54:54Z

@ajkr has updated the pull request. Re-import the pull request

facebook-github-bot · 2020-04-11T22:57:27Z

@ajkr has updated the pull request. Re-import the pull request

facebook-github-bot

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-06-08T19:46:40Z

@ajkr has updated the pull request. Re-import the pull request

table/block_based/block.cc

table/block_based/block.h

ajkr · 2020-06-08T19:55:30Z

table/block_based/block.cc

+      ParseNextDataKey<DecodeEntry>();
+    }
+  } else {
+    SeekToRestartPoint(index - 1);


Done. I also redid the BinarySeek() to avoid changing the binary search. The number of key comparisons saved lessened as a result, but I am fine with leaving those for a future PR..

ajkr

couple explanatory comments

facebook-github-bot · 2020-06-08T19:58:34Z

@ajkr has updated the pull request. Re-import the pull request

facebook-github-bot

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

pdillinger

I like this. Some suggestions for readability, but good.

Wouldn't hurt to have another set of eyes on this for correctness.

table/block_based/block.cc

pdillinger · 2020-06-08T22:29:16Z

MacOS compilation failure

ajkr

MacOS compilation failure

Fixed (I think).

Thanks for the review!

table/block_based/block.cc

facebook-github-bot · 2020-06-09T20:54:38Z

@ajkr has updated the pull request. Re-import the pull request

facebook-github-bot

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

anand1976

LGTM! Just a few nits and suggestions.

anand1976 · 2020-06-10T01:26:57Z

table/block_based/block.cc

+                                            const Comparator* comp) {
+  // Linear search (within restart block) for first key >= target
+  SeekToRestartPoint(index);
+  Next();


A comment here might help. It took me a little while to figure out that this Next() actually doesn't advance to the next key, but just ends up decoding the key at the restart point we seek'd to.

Agree, done.

anand1976 · 2020-06-10T01:47:15Z

table/block_based/block.cc

  *index = left;
+  if (*index == 0) {
+    // Special case as we land at zero as long as restart key at index 1 is >=


We land at 0 if restart key at index 1 > "target", right?

Right, done.

anand1976 · 2020-06-10T01:52:49Z

table/block_based/block.cc

@@ -393,6 +386,7 @@ void IndexBlockIter::Seek(const Slice& target) {
  }
  status_ = Status::OK();
  uint32_t index = 0;
+  bool is_index_key_result = false;


Something like skip_linear_scan might be clearer.

Done. Also renamed "ScanAfterBinarySeek" -> "FindKeyAfterBinarySeek" to make it fit in better.

anand1976 · 2020-06-10T01:54:10Z

table/block_based/block.cc

+      max_offset = port::kMaxUint32;
+    }
+    while (true) {
+      Next();


Can we just use ParseNextDataKey instead of Next and Valid?

It's making use of polymorphism here -- IndexBlockIter::Next() calls ParseNextIndexKey() while DataBlockIter::Next() calls ParseNextDataKey().

This saves up to two key comparisons in block seeks. The first key comparison saved is a redundant key comparison against the restart key where the linear scan starts. This comparison is saved in all cases except when the found key is in the first restart interval. The second key comparison saved is a redundant key comparison against the restart key where the linear scan ends. This is only saved in cases where all keys in the restart interval are less than the target (probability roughly `1/restart_interval`). Test Plan: ran readrandom with mostly default settings and counted key comparisons using `PerfContext`. before: `user_key_comparison_count = 30370310` after: `user_key_comparison_count = 28881965` setup command: ``` $ TEST_TMPDIR=/dev/shm/dbbench ./db_bench -benchmarks=fillrandom,compact -write_buffer_size=1048576 -target_file_size_base=1048576 -max_bytes_for_level_base=4194304 -max_background_jobs=12 -level_compaction_dynamic_level_bytes=true -num=10000000 ``` benchmark command: ``` $ TEST_TMPDIR=/dev/shm/dbbench/ ./db_bench -use_existing_db=true -benchmarks=readrandom -disable_auto_compactions=true -num=10000000 -compression_type=none -reads=1000000 -perf_level=3 ```

ajkr

Thanks for the review! Addressed the comments.

ajkr · 2020-06-10T18:51:56Z

table/block_based/block.cc

@@ -393,6 +386,7 @@ void IndexBlockIter::Seek(const Slice& target) {
  }
  status_ = Status::OK();
  uint32_t index = 0;
+  bool is_index_key_result = false;


Done. Also renamed "ScanAfterBinarySeek" -> "FindKeyAfterBinarySeek" to make it fit in better.

ajkr · 2020-06-10T18:52:09Z

table/block_based/block.cc

+                                            const Comparator* comp) {
+  // Linear search (within restart block) for first key >= target
+  SeekToRestartPoint(index);
+  Next();


Agree, done.

ajkr · 2020-06-10T18:54:00Z

table/block_based/block.cc

+      max_offset = port::kMaxUint32;
+    }
+    while (true) {
+      Next();


It's making use of polymorphism here -- IndexBlockIter::Next() calls ParseNextIndexKey() while DataBlockIter::Next() calls ParseNextDataKey().

ajkr · 2020-06-10T18:54:10Z

table/block_based/block.cc

  *index = left;
+  if (*index == 0) {
+    // Special case as we land at zero as long as restart key at index 1 is >=


Right, done.

facebook-github-bot · 2020-06-10T19:01:00Z

@ajkr has updated the pull request. Re-import the pull request

facebook-github-bot

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-06-11T00:39:02Z

@ajkr merged this pull request in e6be168.

Summary: This is a followup to #6646. In that PR, for simplicity I just appended a comparison against the 0th restart key in case `BinarySeek()`'s binary search landed at index 0. As a result there were `2/(N+1) + log_2(N)` key comparisons. This PR does it differently. Now we expand the binary search range by one so it also covers the case where target is at or before the restart key at index 0. As a result, it involves `log_2(N+1)` key comparisons. Pull Request resolved: #7068 Test Plan: ran readrandom with mostly default settings and counted key comparisons using `PerfContext`. before: `user_key_comparison_count = 28881965` after: `user_key_comparison_count = 27823245` setup command: ``` $ TEST_TMPDIR=/dev/shm/dbbench ./db_bench -benchmarks=fillrandom,compact -write_buffer_size=1048576 -target_file_size_base=1048576 -max_bytes_for_level_base=4194304 -max_background_jobs=12 -level_compaction_dynamic_level_bytes=true -num=10000000 ``` benchmark command: ``` $ TEST_TMPDIR=/dev/shm/dbbench/ ./db_bench -use_existing_db=true -benchmarks=readrandom -disable_auto_compactions=true -num=10000000 -compression_type=none -reads=1000000 -perf_level=3 ``` Reviewed By: anand1976 Differential Revision: D22357032 Pulled By: ajkr fbshipit-source-id: 8b01e9c1c2a4e9d02fc9dfe16c1cc0327f8bdf24

Summary: This is a followup to facebook#6646. In that PR, for simplicity I just appended a comparison against the 0th restart key in case `BinarySeek()`'s binary search landed at index 0. As a result there were `2/(N+1) + log_2(N)` key comparisons. This PR does it differently. Now we expand the binary search range by one so it also covers the case where target is at or before the restart key at index 0. As a result, it involves `log_2(N+1)` key comparisons. Pull Request resolved: facebook#7068 Test Plan: ran readrandom with mostly default settings and counted key comparisons using `PerfContext`. before: `user_key_comparison_count = 28881965` after: `user_key_comparison_count = 27823245` setup command: ``` $ TEST_TMPDIR=/dev/shm/dbbench ./db_bench -benchmarks=fillrandom,compact -write_buffer_size=1048576 -target_file_size_base=1048576 -max_bytes_for_level_base=4194304 -max_background_jobs=12 -level_compaction_dynamic_level_bytes=true -num=10000000 ``` benchmark command: ``` $ TEST_TMPDIR=/dev/shm/dbbench/ ./db_bench -use_existing_db=true -benchmarks=readrandom -disable_auto_compactions=true -num=10000000 -compression_type=none -reads=1000000 -perf_level=3 ``` Reviewed By: anand1976 Differential Revision: D22357032 Pulled By: ajkr fbshipit-source-id: 8b01e9c1c2a4e9d02fc9dfe16c1cc0327f8bdf24

facebook-github-bot added the CLA Signed label Apr 3, 2020

facebook-github-bot reviewed Apr 3, 2020

View reviewed changes

ajkr force-pushed the optimize-key-comparison branch from d1534c0 to a3881b2 Compare April 3, 2020 23:00

facebook-github-bot reviewed Apr 3, 2020

View reviewed changes

ajkr changed the title ~~save a key comparison in DataBlockIter::Seek()~~ [wip] save a key comparison in DataBlockIter::Seek() Apr 3, 2020

ajkr force-pushed the optimize-key-comparison branch from a3881b2 to 430f3bc Compare April 4, 2020 00:07

ajkr force-pushed the optimize-key-comparison branch from 430f3bc to fcd0a44 Compare April 4, 2020 04:59

ajkr changed the title ~~[wip] save a key comparison in DataBlockIter::Seek()~~ save a key comparison in DataBlockIter::Seek() Apr 4, 2020

facebook-github-bot reviewed Apr 4, 2020

View reviewed changes

ajkr force-pushed the optimize-key-comparison branch from fcd0a44 to 262a7e2 Compare April 6, 2020 00:21

ajkr changed the title ~~save a key comparison in DataBlockIter::Seek()~~ save a key comparison in delta-encoded data block seeks Apr 6, 2020

ajkr force-pushed the optimize-key-comparison branch from 262a7e2 to 92af65f Compare April 6, 2020 00:23

facebook-github-bot reviewed Apr 6, 2020

View reviewed changes

ajkr requested a review from siying April 6, 2020 18:12

ajkr force-pushed the optimize-key-comparison branch from 92af65f to 4e67eb7 Compare April 11, 2020 22:18

ajkr changed the title ~~save a key comparison in delta-encoded data block seeks~~ save a key comparison in block seeks Apr 11, 2020

ajkr force-pushed the optimize-key-comparison branch from 4e67eb7 to 397469b Compare April 11, 2020 22:54

ajkr force-pushed the optimize-key-comparison branch from 397469b to efbc981 Compare April 11, 2020 22:57

facebook-github-bot reviewed Apr 11, 2020

View reviewed changes

ajkr force-pushed the optimize-key-comparison branch from efbc981 to 1db34e7 Compare April 12, 2020 01:36

ajkr force-pushed the optimize-key-comparison branch from 3302eeb to 50f890b Compare June 8, 2020 19:46

ajkr commented Jun 8, 2020

View reviewed changes

ajkr force-pushed the optimize-key-comparison branch from 50f890b to c55640a Compare June 8, 2020 19:58

facebook-github-bot reviewed Jun 8, 2020

View reviewed changes

pdillinger approved these changes Jun 8, 2020

View reviewed changes

table/block_based/block.cc Outdated Show resolved Hide resolved

table/block_based/block.cc Outdated Show resolved Hide resolved

ajkr commented Jun 9, 2020

View reviewed changes

table/block_based/block.cc Outdated Show resolved Hide resolved

table/block_based/block.cc Outdated Show resolved Hide resolved

ajkr force-pushed the optimize-key-comparison branch from c55640a to 7f6c018 Compare June 9, 2020 20:54

facebook-github-bot reviewed Jun 9, 2020

View reviewed changes

anand1976 approved these changes Jun 10, 2020

View reviewed changes

ajkr added 3 commits June 10, 2020 11:58

address comments

acb69e1

address comments

463f274

ajkr force-pushed the optimize-key-comparison branch from 7f6c018 to 463f274 Compare June 10, 2020 18:59

ajkr commented Jun 10, 2020

View reviewed changes

facebook-github-bot reviewed Jun 10, 2020

View reviewed changes

facebook-github-bot closed this in e6be168 Jun 10, 2020

facebook-github-bot added the Merged label Jun 11, 2020

ajkr mentioned this pull request Jul 2, 2020

save key comparisons in BlockIter::BinarySeek #7068

Closed

save a key comparison in block seeks #6646

save a key comparison in block seeks #6646

Conversation

ajkr commented Apr 3, 2020 • edited

facebook-github-bot left a comment

Choose a reason for hiding this comment

ajkr commented Apr 3, 2020 • edited

facebook-github-bot commented Apr 3, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Apr 4, 2020

facebook-github-bot commented Apr 4, 2020

ajkr commented Apr 4, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Apr 6, 2020

facebook-github-bot commented Apr 6, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Apr 11, 2020

facebook-github-bot commented Apr 11, 2020

facebook-github-bot commented Apr 11, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jun 8, 2020

Choose a reason for hiding this comment

ajkr left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jun 8, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

pdillinger left a comment

Choose a reason for hiding this comment

pdillinger commented Jun 8, 2020

ajkr left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jun 9, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

anand1976 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jun 10, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jun 11, 2020

ajkr commented Apr 3, 2020 •

edited

ajkr commented Apr 3, 2020 •

edited