Reuse data block iterator in BlockBasedTableReader::MultiGet() #5314

anand1976 · 2019-05-16T20:06:22Z

Instead of creating a new DataBlockIterator for every key in a MultiGet batch, reuse it if the next key is in the same block. This results in a small 1-2% cpu improvement.

TEST_TMPDIR=/dev/shm/multiget numactl -C 10 ./db_bench.tmp -use_existing_db=true -benchmarks="readseq,multireadrandom" -write_buffer_size=4194304 -target_file_size_base=4194304 -max_bytes_for_level_base=16777216 -num=12000000 -reads=12000000 -duration=90 -threads=1 -compression_type=none -cache_size=4194304000 -batch_size=32 -disable_auto_compactions=true -bloom_bits=10 -cache_index_and_filter_blocks=true -pin_l0_filter_and_index_blocks_in_cache=true -multiread_batched=true -multiread_stride=4

Without the change -
multireadrandom : 3.066 micros/op 326122 ops/sec; (29375968 of 29375968 found)

With the change -
multireadrandom : 3.003 micros/op 332945 ops/sec; (29983968 of 29983968 found)

Test:
make check
asan_crash
asan_check

sagar0

lgtm.

Can you mention the db_bench options that you used to run this multigetrandom benchmark in the summary, so that they could be part of the commit message.

siying · 2019-05-16T22:03:11Z

table/block_based_table_reader.cc

@@ -2872,6 +2872,8 @@ void BlockBasedTable::MultiGet(const ReadOptions& read_options,
      iiter_unique_ptr.reset(iiter);
    }

+    std::unique_ptr<DataBlockIter> biter(new DataBlockIter());;


Can we avoid this allocation? We used to put it in stack and now we regress to heap allocation.

@siying Curious what's the problem with DataBlockIter being on heap here?

@sagar0 we need to call malloc. It's something we general want to avoid in critical paths.

Looks like if we have a Reset() method for DataBlockIter, then we do not have to perform dynamic memory allocation.

Oh that's a good point. Let me see how to avoid it

It appears that in BlockBasedTableIterator, we don't even call Reset(), and just call NewDataBlockIterator() to move on to a new data block.

riversand963

LGTM if we can avoid heap allocation.

riversand963 · 2019-05-16T22:20:28Z

table/block_based_table_reader.cc

@@ -2872,6 +2872,8 @@ void BlockBasedTable::MultiGet(const ReadOptions& read_options,
      iiter_unique_ptr.reset(iiter);
    }

+    std::unique_ptr<DataBlockIter> biter(new DataBlockIter());;


Looks like if we have a Reset() method for DataBlockIter, then we do not have to perform dynamic memory allocation.

siying · 2019-05-16T22:49:34Z

table/block_based_table_reader.cc

-        NewDataBlockIterator<DataBlockIter>(
-            rep_, read_options, iiter->value(), &biter, false,
-            true /* key_includes_seq */, get_context);
+        if (iiter->value().offset() != bhandle.offset()) {


What's the initial value of bhandle.offset() before it is initialized? If it is 0, how if iiter->value().offset() is also 0?

Yes, that's a good point. It looks like the default constructor initializes offset_ to 0. To make it more robust, I'll check both offset and size and initialize bhandle to kNullBlockHandle.

siying · 2019-05-16T23:09:05Z

table/block_based_table_reader.cc

-            rep_, read_options, iiter->value(), &biter, false,
-            true /* key_includes_seq */, get_context);
+        if (iiter->value().offset() != bhandle.offset()) {
+          bhandle = iiter->value();


Nit: It seems that we only need to store offset here and we don't need to copy the whole handle.

siying

LGTM

siying · 2019-05-18T00:22:26Z

table/block_based_table_reader.cc

-            rep_, read_options, iiter->value(), &biter, false,
-            true /* key_includes_seq */, get_context);
+        if (iiter->value().offset() != bhandle.offset() ||
+            iiter->value().size() != bhandle.size()) {


I believe offset is enough. We can skip the size check. We already do it when reseeking in iterator since long ago. In this way, actually only offset of the previous block handle needs to be stored.

Actually I think 0 is a valid offset. I just got a failure in asan_crash because of it. But I believe we can just check for offset by initializing it to ULONG_MAX.

Yes 0 is a valid offset. I agree that max int is a good idea. Make sure you use the one defined in port.h.

anand1976 · 2019-05-22T21:22:55Z

Found a bug and fixed in the latest commit. When reusing a data block for a key, the ref count needs to be incremented for the corresponding block cache handle, and we need to register a cleanup function so the ref count can be decrement when the PinnableSlice is deleted.

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

facebook-github-bot

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

facebook-github-bot · 2019-06-10T21:09:18Z

@anand1976 merged this pull request in 63ace8e.

…ook#5314) Summary: Instead of creating a new DataBlockIterator for every key in a MultiGet batch, reuse it if the next key is in the same block. This results in a small 1-2% cpu improvement. TEST_TMPDIR=/dev/shm/multiget numactl -C 10 ./db_bench.tmp -use_existing_db=true -benchmarks="readseq,multireadrandom" -write_buffer_size=4194304 -target_file_size_base=4194304 -max_bytes_for_level_base=16777216 -num=12000000 -reads=12000000 -duration=90 -threads=1 -compression_type=none -cache_size=4194304000 -batch_size=32 -disable_auto_compactions=true -bloom_bits=10 -cache_index_and_filter_blocks=true -pin_l0_filter_and_index_blocks_in_cache=true -multiread_batched=true -multiread_stride=4 Without the change - multireadrandom : 3.066 micros/op 326122 ops/sec; (29375968 of 29375968 found) With the change - multireadrandom : 3.003 micros/op 332945 ops/sec; (29983968 of 29983968 found) Pull Request resolved: facebook#5314 Differential Revision: D15742108 Pulled By: anand1976 fbshipit-source-id: 220fb0b8eea9a0d602ddeb371528f7af7936d771

anand1976 requested review from siying and riversand963 May 16, 2019 20:06

facebook-github-bot added the CLA Signed label May 16, 2019

sagar0 approved these changes May 16, 2019

View reviewed changes

siying reviewed May 16, 2019

View reviewed changes

riversand963 reviewed May 16, 2019

View reviewed changes

siying reviewed May 16, 2019

View reviewed changes

siying approved these changes May 16, 2019

View reviewed changes

siying approved these changes May 18, 2019

View reviewed changes

anand1976 force-pushed the mget_reuse_block branch from 9781ba5 to 3614d10 Compare June 3, 2019 22:02

anand76 added 6 commits June 9, 2019 20:25

Reuse data block iterator in BlockBasedTableReader::MultiGet()

40bb9f1

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

Remove memory allocation for DataBlockIter

c1b44f8

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

More robust checking for block handle equality

3ec73f3

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

Increment/decrement ref count when reusing data block

a3e1bdb

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

Resolve merge conflicts

f2e8765

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

Rebase and resolve merge conflicts

cc8177b

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

anand1976 force-pushed the mget_reuse_block branch from 3614d10 to cc8177b Compare June 10, 2019 17:46

facebook-github-bot reviewed Jun 10, 2019

View reviewed changes

riversand963 approved these changes Jun 10, 2019

View reviewed changes

facebook-github-bot closed this in 63ace8e Jun 10, 2019

facebook-github-bot added the Merged label Jun 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reuse data block iterator in BlockBasedTableReader::MultiGet() #5314

Reuse data block iterator in BlockBasedTableReader::MultiGet() #5314

anand1976 commented May 16, 2019 •

edited

Loading

sagar0 left a comment

siying May 16, 2019

sagar0 May 16, 2019

siying May 16, 2019

riversand963 May 16, 2019

anand1976 May 16, 2019

siying May 16, 2019

riversand963 left a comment

riversand963 May 16, 2019

siying May 16, 2019

anand1976 May 17, 2019

siying May 16, 2019

siying left a comment

siying May 18, 2019

anand1976 May 22, 2019

siying May 22, 2019

anand1976 commented May 22, 2019

facebook-github-bot left a comment

facebook-github-bot commented Jun 10, 2019

Reuse data block iterator in BlockBasedTableReader::MultiGet() #5314

Reuse data block iterator in BlockBasedTableReader::MultiGet() #5314

Conversation

anand1976 commented May 16, 2019 • edited Loading

sagar0 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

riversand963 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

siying 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

anand1976 commented May 22, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jun 10, 2019

anand1976 commented May 16, 2019 •

edited

Loading