Split Block into multiple classes by mrambacher · Pull Request #8690 · facebook/rocksdb

mrambacher · 2021-08-22T23:39:25Z

Makes the Block class into a base class with different implementations (DataBlock, MetaBlock, IndexBlock, etc).

This change can allow different implementations of these base classes for use for different iterator implementations. For example, it is possible to create different data block and iterator implementations that un-"delta encode" the values, which may yield performance improvements under certain circumstances.

By making different subclasses of Block, different implementations can be provided for different functionality. For example, it is possible to create different types of DataBlocks where the data is pre-parsed or the iterators behave differently.

This reverts commit d5e8bb8.

pdillinger

Looks pretty promising so far 👍

pdillinger · 2021-08-24T15:19:45Z

+  const char* data_;      // contents_.data.data()
+  uint32_t limit_;        // contents_.data.size()


I have never understood why these are duplicated from contents_, costing 12 bytes per cached block in memory (0.1 - 0.3% ish?)

I see limit_ is set to 0 if size is too small, but why not just use the < sizeof(uin32_t) in more places or throw away the bad BlockContents and replace it with empty?

I'm slightly concerned about CPU regression due to more virtual calls (and many conditionals remaining, not encoded in the virtual call), but this seems like a good "pay as you go" performance opportunity.

pdillinger · 2021-09-27T18:40:48Z

Still Work In Progress?

mrambacher · 2021-09-28T13:57:17Z

Still Work In Progress?

This is "ready for review" now. This PR is phase 1 in a multi-step process to allow different implementations of Block (specifically DataBlock) to test performance. Phase 2 will be allowing different implementations of DataBlockIter and Phase 3 will be testing different implementations to see if we can improve performance.

pdillinger

Some minor fixes & recommendations. I would not be surprised if there is some subtle regression in here that is not caught by unit tests. Please run crash test locally for some hours before shipping.

pdillinger · 2021-09-28T21:01:02Z

  options.create_if_missing = true;
-  options.prefix_extractor.reset(NewFixedPrefixTransform(20));
+  // Picking a Capped prefix of 3 so that the short key ("foo") fits
+  options.prefix_extractor.reset(NewCappedPrefixTransform(3));


This seems to fundamentally violate the point of the test "prefix longer than key"

pdillinger · 2021-09-28T21:58:30Z

  options.create_if_missing = true;
-  options.prefix_extractor.reset(NewCappedPrefixTransform(5));
+  // Picking a Capped prefix of 3 so that the short key ("foo") fits
+  options.prefix_extractor.reset(NewCappedPrefixTransform(3));


I'm not sure what the purpose of the original code is but changing it should be unnecessary. See #8529 (comment)

pdillinger · 2021-09-29T19:19:50Z

  Status decode_s __attribute__((__unused__)) = decoded_value_.DecodeFrom(
      &v, have_first_key_,
-      (value_delta_encoded_ && shared) ? &decoded_value_.handle : nullptr);
+      (value_delta_encoded_ && is_shared) ? &decoded_value_.handle : nullptr);


We have seen a couple of times in stress test violation of the final assertion in DecodeEntry. (Internal ref T100361844.) In looking into that, I noticed that our test suite never reaches this point in the code with global_seqno_state_ != nullptr && !value_delta_encoded_. Since you are refactoring this code, it would be nice to add coverage if we can, potentially find any bugs old and/or new.

pdillinger · 2021-09-29T19:25:31Z

-      if ((p = GetVarint32Ptr(p, limit, value_length)) == nullptr) {
-        return nullptr;
-      }
+const char* Block::DecodeEntry(const char* p, const char* limit,


Recommend keeping 'inline' (and more below)

ajkr · 2021-09-29T20:39:49Z

I would not be surprised if there is some subtle regression in here that is not caught by unit tests. Please run crash test locally for some hours before shipping.

What about performance tests?

mrambacher · 2021-09-29T20:44:45Z

I would not be surprised if there is some subtle regression in here that is not caught by unit tests. Please run crash test locally for some hours before shipping.

What about performance tests?

I have executed the performance tests from the Wiki page (with also revrange and revrangewhilewriting tests) and so no regressions. I can post the results/comparison in the PR if it helps.

I have not executed stress tests but will do so shortly.

ajkr · 2021-09-29T22:43:26Z

Here are the commands I'm using:

$ TEST_TMPDIR=/dev/shm ./db_bench -benchmarks=filluniquerandom,compact -target_file_size_base=1048576 -compression_type=none
$ make clean && EXTRA_LDFLAGS="-fprofile-generate" EXTRA_CXXFLAGS="-fprofile-generate" V=1 USE_LTO=1 DEBUG_LEVEL=0 OPTIMIZE_LEVEL=-O3 make -j48 db_bench
$ TEST_TMPDIR=/dev/shm ./db_bench -readonly=1 -use_existing_db=1 -benchmarks=seekrandom -seek_nexts=1000 -target_file_size_base=1048576 -cache_size=0 -duration=30
$ find . -name '*.o' | xargs rm -f
$ EXTRA_LDFLAGS="-fprofile-use -fprofile-correction" EXTRA_CXXFLAGS="-Wno-error=missing-profile -fprofile-use -fprofile-correction" V=1 USE_LTO=1 DEBUG_LEVEL=0 OPTIMIZE_LEVEL=-O3 make -j48 db_bench
$ TEST_TMPDIR=/dev/shm ./db_bench -readonly=1 -use_existing_db=1 -benchmarks=seekrandom -seek_nexts=1000 -target_file_size_base=1048576 -cache_size=0 -duration=30

Here are the results.

Results before 8690: 962.0MB/s, 930.0MB/s, 916.5MB/s
Results after 8690: 838.5MB/s, 850.6MB/s, 822.5MB/s

I will do a second build and run from scratch to make sure no mistakes.

pdillinger

This is among the most performance-critical areas of code and Andrew's test indicates notable regression. More investigation and testing needed.

ajkr · 2021-09-29T23:20:48Z

Here are the commands I'm using:

$ TEST_TMPDIR=/dev/shm ./db_bench -benchmarks=filluniquerandom,compact -target_file_size_base=1048576 -compression_type=none
$ make clean && EXTRA_LDFLAGS="-fprofile-generate" EXTRA_CXXFLAGS="-fprofile-generate" V=1 USE_LTO=1 DEBUG_LEVEL=0 OPTIMIZE_LEVEL=-O3 make -j48 db_bench
$ TEST_TMPDIR=/dev/shm ./db_bench -readonly=1 -use_existing_db=1 -benchmarks=seekrandom -seek_nexts=1000 -target_file_size_base=1048576 -cache_size=0 -duration=30
$ find . -name '*.o' | xargs rm -f
$ EXTRA_LDFLAGS="-fprofile-use -fprofile-correction" EXTRA_CXXFLAGS="-Wno-error=missing-profile -fprofile-use -fprofile-correction" V=1 USE_LTO=1 DEBUG_LEVEL=0 OPTIMIZE_LEVEL=-O3 make -j48 db_bench
$ TEST_TMPDIR=/dev/shm ./db_bench -readonly=1 -use_existing_db=1 -benchmarks=seekrandom -seek_nexts=1000 -target_file_size_base=1048576 -cache_size=0 -duration=30

Here are the results.

Results before 8690: 962.0MB/s, 930.0MB/s, 916.5MB/s Results after 8690: 838.5MB/s, 850.6MB/s, 822.5MB/s

I will do a second build and run from scratch to make sure no mistakes.

I did a second run with a slight variation (skipping the USE_LTO=1 in the -fprofile-generate command) and then the regression was 2% instead of 10%. Did ten runs each instead of three to make sure the result was stable.

So, will try a third time with the original commands.

ajkr · 2021-09-29T23:48:04Z

Here are the commands I'm using:
$ TEST_TMPDIR=/dev/shm ./db_bench -benchmarks=filluniquerandom,compact -target_file_size_base=1048576 -compression_type=none
$ make clean && EXTRA_LDFLAGS="-fprofile-generate" EXTRA_CXXFLAGS="-fprofile-generate" V=1 USE_LTO=1 DEBUG_LEVEL=0 OPTIMIZE_LEVEL=-O3 make -j48 db_bench
$ TEST_TMPDIR=/dev/shm ./db_bench -readonly=1 -use_existing_db=1 -benchmarks=seekrandom -seek_nexts=1000 -target_file_size_base=1048576 -cache_size=0 -duration=30
$ find . -name '*.o' | xargs rm -f
$ EXTRA_LDFLAGS="-fprofile-use -fprofile-correction" EXTRA_CXXFLAGS="-Wno-error=missing-profile -fprofile-use -fprofile-correction" V=1 USE_LTO=1 DEBUG_LEVEL=0 OPTIMIZE_LEVEL=-O3 make -j48 db_bench
$ TEST_TMPDIR=/dev/shm ./db_bench -readonly=1 -use_existing_db=1 -benchmarks=seekrandom -seek_nexts=1000 -target_file_size_base=1048576 -cache_size=0 -duration=30
Here are the results.
Results before 8690: 962.0MB/s, 930.0MB/s, 916.5MB/s Results after 8690: 838.5MB/s, 850.6MB/s, 822.5MB/s
I will do a second build and run from scratch to make sure no mistakes.
I did a second run with a slight variation (skipping the USE_LTO=1 in the -fprofile-generate command) and then the regression was 2% instead of 10%. Did ten runs each instead of three to make sure the result was stable.

So, will try a third time with the original commands.

My third rebuild from scratch brought back the 10% regression. That is surprising USE_LTO=1 in the -fprofile-generate command makes a big difference, but beside the point. I'd guess you can repro the issue with less fancy (and faster) build commands though I haven't tried yet actually.

ajkr · 2021-09-30T00:51:41Z

Here are some commands I found to exaggerate the regression up to 15% while using simpler build options:

$ TEST_TMPDIR=/dev/shm ./db_bench -benchmarks=filluniquerandom,compact -target_file_size_base=1048576 -compression_type=none
$ make clean && V=1 DEBUG_LEVEL=0 OPTIMIZE_LEVEL=-O3 make -j48 db_bench
$ TEST_TMPDIR=/dev/shm ./db_bench -mmap_read=1 -verify_checksum=false -use_existing_db=1 -benchmarks=seekrandom -seek_nexts=1000 -target_file_size_base=1048576 -cache_size=0 -duration=30

mrambacher added 2 commits August 17, 2021 21:28

Merge to master

75f6f6f

mrambacher requested review from akankshamahajan15 and pdillinger August 22, 2021 23:39

facebook-github-bot added the CLA Signed label Aug 22, 2021

mrambacher added 4 commits August 22, 2021 19:53

Fix build issues

1fcd80a

Move MetaBlockIter into block.cc

d5e8bb8

Revert "Move MetaBlockIter into block.cc"

25da4ea

This reverts commit d5e8bb8.

Make Invalidate a virtual function

de4fb37

pdillinger reviewed Aug 24, 2021

View reviewed changes

zhichao-cao requested review from anand1976 and zhichao-cao August 24, 2021 21:08

mrambacher added 9 commits September 1, 2021 09:32

Merge branch 'master' into DataBlock

def0156

Add prev_entries back to index. Fix tests

2e13ae6

Rename methods to fix some build failures.

82877a3

Merge branch 'main' into DataBlock

7b942b4

Add IsIndexDeltaEncoded to BlockLikeOptions

7545f4e

Make BlockLikeOptions more like a Concept (rather than inheritance)

1b3fba1

Update to latest

03a49a6

Remove data_ element; Fix CMAKE issues

f17cd14

make format

215870e

mrambacher changed the title ~~WIP: Split Block into multiple classes~~ Sep 28, 2021

pdillinger approved these changes Sep 29, 2021

View reviewed changes

pdillinger requested changes Sep 29, 2021

View reviewed changes

Merge to latest

2f3ff7f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Split Block into multiple classes#8690

Split Block into multiple classes#8690
mrambacher wants to merge 16 commits into
facebook:mainfrom
mrambacher:DataBlock

mrambacher commented Aug 22, 2021 •

edited

Loading

pdillinger left a comment

Uh oh!

pdillinger Aug 24, 2021

Uh oh!

Uh oh!

pdillinger commented Sep 27, 2021

mrambacher commented Sep 28, 2021

pdillinger left a comment

pdillinger Sep 28, 2021

pdillinger Sep 28, 2021

pdillinger Sep 29, 2021

pdillinger Sep 29, 2021

ajkr commented Sep 29, 2021

mrambacher commented Sep 29, 2021

ajkr commented Sep 29, 2021 •

edited

Loading

pdillinger left a comment

ajkr commented Sep 29, 2021

ajkr commented Sep 29, 2021 •

edited

Loading

ajkr commented Sep 30, 2021

Labels

4 participants

		const char* data_; // contents_.data.data()
		uint32_t limit_; // contents_.data.size()

Uh oh!

Conversation

mrambacher commented Aug 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

pdillinger left a comment

Choose a reason for hiding this comment

Uh oh!

pdillinger Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pdillinger commented Sep 27, 2021

mrambacher commented Sep 28, 2021

pdillinger left a comment

Choose a reason for hiding this comment

pdillinger Sep 28, 2021

Choose a reason for hiding this comment

pdillinger Sep 28, 2021

Choose a reason for hiding this comment

pdillinger Sep 29, 2021

Choose a reason for hiding this comment

pdillinger Sep 29, 2021

Choose a reason for hiding this comment

ajkr commented Sep 29, 2021

mrambacher commented Sep 29, 2021

ajkr commented Sep 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

pdillinger left a comment

Choose a reason for hiding this comment

ajkr commented Sep 29, 2021

ajkr commented Sep 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ajkr commented Sep 30, 2021

Labels

4 participants

mrambacher commented Aug 22, 2021 •

edited

Loading

ajkr commented Sep 29, 2021 •

edited

Loading

ajkr commented Sep 29, 2021 •

edited

Loading