Fix a major performance bug in 7.0 re: filter compatibility #9736

pdillinger · 2022-03-22T20:23:39Z

Summary: Bloom filters generated by pre-7.0 releases are not read by
7.0.x releases (and vice-versa) due to changes to FilterPolicy::Name()
in #9590. This can severely impact read performance and read I/O on
upgrade or downgrade with existing DB, but not data correctness.

To fix, we go back using the old, unified name in SST metadata but (for
a while anyway) recognize the aliases that could be generated by early
7.0.x releases. This unfortunately requires a public API change to avoid
interfering with all the good changes from #9590, but the API change
only affects users with custom FilterPolicy, which should be very few.

Test Plan: manual

Generate DBs with

./db_bench.7.0 -db=/dev/shm/rocksdb.7.0 -bloom_bits=10 -cache_index_and_filter_blocks=1 -benchmarks=fillrandom -num=10000000 -compaction_style=2 -fifo_compaction_max_table_files_size_mb=10000 -fifo_compaction_allow_compaction=0

and similar. Compare with

for IMPL in 6.29 7.0 fixed; do for DB in 6.29 7.0 fixed; do echo "Testing $IMPL on $DB:"; ./db_bench.$IMPL -db=/dev/shm/rocksdb.$DB -use_existing_db -readonly -bloom_bits=10 -benchmarks=readrandom -num=10000000 -compaction_style=2 -fifo_compaction_max_table_files_size_mb=10000 -fifo_compaction_allow_compaction=0 -duration=10 2>&1 | grep micros/op; done; done

Results:

Testing 6.29 on 6.29:
readrandom   :      34.381 micros/op 29085 ops/sec;    3.2 MB/s (291999 of 291999 found)
Testing 6.29 on 7.0:
readrandom   :     190.443 micros/op 5249 ops/sec;    0.6 MB/s (52999 of 52999 found)
Testing 6.29 on fixed:
readrandom   :      40.148 micros/op 24907 ops/sec;    2.8 MB/s (249999 of 249999 found)
Testing 7.0 on 6.29:
readrandom   :     229.430 micros/op 4357 ops/sec;    0.5 MB/s (43999 of 43999 found)
Testing 7.0 on 7.0:
readrandom   :      33.348 micros/op 29986 ops/sec;    3.3 MB/s (299999 of 299999 found)
Testing 7.0 on fixed:
readrandom   :     152.734 micros/op 6546 ops/sec;    0.7 MB/s (65999 of 65999 found)
Testing fixed on 6.29:
readrandom   :      32.024 micros/op 31224 ops/sec;    3.5 MB/s (312999 of 312999 found)
Testing fixed on 7.0:
readrandom   :      33.990 micros/op 29390 ops/sec;    3.3 MB/s (294999 of 294999 found)
Testing fixed on fixed:
readrandom   :      28.714 micros/op 34825 ops/sec;    3.9 MB/s (348999 of 348999 found)

Just paying attention to order of magnitude of ops/sec (short test
durations, lots of noise), it's clear that with the fix we can read <= 6.29
& >= 7.0 at full speed, where neither 6.29 nor 7.0 can on both. And 6.29
release can properly read fixed DB at full speed.

Summary: Bloom filters generated by pre-7.0 releases are not read by 7.0.x releases (and vice-versa) due to changes to FilterPolicy::Name() in facebook#9590. This can severely impact read performance and read I/O on upgrade or downgrade with existing DB, but not data correctness. To fix, we go back using the old, unified name in SST metadata but (for a while anyway) recognize the aliases that could be generated by early 7.0.x releases. This unfortunately requires a public API change to avoid interfering with all the good changes from facebook#9590, but the API change only affects users with custom FilterPolicy, which should be very few. Test Plan: manual Generate DBs with ``` ./db_bench.7.0 -db=/dev/shm/rocksdb.7.0 -bloom_bits=10 -cache_index_and_filter_blocks=1 -benchmarks=fillrandom -num=10000000 -compaction_style=2 -fifo_compaction_max_table_files_size_mb=10000 -fifo_compaction_allow_compaction=0 ``` and similar. Compare with ``` for IMPL in 6.29 7.0 fixed; do for DB in 6.29 7.0 fixed; do echo "Testing $IMPL on $DB:"; ./db_bench.$IMPL -db=/dev/shm/rocksdb.$DB -use_existing_db -readonly -bloom_bits=10 -benchmarks=readrandom -num=10000000 -compaction_style=2 -fifo_compaction_max_table_files_size_mb=10000 -fifo_compaction_allow_compaction=0 -duration=10 2>&1 | grep micros/op; done; done ``` Results: ``` Testing 6.29 on 6.29: readrandom : 34.381 micros/op 29085 ops/sec; 3.2 MB/s (291999 of 291999 found) Testing 6.29 on 7.0: readrandom : 190.443 micros/op 5249 ops/sec; 0.6 MB/s (52999 of 52999 found) Testing 6.29 on fixed: readrandom : 40.148 micros/op 24907 ops/sec; 2.8 MB/s (249999 of 249999 found) Testing 7.0 on 6.29: readrandom : 229.430 micros/op 4357 ops/sec; 0.5 MB/s (43999 of 43999 found) Testing 7.0 on 7.0: readrandom : 33.348 micros/op 29986 ops/sec; 3.3 MB/s (299999 of 299999 found) Testing 7.0 on fixed: readrandom : 152.734 micros/op 6546 ops/sec; 0.7 MB/s (65999 of 65999 found) Testing fixed on 6.29: readrandom : 32.024 micros/op 31224 ops/sec; 3.5 MB/s (312999 of 312999 found) Testing fixed on 7.0: readrandom : 33.990 micros/op 29390 ops/sec; 3.3 MB/s (294999 of 294999 found) Testing fixed on fixed: readrandom : 28.714 micros/op 34825 ops/sec; 3.9 MB/s (348999 of 348999 found) ``` Just paying attention to order of magnitude of ops/sec (short test durations, lots of noise), it's clear that with the fix we can read <= 6.29 & >= 7.0 at full speed, where neither 6.29 nor 7.0 can on both. And 6.29 release can properly read fixed DB at full speed.

facebook-github-bot · 2022-03-22T20:24:35Z

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

facebook-github-bot · 2022-03-22T21:16:44Z

@pdillinger has updated the pull request. You must reimport the pull request before landing.

ajkr

Will finish review before it lands in fbcode.

facebook-github-bot · 2022-03-22T21:19:45Z

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

siying · 2022-03-22T23:15:11Z

table/block_based/block_based_table_reader.cc

+            }
+          }
+        }
+      } else {


This is so complicated. Are we going to take the risk? Can we just do two FindMetaBlock() calls instead? At least in this minor release?

Ignore this comment. Now I got that it's more than two.

siying · 2022-03-22T23:26:19Z

table/block_based/block_based_table_reader.cc

+            }
+          }
+        }
+      } else {


Ignore this comment. Now I got that it's more than two.

siying · 2022-03-22T23:26:48Z

table/block_based/block_based_table_reader.cc

+            test::Standard128RibbonFilterPolicy::kClassName(),
+            DeprecatedBlockBasedBloomFilterPolicy::kClassName(),
+            BloomFilterPolicy::kClassName(),
+            RibbonFilterPolicy::kClassName(),


Are they all needed? Can we remove some that we know that are not in use?

I'm pretty sure the test:: ones are not needed for production, but I'd prefer to be conservative in case I've missed some detail.

ajkr · 2022-03-22T23:45:20Z

Will finish review before it lands in fbcode.

I finished reviewing on D35060074. LGTM.

mrambacher · 2022-03-22T22:46:02Z

table/block_based/block_based_table_reader.cc

+          std::make_pair(Rep::FilterType::kPartitionedFilter,
+                         kPartitionedFilterBlockPrefix),
+          std::make_pair(Rep::FilterType::kBlockFilter, kFilterBlockPrefix)}) {
+      if (strcmp(name, BuiltinFilterPolicy::kCompatibilityName()) == 0) {


Why not do this strcmp once rather than on every loop? I

mrambacher · 2022-03-23T00:06:04Z

include/rocksdb/filter_policy.h

+  // The name used for identifying whether a filter on disk is readable
+  // by this FilterPolicy. This e.g. allows BloomFilterPolcy and
+  // RibbonFilterPolicy to have distinct Customizable names but able to
+  // read each others filters. Filter policies not based on built-in
+  // implementations may return Name(), but this is pure virtual so that
+  // wrappers around built-in policies are prompted to defer to
+  // CompatibilityName() of the wrapped policy.
+  virtual const char* CompatibilityName() const = 0;


I find this comment confusing and not sure what I would do if I needed to implement this, except to just make it return Name(). Why not have the default implementation return Name() and override it for the implementations that require it?

Because that does the WRONG thing by default for wrappers of built-in policy, which should be most common case

I understand it does the wrong thing for the builtins, but since RocksDB controls those, they are easier to test and verify they do the right thing, rather than assuming someone who writes their own having to understand what to do.

And if there is a custom one out there, this change breaks compile-time compatibility.

mrambacher · 2022-03-23T00:09:00Z

table/block_based/block_based_table_reader.cc

+        // It should be OK to remove this in the future and just have the
+        // 'else' code.


Please clarify under what conditions can this be removed.

The passage of time (a minimum of "bad" files still around)

facebook-github-bot · 2022-03-23T14:57:59Z

@pdillinger has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-03-23T15:07:11Z

@pdillinger has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-03-23T15:07:29Z

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

facebook-github-bot · 2022-03-23T16:04:10Z

@pdillinger has updated the pull request. You must reimport the pull request before landing.

pdillinger · 2022-03-23T17:07:38Z

I also tested -format_version=4 for legacy Bloom filter and -use_ribbon_filter for Ribbon filter. Both work as expected with the fix. Also as discussed internally, table properties use new names:

$ sst_dump --file=/dev/shm/rocksdb.fixed/000033.sst --show_properties | grep 'filter policy'
  filter policy name: ribbonfilter

…#9736) Summary: Bloom filters generated by pre-7.0 releases are not read by 7.0.x releases (and vice-versa) due to changes to FilterPolicy::Name() in facebook#9590. This can severely impact read performance and read I/O on upgrade or downgrade with existing DB, but not data correctness. To fix, we go back using the old, unified name in SST metadata but (for a while anyway) recognize the aliases that could be generated by early 7.0.x releases. This unfortunately requires a public API change to avoid interfering with all the good changes from facebook#9590, but the API change only affects users with custom FilterPolicy, which should be very few. Pull Request resolved: facebook#9736 Test Plan: manual Generate DBs with ``` ./db_bench.7.0 -db=/dev/shm/rocksdb.7.0 -bloom_bits=10 -cache_index_and_filter_blocks=1 -benchmarks=fillrandom -num=10000000 -compaction_style=2 -fifo_compaction_max_table_files_size_mb=10000 -fifo_compaction_allow_compaction=0 ``` and similar. Compare with ``` for IMPL in 6.29 7.0 fixed; do for DB in 6.29 7.0 fixed; do echo "Testing $IMPL on $DB:"; ./db_bench.$IMPL -db=/dev/shm/rocksdb.$DB -use_existing_db -readonly -bloom_bits=10 -benchmarks=readrandom -num=10000000 -compaction_style=2 -fifo_compaction_max_table_files_size_mb=10000 -fifo_compaction_allow_compaction=0 -duration=10 2>&1 | grep micros/op; done; done ``` Results: ``` Testing 6.29 on 6.29: readrandom : 34.381 micros/op 29085 ops/sec; 3.2 MB/s (291999 of 291999 found) Testing 6.29 on 7.0: readrandom : 190.443 micros/op 5249 ops/sec; 0.6 MB/s (52999 of 52999 found) Testing 6.29 on fixed: readrandom : 40.148 micros/op 24907 ops/sec; 2.8 MB/s (249999 of 249999 found) Testing 7.0 on 6.29: readrandom : 229.430 micros/op 4357 ops/sec; 0.5 MB/s (43999 of 43999 found) Testing 7.0 on 7.0: readrandom : 33.348 micros/op 29986 ops/sec; 3.3 MB/s (299999 of 299999 found) Testing 7.0 on fixed: readrandom : 152.734 micros/op 6546 ops/sec; 0.7 MB/s (65999 of 65999 found) Testing fixed on 6.29: readrandom : 32.024 micros/op 31224 ops/sec; 3.5 MB/s (312999 of 312999 found) Testing fixed on 7.0: readrandom : 33.990 micros/op 29390 ops/sec; 3.3 MB/s (294999 of 294999 found) Testing fixed on fixed: readrandom : 28.714 micros/op 34825 ops/sec; 3.9 MB/s (348999 of 348999 found) ``` Just paying attention to order of magnitude of ops/sec (short test durations, lots of noise), it's clear that with the fix we can read <= 6.29 & >= 7.0 at full speed, where neither 6.29 nor 7.0 can on both. And 6.29 release can properly read fixed DB at full speed. Reviewed By: siying, ajkr Differential Revision: D35057844 Pulled By: pdillinger fbshipit-source-id: a46893a6af4bf084375ebe4728066d00eb08f050

Summary: This change adds two unit tests that would each catch the regression fixed in facebook#9736 * TableMetaIndexKeys - detects any churn in metaindex block keys generated by SST files using standard db_test_util configurations. * BloomFilterCompatibility - this detects if any common built-in FilterPolicy configurations fail to read filters generated by another. (The regression bug caused NewRibbonFilterPolicy not to read filters from NewBloomFilterPolicy and vice-versa.) This replaces some previous tests that didn't really appear to be testing much of anything except basic data correctness, which doesn't tell you a filter is being used. Light refactoring in meta_blocks.cc/h to support inspecting metaindex keys. Test Plan: this is the test

Summary: This change adds two unit tests that would each catch the regression fixed in #9736 * TableMetaIndexKeys - detects any churn in metaindex block keys generated by SST files using standard db_test_util configurations. * BloomFilterCompatibility - this detects if any common built-in FilterPolicy configurations fail to read filters generated by another. (The regression bug caused NewRibbonFilterPolicy not to read filters from NewBloomFilterPolicy and vice-versa.) This replaces some previous tests that didn't really appear to be testing much of anything except basic data correctness, which doesn't tell you a filter is being used. Light refactoring in meta_blocks.cc/h to support inspecting metaindex keys. Pull Request resolved: #9773 Test Plan: this is the test. Verified that 7.0.2 fails both tests and 7.0.3 passes. With backporting for intentional API changes in 7.0, 6.29 also passes. Reviewed By: ajkr Differential Revision: D35236248 Pulled By: pdillinger fbshipit-source-id: 493dfe9ad7e27524bf7c6c1af8a4b8c31bc6ef5a

pdillinger requested review from mrambacher and ajkr March 22, 2022 20:23

facebook-github-bot added the CLA Signed label Mar 22, 2022

Some test fixes, and log Name() to TableProperties

e1627ec

ajkr approved these changes Mar 22, 2022

View reviewed changes

siying reviewed Mar 22, 2022

View reviewed changes

siying approved these changes Mar 22, 2022

View reviewed changes

ajkr mentioned this pull request Mar 23, 2022

rocksdbjni 6.29.4 and 7.0.3 are missing from maven central #9689

Closed

mrambacher approved these changes Mar 23, 2022

View reviewed changes

pdillinger added 2 commits March 23, 2022 07:52

Improve comments

992fe50

Merge remote-tracking branch 'origin/main' into fix_filter_policy_names

ab1add6

Optimize strcmp

0a2c0ce

Trivial commit to try to trigger CircleCI

f479a6c

facebook-github-bot closed this in 91687d7 Mar 23, 2022

pdillinger mentioned this pull request Mar 29, 2022

Tests for filter compatibility #9773

Closed

pdillinger mentioned this pull request Apr 15, 2022

Multiple crashes with rocksdbjni 7.0.4 #9826

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a major performance bug in 7.0 re: filter compatibility #9736

Fix a major performance bug in 7.0 re: filter compatibility #9736

pdillinger commented Mar 22, 2022

facebook-github-bot commented Mar 22, 2022

facebook-github-bot commented Mar 22, 2022

ajkr left a comment

facebook-github-bot commented Mar 22, 2022

siying Mar 22, 2022

siying Mar 22, 2022

siying Mar 22, 2022

siying Mar 22, 2022

pdillinger Mar 23, 2022

ajkr commented Mar 22, 2022

mrambacher Mar 22, 2022

pdillinger Mar 23, 2022

mrambacher Mar 23, 2022

pdillinger Mar 23, 2022

mrambacher Mar 23, 2022

mrambacher Mar 23, 2022

pdillinger Mar 23, 2022 •

edited

Loading

facebook-github-bot commented Mar 23, 2022

facebook-github-bot commented Mar 23, 2022

facebook-github-bot commented Mar 23, 2022

facebook-github-bot commented Mar 23, 2022

pdillinger commented Mar 23, 2022

		// It should be OK to remove this in the future and just have the
		// 'else' code.

Fix a major performance bug in 7.0 re: filter compatibility #9736

Fix a major performance bug in 7.0 re: filter compatibility #9736

Conversation

pdillinger commented Mar 22, 2022

facebook-github-bot commented Mar 22, 2022

facebook-github-bot commented Mar 22, 2022

ajkr left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 22, 2022

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 commented Mar 22, 2022

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

pdillinger Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented Mar 23, 2022

facebook-github-bot commented Mar 23, 2022

facebook-github-bot commented Mar 23, 2022

facebook-github-bot commented Mar 23, 2022

pdillinger commented Mar 23, 2022

pdillinger Mar 23, 2022 •

edited

Loading