From d21b2a96997a1f669e30063ee97437dbcd676861 Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Tue, 20 Apr 2021 19:45:08 -0700 Subject: [PATCH] Revert Ribbon starting level support from #8198 (#8212) Summary: This partially reverts commit 10196d7edc2fc5c03553c76acaf1337b5c7c1718. The problem with this change is because of important filter use cases: FIFO compaction and SST writer. FIFO "compaction" always uses level 0 so would only use Ribbon filters if specifically including level 0 for the Ribbon filter policy. SST writer sets level_at_creation=-1 to indicate unknown level, and this would be treated the same as level 0 unless fixed. We are keeping the part about committing to permanent schema, which is only changes to API comments and HISTORY.md. Pull Request resolved: https://github.com/facebook/rocksdb/pull/8212 Test Plan: CI Reviewed By: jay-zhuang Differential Revision: D27896468 Pulled By: pdillinger fbshipit-source-id: 50a775f7cba5d64fb729d9b982e355864020596e --- HISTORY.md | 1 - db_stress_tool/db_stress_common.h | 2 +- db_stress_tool/db_stress_gflags.cc | 4 +- db_stress_tool/db_stress_test_base.cc | 9 ++- include/rocksdb/filter_policy.h | 22 ++----- options/options_test.cc | 9 --- table/block_based/filter_policy.cc | 75 ++++------------------ table/block_based/filter_policy_internal.h | 49 ++++++-------- tools/db_crashtest.py | 2 +- util/bloom_test.cc | 45 ------------- 10 files changed, 48 insertions(+), 170 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index aa326b532..dd48bfe8d 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -31,7 +31,6 @@ * Added an optional output parameter to BackupEngine::CreateNewBackup(WithMetadata) to return the BackupID of the new backup. * Added BackupEngine::GetBackupInfo / GetLatestBackupInfo for querying individual backups. * Made the Ribbon filter a long-term supported feature in terms of the SST schema(compatible with version >= 6.15.0) though the API for enabling it is expected to change. -* Added hybrid configuration of Ribbon filter and Bloom filter where some LSM levels use Ribbon for memory space efficiency and some use Bloom for speed. See NewExperimentalRibbonFilterPolicy. This also changes the default behavior of NewExperimentalRibbonFilterPolicy to use Bloom on level 0 and Ribbon on later levels. ## 6.19.0 (03/21/2021) ### Bug Fixes diff --git a/db_stress_tool/db_stress_common.h b/db_stress_tool/db_stress_common.h index 545a78a82..b6869964c 100644 --- a/db_stress_tool/db_stress_common.h +++ b/db_stress_tool/db_stress_common.h @@ -144,7 +144,7 @@ DECLARE_bool(enable_write_thread_adaptive_yield); DECLARE_int32(reopen); DECLARE_double(bloom_bits); DECLARE_bool(use_block_based_filter); -DECLARE_int32(ribbon_starting_level); +DECLARE_bool(use_ribbon_filter); DECLARE_bool(partition_filters); DECLARE_bool(optimize_filters_for_memory); DECLARE_int32(index_type); diff --git a/db_stress_tool/db_stress_gflags.cc b/db_stress_tool/db_stress_gflags.cc index 5183fa40f..873dca59c 100644 --- a/db_stress_tool/db_stress_gflags.cc +++ b/db_stress_tool/db_stress_gflags.cc @@ -410,8 +410,8 @@ DEFINE_bool(use_block_based_filter, false, "use block based filter" "instead of full filter for block based table"); -DEFINE_int32(ribbon_starting_level, false, - "First level to use Ribbon filter instead of Bloom"); +DEFINE_bool(use_ribbon_filter, false, + "Use Ribbon filter instead of Bloom filter"); DEFINE_bool(partition_filters, false, "use partitioned filters " diff --git a/db_stress_tool/db_stress_test_base.cc b/db_stress_tool/db_stress_test_base.cc index 257cb9a0a..1df4aa4de 100644 --- a/db_stress_tool/db_stress_test_base.cc +++ b/db_stress_tool/db_stress_test_base.cc @@ -26,12 +26,11 @@ StressTest::StressTest() compressed_cache_(NewLRUCache(FLAGS_compressed_cache_size)), filter_policy_( FLAGS_bloom_bits >= 0 - ? FLAGS_ribbon_starting_level < FLAGS_num_levels - ? NewExperimentalRibbonFilterPolicy( - FLAGS_bloom_bits, FLAGS_ribbon_starting_level) + ? FLAGS_use_ribbon_filter + ? NewExperimentalRibbonFilterPolicy(FLAGS_bloom_bits) : FLAGS_use_block_based_filter - ? NewBloomFilterPolicy(FLAGS_bloom_bits, true) - : NewBloomFilterPolicy(FLAGS_bloom_bits, false) + ? NewBloomFilterPolicy(FLAGS_bloom_bits, true) + : NewBloomFilterPolicy(FLAGS_bloom_bits, false) : nullptr), db_(nullptr), #ifndef ROCKSDB_LITE diff --git a/include/rocksdb/filter_policy.h b/include/rocksdb/filter_policy.h index faad9264d..c772eb2db 100644 --- a/include/rocksdb/filter_policy.h +++ b/include/rocksdb/filter_policy.h @@ -217,7 +217,7 @@ extern const FilterPolicy* NewBloomFilterPolicy( double bits_per_key, bool use_block_based_builder = false); // An new Bloom alternative that saves about 30% space compared to -// Bloom filters, with about 3-4x construction time and similar +// Bloom filters, with about 3-4x construction CPU time and similar // query times. For example, if you pass in 10 for // bloom_equivalent_bits_per_key, you'll get the same 0.95% FP rate // as Bloom filter but only using about 7 bits per key. (This @@ -225,24 +225,16 @@ extern const FilterPolicy* NewBloomFilterPolicy( // and/or transitional, so is expected to be replaced with a new API. // The constructed filters will be given long-term support.) // -// The space savings of Ribbon filters makes sense for lower (higher -// numbered; larger; longer-lived) levels of LSM, whereas the speed of -// Bloom filters make sense for highest levels of LSM. Setting -// ribbon_starting_level allows for this design. For example, -// ribbon_starting_level=1 means that Bloom filters will be used in -// level 0, including flushes, and Ribbon filters elsewhere. -// ribbon_starting_level=0 means (almost) always use Ribbon. -// // Ribbon filters are compatible with RocksDB >= 6.15.0. Earlier // versions reading the data will behave as if no filter was used // (degraded performance until compaction rebuilds filters). // -// Note: even with ribbon_starting_level=0, this policy can generate -// Bloom filters in some cases. For very small filters (well under 1KB), -// Bloom fallback is by design, as the current Ribbon schema is not -// optimized to save vs. Bloom for such small filters. Other cases of -// Bloom fallback should be exceptional and log an appropriate warning. +// Note: this policy can generate Bloom filters in some cases. +// For very small filters (well under 1KB), Bloom fallback is by +// design, as the current Ribbon schema is not optimized to save vs. +// Bloom for such small filters. Other cases of Bloom fallback should +// be exceptional and log an appropriate warning. extern const FilterPolicy* NewExperimentalRibbonFilterPolicy( - double bloom_equivalent_bits_per_key, int ribbon_starting_level = 1); + double bloom_equivalent_bits_per_key); } // namespace ROCKSDB_NAMESPACE diff --git a/options/options_test.cc b/options/options_test.cc index 5323fedc4..bb2f34146 100644 --- a/options/options_test.cc +++ b/options/options_test.cc @@ -940,15 +940,6 @@ TEST_F(OptionsTest, GetBlockBasedTableOptionsFromString) { &new_opt)); ASSERT_TRUE(new_opt.filter_policy != nullptr); bfp = dynamic_cast(new_opt.filter_policy.get()); - // Not a BloomFilterPolicy - EXPECT_FALSE(bfp); - - ASSERT_OK(GetBlockBasedTableOptionsFromString( - config_options, table_opt, "filter_policy=experimental_ribbon:5.678:0;", - &new_opt)); - ASSERT_TRUE(new_opt.filter_policy != nullptr); - bfp = dynamic_cast(new_opt.filter_policy.get()); - // Pure Ribbon configuration is (oddly) BloomFilterPolicy EXPECT_EQ(bfp->GetMillibitsPerKey(), 5678); EXPECT_EQ(bfp->GetMode(), BloomFilterPolicy::kStandard128Ribbon); diff --git a/table/block_based/filter_policy.cc b/table/block_based/filter_policy.cc index 1b6c61307..0f79143d1 100644 --- a/table/block_based/filter_policy.cc +++ b/table/block_based/filter_policy.cc @@ -23,7 +23,6 @@ #include "util/hash.h" #include "util/ribbon_config.h" #include "util/ribbon_impl.h" -#include "util/string_util.h" namespace ROCKSDB_NAMESPACE { @@ -1055,7 +1054,7 @@ BloomFilterPolicy::BloomFilterPolicy(double bits_per_key, Mode mode) BloomFilterPolicy::~BloomFilterPolicy() {} -const char* BuiltinFilterPolicy::Name() const { +const char* BloomFilterPolicy::Name() const { return "rocksdb.BuiltinBloomFilter"; } @@ -1088,8 +1087,8 @@ void BloomFilterPolicy::CreateFilter(const Slice* keys, int n, } } -bool BuiltinFilterPolicy::KeyMayMatch(const Slice& key, - const Slice& bloom_filter) const { +bool BloomFilterPolicy::KeyMayMatch(const Slice& key, + const Slice& bloom_filter) const { const size_t len = bloom_filter.size(); if (len < 2 || len > 0xffffffffU) { return false; @@ -1111,7 +1110,7 @@ bool BuiltinFilterPolicy::KeyMayMatch(const Slice& key, array); } -FilterBitsBuilder* BuiltinFilterPolicy::GetFilterBitsBuilder() const { +FilterBitsBuilder* BloomFilterPolicy::GetFilterBitsBuilder() const { // This code path should no longer be used, for the built-in // BloomFilterPolicy. Internal to RocksDB and outside // BloomFilterPolicy, only get a FilterBitsBuilder with @@ -1185,7 +1184,7 @@ FilterBitsBuilder* BloomFilterPolicy::GetBuilderFromContext( // Read metadata to determine what kind of FilterBitsReader is needed // and return a new one. -FilterBitsReader* BuiltinFilterPolicy::GetFilterBitsReader( +FilterBitsReader* BloomFilterPolicy::GetFilterBitsReader( const Slice& contents) const { uint32_t len_with_meta = static_cast(contents.size()); if (len_with_meta <= kMetadataLen) { @@ -1266,7 +1265,7 @@ FilterBitsReader* BuiltinFilterPolicy::GetFilterBitsReader( log2_cache_line_size); } -FilterBitsReader* BuiltinFilterPolicy::GetRibbonBitsReader( +FilterBitsReader* BloomFilterPolicy::GetRibbonBitsReader( const Slice& contents) const { uint32_t len_with_meta = static_cast(contents.size()); uint32_t len = len_with_meta - kMetadataLen; @@ -1290,7 +1289,7 @@ FilterBitsReader* BuiltinFilterPolicy::GetRibbonBitsReader( } // For newer Bloom filter implementations -FilterBitsReader* BuiltinFilterPolicy::GetBloomBitsReader( +FilterBitsReader* BloomFilterPolicy::GetBloomBitsReader( const Slice& contents) const { uint32_t len_with_meta = static_cast(contents.size()); uint32_t len = len_with_meta - kMetadataLen; @@ -1363,50 +1362,10 @@ const FilterPolicy* NewBloomFilterPolicy(double bits_per_key, return new BloomFilterPolicy(bits_per_key, m); } -// Chooses between two filter policies based on LSM level -class LevelThresholdFilterPolicy : public BuiltinFilterPolicy { - public: - LevelThresholdFilterPolicy(std::unique_ptr&& a, - std::unique_ptr&& b, - int starting_level_for_b) - : policy_a_(std::move(a)), - policy_b_(std::move(b)), - starting_level_for_b_(starting_level_for_b) { - assert(starting_level_for_b_ >= 0); - } - - // Deprecated block-based filter only - void CreateFilter(const Slice* keys, int n, std::string* dst) const override { - policy_a_->CreateFilter(keys, n, dst); - } - - FilterBitsBuilder* GetBuilderWithContext( - const FilterBuildingContext& context) const override { - if (context.level_at_creation >= starting_level_for_b_) { - return policy_b_->GetBuilderWithContext(context); - } else { - return policy_a_->GetBuilderWithContext(context); - } - } - - private: - const std::unique_ptr policy_a_; - const std::unique_ptr policy_b_; - int starting_level_for_b_; -}; - extern const FilterPolicy* NewExperimentalRibbonFilterPolicy( - double bloom_equivalent_bits_per_key, int ribbon_starting_level) { - std::unique_ptr ribbon_only{new BloomFilterPolicy( - bloom_equivalent_bits_per_key, BloomFilterPolicy::kStandard128Ribbon)}; - if (ribbon_starting_level > 0) { - std::unique_ptr bloom_only{new BloomFilterPolicy( - bloom_equivalent_bits_per_key, BloomFilterPolicy::kFastLocalBloom)}; - return new LevelThresholdFilterPolicy( - std::move(bloom_only), std::move(ribbon_only), ribbon_starting_level); - } else { - return ribbon_only.release(); - } + double bloom_equivalent_bits_per_key) { + return new BloomFilterPolicy(bloom_equivalent_bits_per_key, + BloomFilterPolicy::kStandard128Ribbon); } FilterBuildingContext::FilterBuildingContext( @@ -1437,18 +1396,10 @@ Status FilterPolicy::CreateFromString( NewBloomFilterPolicy(bits_per_key, use_block_based_builder)); } } else if (value.compare(0, kExpRibbonName.size(), kExpRibbonName) == 0) { - size_t pos = value.find(':', kExpRibbonName.size()); - int ribbon_starting_level; - if (pos == std::string::npos) { - pos = value.size(); - ribbon_starting_level = 1; - } else { - ribbon_starting_level = ParseInt(trim(value.substr(pos + 1))); - } double bloom_equivalent_bits_per_key = - ParseDouble(trim(value.substr(kExpRibbonName.size(), pos))); - policy->reset(NewExperimentalRibbonFilterPolicy( - bloom_equivalent_bits_per_key, ribbon_starting_level)); + ParseDouble(trim(value.substr(kExpRibbonName.size()))); + policy->reset( + NewExperimentalRibbonFilterPolicy(bloom_equivalent_bits_per_key)); } else { return Status::NotFound("Invalid filter policy name ", value); #else diff --git a/table/block_based/filter_policy_internal.h b/table/block_based/filter_policy_internal.h index 21b7dbac2..1a8acfc9d 100644 --- a/table/block_based/filter_policy_internal.h +++ b/table/block_based/filter_policy_internal.h @@ -38,38 +38,10 @@ class BuiltinFilterBitsBuilder : public FilterBitsBuilder { virtual double EstimatedFpRate(size_t num_entries, size_t bytes) = 0; }; -// Abstract base class for RocksDB built-in filter policies. -// This class is considered internal API and subject to change. -class BuiltinFilterPolicy : public FilterPolicy { - public: - // Shared name because any built-in policy can read filters from - // any other - const char* Name() const override; - - // Deprecated block-based filter only - bool KeyMayMatch(const Slice& key, const Slice& bloom_filter) const override; - - // Old API - FilterBitsBuilder* GetFilterBitsBuilder() const override; - - // Read metadata to determine what kind of FilterBitsReader is needed - // and return a new one. This must successfully process any filter data - // generated by a built-in FilterBitsBuilder, regardless of the impl - // chosen for this BloomFilterPolicy. Not compatible with CreateFilter. - FilterBitsReader* GetFilterBitsReader(const Slice& contents) const override; - - private: - // For newer Bloom filter implementation(s) - FilterBitsReader* GetBloomBitsReader(const Slice& contents) const; - - // For Ribbon filter implementation(s) - FilterBitsReader* GetRibbonBitsReader(const Slice& contents) const; -}; - // RocksDB built-in filter policy for Bloom or Bloom-like filters. // This class is considered internal API and subject to change. // See NewBloomFilterPolicy. -class BloomFilterPolicy : public BuiltinFilterPolicy { +class BloomFilterPolicy : public FilterPolicy { public: // An internal marker for operating modes of BloomFilterPolicy, in terms // of selecting an implementation. This makes it easier for tests to track @@ -116,9 +88,16 @@ class BloomFilterPolicy : public BuiltinFilterPolicy { ~BloomFilterPolicy() override; + const char* Name() const override; + // Deprecated block-based filter only void CreateFilter(const Slice* keys, int n, std::string* dst) const override; + // Deprecated block-based filter only + bool KeyMayMatch(const Slice& key, const Slice& bloom_filter) const override; + + FilterBitsBuilder* GetFilterBitsBuilder() const override; + // To use this function, call GetBuilderFromContext(). // // Neither the context nor any objects therein should be saved beyond @@ -131,6 +110,12 @@ class BloomFilterPolicy : public BuiltinFilterPolicy { // (An internal convenience function to save boilerplate.) static FilterBitsBuilder* GetBuilderFromContext(const FilterBuildingContext&); + // Read metadata to determine what kind of FilterBitsReader is needed + // and return a new one. This must successfully process any filter data + // generated by a built-in FilterBitsBuilder, regardless of the impl + // chosen for this BloomFilterPolicy. Not compatible with CreateFilter. + FilterBitsReader* GetFilterBitsReader(const Slice& contents) const override; + // Essentially for testing only: configured millibits/key int GetMillibitsPerKey() const { return millibits_per_key_; } // Essentially for testing only: legacy whole bits/key @@ -172,6 +157,12 @@ class BloomFilterPolicy : public BuiltinFilterPolicy { // Sum over all generated filters f: // (predicted_fp_rate(f) - predicted_fp_rate(f|o_f_f_m=false)) * 2^32 mutable std::atomic aggregate_rounding_balance_; + + // For newer Bloom filter implementation(s) + FilterBitsReader* GetBloomBitsReader(const Slice& contents) const; + + // For Ribbon filter implementation(s) + FilterBitsReader* GetRibbonBitsReader(const Slice& contents) const; }; } // namespace ROCKSDB_NAMESPACE diff --git a/tools/db_crashtest.py b/tools/db_crashtest.py index b4d1984e0..ae37f9706 100644 --- a/tools/db_crashtest.py +++ b/tools/db_crashtest.py @@ -102,7 +102,7 @@ default_params = { "mock_direct_io": False, "use_full_merge_v1": lambda: random.randint(0, 1), "use_merge": lambda: random.randint(0, 1), - "ribbon_starting_level": lambda: random.randint(0, 10), + "use_ribbon_filter": lambda: random.randint(0, 1), "verify_checksum": 1, "write_buffer_size": 4 * 1024 * 1024, "writepercent": 35, diff --git a/util/bloom_test.cc b/util/bloom_test.cc index 121fbc0d5..660e56611 100644 --- a/util/bloom_test.cc +++ b/util/bloom_test.cc @@ -1195,51 +1195,6 @@ INSTANTIATE_TEST_CASE_P(Full, FullBloomTest, BloomFilterPolicy::kFastLocalBloom, BloomFilterPolicy::kStandard128Ribbon)); -static double GetEffectiveBitsPerKey(FilterBitsBuilder* builder) { - union { - uint64_t key_value; - char key_bytes[8]; - }; - - const unsigned kNumKeys = 1000; - - Slice key_slice{key_bytes, 8}; - for (key_value = 0; key_value < kNumKeys; ++key_value) { - builder->AddKey(key_slice); - } - - std::unique_ptr buf; - auto filter = builder->Finish(&buf); - return filter.size() * /*bits per byte*/ 8 / (1.0 * kNumKeys); -} - -TEST(RibbonTest, RibbonTestLevelThreshold) { - BlockBasedTableOptions opts; - FilterBuildingContext ctx(opts); - // A few settings - for (int ribbon_starting_level : {0, 1, 10}) { - std::unique_ptr policy{ - NewExperimentalRibbonFilterPolicy(8, ribbon_starting_level)}; - - // Claim to be generating filter for this level - ctx.level_at_creation = ribbon_starting_level; - std::unique_ptr builder{ - policy->GetBuilderWithContext(ctx)}; - - // Must be Ribbon (more space efficient than 8 bits per key) - ASSERT_LT(GetEffectiveBitsPerKey(builder.get()), 7.5); - - if (ribbon_starting_level > 0) { - // Claim to be generating filter for this level - ctx.level_at_creation = ribbon_starting_level - 1; - builder.reset(policy->GetBuilderWithContext(ctx)); - - // Must be Bloom (~ 8 bits per key) - ASSERT_GT(GetEffectiveBitsPerKey(builder.get()), 7.5); - } - } -} - } // namespace ROCKSDB_NAMESPACE int main(int argc, char** argv) {