From 6c246cfa9de93bce451f750dafd069f0f3563a78 Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Mon, 25 Nov 2019 14:58:24 -0800 Subject: [PATCH 1/7] Expose and elaborate FilterBuildingContext Summary: This change enables custom implementations of FilterPolicy to wrap a variety of NewBloomFilterPolicy and select among them based on contextual information such as table level and compaction style. * Moves FilterBuildingContext to public API and elaborates it with more useful data. (It would be nice to put more general options-like data, but at the time this object is constructed, we are using internal APIs ImmutableCFOptions and MutableCFOptions and don't have easy access to ColumnFamilyOptions that I can tell.) * Renames BloomFilterPolicy::GetFilterBitsBuilderInternal to GetBuilderWithContext, because it's now public. * Plumbs through the table's "level_at_creation" for filter building context. * Simplified some tests by adding GetBuilder() to MockBlockBasedTableTester. * Adds test as DBBloomFilterTest.ContextCustomFilterPolicy, including sample wrapper class LevelAndStyleCustomFilterPolicy. * Fixes a cross-test bug in DBBloomFilterTest.OptimizeFiltersForHits where it does not reset perf context. Test Plan: make check, valgrind on db_bloom_filter_test --- HISTORY.md | 2 +- db/db_bloom_filter_test.cc | 179 +++++++++++++++++- db/db_test_util.h | 5 + include/rocksdb/filter_policy.h | 54 ++++-- .../block_based/block_based_table_builder.cc | 40 ++-- table/block_based/block_based_table_builder.h | 5 +- .../block_based/block_based_table_factory.cc | 2 +- table/block_based/filter_policy.cc | 21 +- table/block_based/filter_policy_internal.h | 33 +--- table/block_based/full_filter_block_test.cc | 18 +- table/block_based/mock_block_based_table.h | 17 +- util/filter_bench.cc | 2 +- 12 files changed, 288 insertions(+), 90 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 247e95a3794..c99da04895b 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -9,7 +9,7 @@ * Changed the default value of periodic_compaction_seconds to `UINT64_MAX` which allows RocksDB to auto-tune periodic compaction scheduling. When using the default value, periodic compactions are now auto-enabled if a compaction filter is used. A value of `0` will turn off the feature completely. * With FIFO compaction style, options.periodic_compaction_seconds will have the same meaning as options.ttl. Whichever stricter will be used. With the default options.periodic_compaction_seconds value with options.ttl's default of 0, RocksDB will give a default of 30 days. * Added an API GetCreationTimeOfOldestFile(uint64_t* creation_time) to get the file_creation_time of the oldest SST file in the DB. -* An unlikely usage of FilterPolicy is no longer supported. Calling GetFilterBitsBuilder() on the FilterPolicy returned by NewBloomFilterPolicy will now cause an assertion violation in debug builds, because RocksDB has internally migrated to a more elaborate interface that is expected to evolve further. Custom implementations of FilterPolicy should work as before, except those wrapping the return of NewBloomFilterPolicy, which will require a new override of a protected function in FilterPolicy. +* FilterPolicy now exposes additional API to make it possible to choose filter configurations based on context, such as table level and compaction style. See `LevelAndStyleCustomFilterPolicy` in db_bloom_filter_test.cc. While most existing custom implementations of FilterPolicy should continue to work as before, those wrapping the return of NewBloomFilterPolicy will require overriding new function `GetBuilderWithContext()`, because calling `GetFilterBitsBuilder()` on the FilterPolicy returned by NewBloomFilterPolicy is no longer supported. ### New Features * Universal compaction to support options.periodic_compaction_seconds. A full compaction will be triggered if any file is over the threshold. diff --git a/db/db_bloom_filter_test.cc b/db/db_bloom_filter_test.cc index a3a47e7d54e..63e79b9df4d 100644 --- a/db/db_bloom_filter_test.cc +++ b/db/db_bloom_filter_test.cc @@ -648,15 +648,17 @@ TEST_F(DBBloomFilterTest, BloomFilterReverseCompatibility) { } namespace { -// A wrapped bloom over default FilterPolicy -class WrappedBloom : public FilterPolicy { +// A wrapped bloom over block-based FilterPolicy +class TestingWrappedBlockBasedFilterPolicy : public FilterPolicy { public: - explicit WrappedBloom(int bits_per_key) + explicit TestingWrappedBlockBasedFilterPolicy(int bits_per_key) : filter_(NewBloomFilterPolicy(bits_per_key, true)), counter_(0) {} - ~WrappedBloom() override { delete filter_; } + ~TestingWrappedBlockBasedFilterPolicy() override { delete filter_; } - const char* Name() const override { return "WrappedRocksDbFilterPolicy"; } + const char* Name() const override { + return "TestingWrappedBlockBasedFilterPolicy"; + } void CreateFilter(const rocksdb::Slice* keys, int n, std::string* dst) const override { @@ -683,12 +685,13 @@ class WrappedBloom : public FilterPolicy { }; } // namespace -TEST_F(DBBloomFilterTest, BloomFilterWrapper) { +TEST_F(DBBloomFilterTest, WrappedBlockBasedFilterPolicy) { Options options = CurrentOptions(); options.statistics = rocksdb::CreateDBStatistics(); BlockBasedTableOptions table_options; - WrappedBloom* policy = new WrappedBloom(10); + TestingWrappedBlockBasedFilterPolicy* policy = + new TestingWrappedBlockBasedFilterPolicy(10); table_options.filter_policy.reset(policy); options.table_factory.reset(NewBlockBasedTableFactory(table_options)); @@ -718,6 +721,167 @@ TEST_F(DBBloomFilterTest, BloomFilterWrapper) { ASSERT_EQ(2U * maxKey, policy->GetCounter()); } +namespace { +// NOTE: This class is referenced by HISTORY.md as a model for a wrapper +// FilterPolicy selecting among configurations based on context. +class LevelAndStyleCustomFilterPolicy : public FilterPolicy { + public: + explicit LevelAndStyleCustomFilterPolicy(int bpk_fifo, int bpk_l0_other, + int bpk_otherwise) + : policy_fifo_(NewBloomFilterPolicy(bpk_fifo)), + policy_l0_other_(NewBloomFilterPolicy(bpk_l0_other)), + policy_otherwise_(NewBloomFilterPolicy(bpk_otherwise)) {} + + // OK to use built-in policy name because we are deferring to a + // built-in builder. We aren't changing the serialized format. + const char* Name() const override { return policy_fifo_->Name(); } + + FilterBitsBuilder* GetBuilderWithContext( + const FilterBuildingContext& context) const override { + if (context.compaction_style == kCompactionStyleFIFO) { + return policy_fifo_->GetBuilderWithContext(context); + } else if (context.level_at_creation == 0) { + return policy_l0_other_->GetBuilderWithContext(context); + } else { + return policy_otherwise_->GetBuilderWithContext(context); + } + } + + FilterBitsReader* GetFilterBitsReader(const Slice& contents) const { + // OK to defer to any of them; they all can parse built-in filters + // from any settings. + return policy_fifo_->GetFilterBitsReader(contents); + } + + // Defer just in case configuration uses block-based filter + void CreateFilter(const Slice* keys, int n, std::string* dst) const override { + policy_otherwise_->CreateFilter(keys, n, dst); + } + bool KeyMayMatch(const Slice& key, const Slice& filter) const override { + return policy_otherwise_->KeyMayMatch(key, filter); + } + + private: + const std::unique_ptr policy_fifo_; + const std::unique_ptr policy_l0_other_; + const std::unique_ptr policy_otherwise_; +}; + +class TestingContextCustomFilterPolicy + : public LevelAndStyleCustomFilterPolicy { + public: + explicit TestingContextCustomFilterPolicy(int bpk_fifo, int bpk_l0_other, + int bpk_otherwise) + : LevelAndStyleCustomFilterPolicy(bpk_fifo, bpk_l0_other, bpk_otherwise) { + } + + FilterBitsBuilder* GetBuilderWithContext( + const FilterBuildingContext& context) const override { + test_report_ += "cf="; + test_report_ += context.column_family_name; + test_report_ += ",cs="; + test_report_ += + OptionsHelper::compaction_style_to_string[context.compaction_style]; + test_report_ += ",lv="; + test_report_ += std::to_string(context.level_at_creation); + test_report_ += "\n"; + + return LevelAndStyleCustomFilterPolicy::GetBuilderWithContext(context); + } + + std::string DumpTestReport() { + std::string rv; + std::swap(rv, test_report_); + return rv; + } + + private: + mutable std::string test_report_; +}; +} // namespace + +TEST_F(DBBloomFilterTest, ContextCustomFilterPolicy) { + for (bool fifo : {true, false}) { + Options options = CurrentOptions(); + options.statistics = rocksdb::CreateDBStatistics(); + options.compaction_style = + fifo ? kCompactionStyleFIFO : kCompactionStyleLevel; + + BlockBasedTableOptions table_options; + std::shared_ptr policy( + new TestingContextCustomFilterPolicy(15, 8, 5)); + table_options.filter_policy = policy; + table_options.format_version = 5; + options.table_factory.reset(NewBlockBasedTableFactory(table_options)); + + CreateAndReopenWithCF({fifo ? "abe" : "bob"}, options); + + const int maxKey = 10000; + for (int i = 0; i < maxKey / 2; i++) { + ASSERT_OK(Put(1, Key(i), Key(i))); + } + // Add a large key to make the file contain wide range + ASSERT_OK(Put(1, Key(maxKey + 55555), Key(maxKey + 55555))); + Flush(1); + EXPECT_EQ(policy->DumpTestReport(), + fifo ? "cf=abe,cs=kCompactionStyleFIFO,lv=0\n" + : "cf=bob,cs=kCompactionStyleLevel,lv=0\n"); + + for (int i = maxKey / 2; i < maxKey; i++) { + ASSERT_OK(Put(1, Key(i), Key(i))); + } + Flush(1); + EXPECT_EQ(policy->DumpTestReport(), + fifo ? "cf=abe,cs=kCompactionStyleFIFO,lv=0\n" + : "cf=bob,cs=kCompactionStyleLevel,lv=0\n"); + + // Check that they can be found + for (int i = 0; i < maxKey; i++) { + ASSERT_EQ(Key(i), Get(1, Key(i))); + } + // Since we have two tables / two filters, we might have Bloom checks on + // our queries, but no more than one "useful" per query on a found key. + EXPECT_LE(TestGetAndResetTickerCount(options, BLOOM_FILTER_USEFUL), maxKey); + + // Check that we have two filters, each about + // fifo: 0.12% FP rate (15 bits per key) + // level: 2.3% FP rate (8 bits per key) + for (int i = 0; i < maxKey; i++) { + ASSERT_EQ("NOT_FOUND", Get(1, Key(i + 33333))); + } + { + auto useful_count = + TestGetAndResetTickerCount(options, BLOOM_FILTER_USEFUL); + EXPECT_GE(useful_count, maxKey * 2 * (fifo ? 0.998 : 0.975)); + EXPECT_LE(useful_count, maxKey * 2 * (fifo ? 0.999 : 0.98)); + } + + if (!fifo) { // FIFO only has L0 + // Full compaction + ASSERT_OK(db_->CompactRange(CompactRangeOptions(), handles_[1], nullptr, + nullptr)); + EXPECT_EQ(policy->DumpTestReport(), + "cf=bob,cs=kCompactionStyleLevel,lv=1\n"); + + // Check that we now have one filter, about 9.2% FP rate (5 bits per key) + for (int i = 0; i < maxKey; i++) { + ASSERT_EQ("NOT_FOUND", Get(1, Key(i + 33333))); + } + { + auto useful_count = + TestGetAndResetTickerCount(options, BLOOM_FILTER_USEFUL); + EXPECT_GE(useful_count, maxKey * 0.90); + EXPECT_LE(useful_count, maxKey * 0.91); + } + } + + // Destroy + ASSERT_OK(dbfull()->DropColumnFamily(handles_[1])); + dbfull()->DestroyColumnFamilyHandle(handles_[1]); + handles_[1] = nullptr; + } +} + class SliceTransformLimitedDomain : public SliceTransform { const char* Name() const override { return "SliceTransformLimitedDomain"; } @@ -1159,6 +1323,7 @@ TEST_F(DBBloomFilterTest, OptimizeFiltersForHits) { options.table_factory.reset(NewBlockBasedTableFactory(bbto)); options.optimize_filters_for_hits = true; options.statistics = rocksdb::CreateDBStatistics(); + get_perf_context()->Reset(); get_perf_context()->EnablePerLevelPerfContext(); CreateAndReopenWithCF({"mypikachu"}, options); diff --git a/db/db_test_util.h b/db/db_test_util.h index f8a01ad156e..c9678ee1c3d 100644 --- a/db/db_test_util.h +++ b/db/db_test_util.h @@ -988,6 +988,11 @@ class DBTestBase : public testing::Test { uint64_t TestGetTickerCount(const Options& options, Tickers ticker_type) { return options.statistics->getTickerCount(ticker_type); } + + uint64_t TestGetAndResetTickerCount(const Options& options, + Tickers ticker_type) { + return options.statistics->getAndResetTickerCount(ticker_type); + } }; } // namespace rocksdb diff --git a/include/rocksdb/filter_policy.h b/include/rocksdb/filter_policy.h index 2e72d83f671..a24010f27ec 100644 --- a/include/rocksdb/filter_policy.h +++ b/include/rocksdb/filter_policy.h @@ -25,9 +25,12 @@ #include #include +#include "rocksdb/advanced_options.h" + namespace rocksdb { class Slice; +class BlockBasedTableOptions; // A class that takes a bunch of keys, then generates filter class FilterBitsBuilder { @@ -80,8 +83,30 @@ class FilterBitsReader { } }; -// Internal type required for FilterPolicy -struct FilterBuildingContext; +// Contextual information passed to BloomFilterPolicy at filter building time. +// Used in overriding FilterPolicy::GetBuilderWithContext(). +struct FilterBuildingContext { + // This constructor is for internal use only and subject to change. + FilterBuildingContext(const BlockBasedTableOptions& table_options); + + // Returns a new FilterBitsBuilder from the filter_policy in + // table_options, or nullptr if not applicable. + // (An internal convenience function to save boilerplate.) + FilterBitsBuilder* GetBuilder() const; + + // Options for the table being built + const BlockBasedTableOptions& table_options; + + // Name of the column family for the table (or empty string if unknown) + std::string column_family_name; + + // The compactions style in effect for the table + CompactionStyle compaction_style = kCompactionStyleLevel; + + // The table level at time of constructing the SST file, or -1 if unknown. + // (The table file could later be used at a different level.) + int level_at_creation = -1; +}; // We add a new format of filter block called full filter block // This new interface gives you more space of customization @@ -125,8 +150,21 @@ class FilterPolicy { // Return a new FilterBitsBuilder for full or partitioned filter blocks, or // nullptr if using block-based filter. + // NOTE: This function is only called by GetBuilderWithContext() below for + // custom FilterPolicy implementations. Thus, it is not necessary to + // override this function if overriding GetBuilderWithContext(). virtual FilterBitsBuilder* GetFilterBitsBuilder() const { return nullptr; } + // A newer variant of GetFilterBitsBuilder that allows a FilterPolicy + // to customize the builder for contextual constraints and hints. + // (Name changed to avoid triggering -Werror=overloaded-virtual.) + // If overriding GetFilterBitsBuilder() suffices, it is not necessary to + // override this function. + virtual FilterBitsBuilder* GetBuilderWithContext( + const FilterBuildingContext&) const { + return GetFilterBitsBuilder(); + } + // Return a new FilterBitsReader for full or partitioned filter blocks, or // nullptr if using block-based filter. // As here, the input slice should NOT be deleted by FilterPolicy. @@ -134,18 +172,6 @@ class FilterPolicy { const Slice& /*contents*/) const { return nullptr; } - - protected: - // An internal-use-only variant of GetFilterBitsBuilder that allows - // a built-in FilterPolicy to customize the builder for contextual - // constraints and hints. (Name changed to avoid triggering - // -Werror=overloaded-virtual.) - virtual FilterBitsBuilder* GetFilterBitsBuilderInternal( - const FilterBuildingContext&) const { - return GetFilterBitsBuilder(); - } - - friend FilterBuildingContext; }; // Return a new filter policy that uses a bloom filter with approximately diff --git a/table/block_based/block_based_table_builder.cc b/table/block_based/block_based_table_builder.cc index 3e40549c488..511bdb5010c 100644 --- a/table/block_based/block_based_table_builder.cc +++ b/table/block_based/block_based_table_builder.cc @@ -62,13 +62,13 @@ namespace { // Create a filter block builder based on its type. FilterBlockBuilder* CreateFilterBlockBuilder( const ImmutableCFOptions& /*opt*/, const MutableCFOptions& mopt, - const BlockBasedTableOptions& table_opt, + const FilterBuildingContext& context, const bool use_delta_encoding_for_index_values, PartitionedIndexBuilder* const p_index_builder) { + const BlockBasedTableOptions& table_opt = context.table_options; if (table_opt.filter_policy == nullptr) return nullptr; - FilterBitsBuilder* filter_bits_builder = - FilterBuildingContext(table_opt).GetBuilder(); + FilterBitsBuilder* filter_bits_builder = context.GetBuilder(); if (filter_bits_builder == nullptr) { return new BlockBasedFilterBlockBuilder(mopt.prefix_extractor.get(), table_opt); @@ -345,6 +345,7 @@ struct BlockBasedTableBuilder::Rep { std::string compressed_output; std::unique_ptr flush_block_policy; + int level_at_creation; uint32_t column_family_id; const std::string& column_family_name; uint64_t creation_time = 0; @@ -363,9 +364,9 @@ struct BlockBasedTableBuilder::Rep { const CompressionType _compression_type, const uint64_t _sample_for_compression, const CompressionOptions& _compression_opts, const bool skip_filters, - const std::string& _column_family_name, const uint64_t _creation_time, - const uint64_t _oldest_key_time, const uint64_t _target_file_size, - const uint64_t _file_creation_time) + const int _level_at_creation, const std::string& _column_family_name, + const uint64_t _creation_time, const uint64_t _oldest_key_time, + const uint64_t _target_file_size, const uint64_t _file_creation_time) : ioptions(_ioptions), moptions(_moptions), table_options(table_opt), @@ -398,6 +399,7 @@ struct BlockBasedTableBuilder::Rep { flush_block_policy( table_options.flush_block_policy_factory->NewFlushBlockPolicy( table_options, data_block)), + level_at_creation(_level_at_creation), column_family_id(_column_family_id), column_family_name(_column_family_name), creation_time(_creation_time), @@ -419,9 +421,13 @@ struct BlockBasedTableBuilder::Rep { if (skip_filters) { filter_builder = nullptr; } else { + FilterBuildingContext context(table_options); + context.column_family_name = column_family_name; + context.compaction_style = ioptions.compaction_style; + context.level_at_creation = level_at_creation; filter_builder.reset(CreateFilterBlockBuilder( - _ioptions, _moptions, table_options, - use_delta_encoding_for_index_values, p_index_builder_)); + ioptions, moptions, context, use_delta_encoding_for_index_values, + p_index_builder_)); } for (auto& collector_factories : *int_tbl_prop_collector_factories) { @@ -454,9 +460,9 @@ BlockBasedTableBuilder::BlockBasedTableBuilder( const CompressionType compression_type, const uint64_t sample_for_compression, const CompressionOptions& compression_opts, const bool skip_filters, - const std::string& column_family_name, const uint64_t creation_time, - const uint64_t oldest_key_time, const uint64_t target_file_size, - const uint64_t file_creation_time) { + const std::string& column_family_name, const int level_at_creation, + const uint64_t creation_time, const uint64_t oldest_key_time, + const uint64_t target_file_size, const uint64_t file_creation_time) { BlockBasedTableOptions sanitized_table_options(table_options); if (sanitized_table_options.format_version == 0 && sanitized_table_options.checksum != kCRC32c) { @@ -469,12 +475,12 @@ BlockBasedTableBuilder::BlockBasedTableBuilder( sanitized_table_options.format_version = 1; } - rep_ = - new Rep(ioptions, moptions, sanitized_table_options, internal_comparator, - int_tbl_prop_collector_factories, column_family_id, file, - compression_type, sample_for_compression, compression_opts, - skip_filters, column_family_name, creation_time, oldest_key_time, - target_file_size, file_creation_time); + rep_ = new Rep(ioptions, moptions, sanitized_table_options, + internal_comparator, int_tbl_prop_collector_factories, + column_family_id, file, compression_type, + sample_for_compression, compression_opts, skip_filters, + level_at_creation, column_family_name, creation_time, + oldest_key_time, target_file_size, file_creation_time); if (rep_->filter_builder != nullptr) { rep_->filter_builder->StartBlock(0); diff --git a/table/block_based/block_based_table_builder.h b/table/block_based/block_based_table_builder.h index bd099ab3dae..5dd5065bb20 100644 --- a/table/block_based/block_based_table_builder.h +++ b/table/block_based/block_based_table_builder.h @@ -47,8 +47,9 @@ class BlockBasedTableBuilder : public TableBuilder { const CompressionType compression_type, const uint64_t sample_for_compression, const CompressionOptions& compression_opts, const bool skip_filters, - const std::string& column_family_name, const uint64_t creation_time = 0, - const uint64_t oldest_key_time = 0, const uint64_t target_file_size = 0, + const std::string& column_family_name, const int level_at_creation, + const uint64_t creation_time = 0, const uint64_t oldest_key_time = 0, + const uint64_t target_file_size = 0, const uint64_t file_creation_time = 0); // No copying allowed diff --git a/table/block_based/block_based_table_factory.cc b/table/block_based/block_based_table_factory.cc index 5f48b78cae0..179ae1f4371 100644 --- a/table/block_based/block_based_table_factory.cc +++ b/table/block_based/block_based_table_factory.cc @@ -218,7 +218,7 @@ TableBuilder* BlockBasedTableFactory::NewTableBuilder( table_builder_options.sample_for_compression, table_builder_options.compression_opts, table_builder_options.skip_filters, - table_builder_options.column_family_name, + table_builder_options.column_family_name, table_builder_options.level, table_builder_options.creation_time, table_builder_options.oldest_key_time, table_builder_options.target_file_size, diff --git a/table/block_based/filter_policy.cc b/table/block_based/filter_policy.cc index dc7c299854d..709c7f28b92 100644 --- a/table/block_based/filter_policy.cc +++ b/table/block_based/filter_policy.cc @@ -479,15 +479,14 @@ 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 FilterBuildingContext::GetBuilder(), - // which will call BloomFilterPolicy::GetFilterBitsBuilderInternal. + // which will call BloomFilterPolicy::GetBuilderWithContext. // RocksDB users have been warned (HISTORY.md) that they can no longer // call this on the built-in BloomFilterPolicy (unlikely). assert(false); - return GetFilterBitsBuilderInternal( - FilterBuildingContext(BlockBasedTableOptions())); + return GetBuilderWithContext(FilterBuildingContext(BlockBasedTableOptions())); } -FilterBitsBuilder* BloomFilterPolicy::GetFilterBitsBuilderInternal( +FilterBitsBuilder* BloomFilterPolicy::GetBuilderWithContext( const FilterBuildingContext& context) const { Mode cur = mode_; // Unusual code construction so that we can have just @@ -495,7 +494,7 @@ FilterBitsBuilder* BloomFilterPolicy::GetFilterBitsBuilderInternal( for (int i = 0; i < 2; ++i) { switch (cur) { case kAuto: - if (context.table_options_.format_version < 5) { + if (context.table_options.format_version < 5) { cur = kLegacyBloom; } else { cur = kFastLocalBloom; @@ -663,6 +662,18 @@ const FilterPolicy* NewBloomFilterPolicy(int bits_per_key, return new BloomFilterPolicy(bits_per_key, m); } +FilterBuildingContext::FilterBuildingContext( + const BlockBasedTableOptions& _table_options) + : table_options(_table_options) {} + +FilterBitsBuilder* FilterBuildingContext::GetBuilder() const { + if (table_options.filter_policy) { + return table_options.filter_policy->GetBuilderWithContext(*this); + } else { + return nullptr; + } +} + FilterPolicy::~FilterPolicy() { } } // namespace rocksdb diff --git a/table/block_based/filter_policy_internal.h b/table/block_based/filter_policy_internal.h index 2129a781eb0..d0a0bdc0ba1 100644 --- a/table/block_based/filter_policy_internal.h +++ b/table/block_based/filter_policy_internal.h @@ -29,24 +29,6 @@ class BuiltinFilterBitsBuilder : public FilterBitsBuilder { virtual uint32_t CalculateSpace(const int num_entry) = 0; }; -// Current information passed to BloomFilterPolicy at filter building -// time. Subject to change. -struct FilterBuildingContext { - explicit FilterBuildingContext(const BlockBasedTableOptions& table_options) - : table_options_(table_options) {} - - // A convenience function to save boilerplate - FilterBitsBuilder* GetBuilder() const { - if (table_options_.filter_policy) { - return table_options_.filter_policy->GetFilterBitsBuilderInternal(*this); - } else { - return nullptr; - } - } - - const BlockBasedTableOptions& table_options_; -}; - // RocksDB built-in filter policy for Bloom or Bloom-like filters. // This class is considered internal API and subject to change. // See NewBloomFilterPolicy. @@ -105,20 +87,19 @@ class BloomFilterPolicy : public FilterPolicy { 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; - - protected: // To use this function, call FilterBuildingContext::GetBuilder(). // // Neither the context nor any objects therein should be saved beyond // the call to this function, unless it's shared_ptr. - FilterBitsBuilder* GetFilterBitsBuilderInternal( + FilterBitsBuilder* GetBuilderWithContext( const FilterBuildingContext&) 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: int bits_per_key_; int num_probes_; diff --git a/table/block_based/full_filter_block_test.cc b/table/block_based/full_filter_block_test.cc index eb4506f89a5..bde19121c95 100644 --- a/table/block_based/full_filter_block_test.cc +++ b/table/block_based/full_filter_block_test.cc @@ -110,8 +110,7 @@ class PluginFullFilterBlockTest : public mock::MockBlockBasedTableTester, }; TEST_F(PluginFullFilterBlockTest, PluginEmptyBuilder) { - FullFilterBlockBuilder builder( - nullptr, true, FilterBuildingContext(table_options_).GetBuilder()); + FullFilterBlockBuilder builder(nullptr, true, GetBuilder()); Slice slice = builder.Finish(); ASSERT_EQ("", EscapeString(slice)); @@ -130,8 +129,7 @@ TEST_F(PluginFullFilterBlockTest, PluginEmptyBuilder) { } TEST_F(PluginFullFilterBlockTest, PluginSingleChunk) { - FullFilterBlockBuilder builder( - nullptr, true, FilterBuildingContext(table_options_).GetBuilder()); + FullFilterBlockBuilder builder(nullptr, true, GetBuilder()); builder.Add("foo"); builder.Add("bar"); builder.Add("box"); @@ -188,8 +186,7 @@ class FullFilterBlockTest : public mock::MockBlockBasedTableTester, }; TEST_F(FullFilterBlockTest, EmptyBuilder) { - FullFilterBlockBuilder builder( - nullptr, true, FilterBuildingContext(table_options_).GetBuilder()); + FullFilterBlockBuilder builder(nullptr, true, GetBuilder()); Slice slice = builder.Finish(); ASSERT_EQ("", EscapeString(slice)); @@ -238,8 +235,7 @@ TEST_F(FullFilterBlockTest, DuplicateEntries) { { // empty prefixes std::unique_ptr prefix_extractor( NewFixedPrefixTransform(0)); - auto bits_builder = new CountUniqueFilterBitsBuilderWrapper( - FilterBuildingContext(table_options_).GetBuilder()); + auto bits_builder = new CountUniqueFilterBitsBuilderWrapper(GetBuilder()); const bool WHOLE_KEY = true; FullFilterBlockBuilder builder(prefix_extractor.get(), WHOLE_KEY, bits_builder); @@ -262,8 +258,7 @@ TEST_F(FullFilterBlockTest, DuplicateEntries) { // mix of empty and non-empty std::unique_ptr prefix_extractor( NewFixedPrefixTransform(7)); - auto bits_builder = new CountUniqueFilterBitsBuilderWrapper( - FilterBuildingContext(table_options_).GetBuilder()); + auto bits_builder = new CountUniqueFilterBitsBuilderWrapper(GetBuilder()); const bool WHOLE_KEY = true; FullFilterBlockBuilder builder(prefix_extractor.get(), WHOLE_KEY, bits_builder); @@ -279,8 +274,7 @@ TEST_F(FullFilterBlockTest, DuplicateEntries) { } TEST_F(FullFilterBlockTest, SingleChunk) { - FullFilterBlockBuilder builder( - nullptr, true, FilterBuildingContext(table_options_).GetBuilder()); + FullFilterBlockBuilder builder(nullptr, true, GetBuilder()); ASSERT_EQ(0, builder.NumAdded()); builder.Add("foo"); builder.Add("bar"); diff --git a/table/block_based/mock_block_based_table.h b/table/block_based/mock_block_based_table.h index 7775e06781a..e81ccc58732 100644 --- a/table/block_based/mock_block_based_table.h +++ b/table/block_based/mock_block_based_table.h @@ -18,6 +18,8 @@ class MockBlockBasedTable : public BlockBasedTable { }; class MockBlockBasedTableTester { + static constexpr int kMockLevel = 0; + public: Options options_; ImmutableCFOptions ioptions_; @@ -33,11 +35,18 @@ class MockBlockBasedTableTester { table_options_.filter_policy.reset(filter_policy); constexpr bool skip_filters = false; - constexpr int level = 0; constexpr bool immortal_table = false; - table_.reset(new MockBlockBasedTable( - new BlockBasedTable::Rep(ioptions_, env_options_, table_options_, - icomp_, skip_filters, level, immortal_table))); + table_.reset(new MockBlockBasedTable(new BlockBasedTable::Rep( + ioptions_, env_options_, table_options_, icomp_, skip_filters, + kMockLevel, immortal_table))); + } + + FilterBitsBuilder* GetBuilder() const { + FilterBuildingContext context(table_options_); + context.column_family_name = "mock_cf"; + context.compaction_style = ioptions_.compaction_style; + context.level_at_creation = kMockLevel; + return context.GetBuilder(); } }; diff --git a/util/filter_bench.cc b/util/filter_bench.cc index 6ff496cb10e..d8bba638093 100644 --- a/util/filter_bench.cc +++ b/util/filter_bench.cc @@ -280,7 +280,7 @@ void FilterBench::Go() { std::unique_ptr builder; if (!FLAGS_use_plain_table_bloom && FLAGS_impl != 1) { - builder.reset(FilterBuildingContext(table_options_).GetBuilder()); + builder.reset(GetBuilder()); } uint32_t variance_mask = 1; From b10a9a8c5a2bc7e04fe4554a2053adb23db9bd49 Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Mon, 25 Nov 2019 15:33:25 -0800 Subject: [PATCH 2/7] struct BlockBasedTableOptions --- include/rocksdb/filter_policy.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/rocksdb/filter_policy.h b/include/rocksdb/filter_policy.h index a24010f27ec..07a795a0ffe 100644 --- a/include/rocksdb/filter_policy.h +++ b/include/rocksdb/filter_policy.h @@ -30,7 +30,7 @@ namespace rocksdb { class Slice; -class BlockBasedTableOptions; +struct BlockBasedTableOptions; // A class that takes a bunch of keys, then generates filter class FilterBitsBuilder { From 64518534c5a1d7d95d7b2f373f9be53fa6f113fd Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Tue, 26 Nov 2019 14:38:47 -0800 Subject: [PATCH 3/7] Move FilterBuildingContext::GetBuilder() To BloomFilterPolicy::GetBuilderFromContext() (internal only) --- include/rocksdb/filter_policy.h | 5 ---- .../block_based/block_based_table_builder.cc | 3 ++- table/block_based/filter_policy.cc | 27 ++++++++++--------- table/block_based/filter_policy_internal.h | 8 +++++- table/block_based/mock_block_based_table.h | 3 ++- .../partitioned_filter_block_test.cc | 2 +- util/bloom_test.cc | 2 +- 7 files changed, 27 insertions(+), 23 deletions(-) diff --git a/include/rocksdb/filter_policy.h b/include/rocksdb/filter_policy.h index 07a795a0ffe..abfd449863a 100644 --- a/include/rocksdb/filter_policy.h +++ b/include/rocksdb/filter_policy.h @@ -89,11 +89,6 @@ struct FilterBuildingContext { // This constructor is for internal use only and subject to change. FilterBuildingContext(const BlockBasedTableOptions& table_options); - // Returns a new FilterBitsBuilder from the filter_policy in - // table_options, or nullptr if not applicable. - // (An internal convenience function to save boilerplate.) - FilterBitsBuilder* GetBuilder() const; - // Options for the table being built const BlockBasedTableOptions& table_options; diff --git a/table/block_based/block_based_table_builder.cc b/table/block_based/block_based_table_builder.cc index 511bdb5010c..58f3ff4339a 100644 --- a/table/block_based/block_based_table_builder.cc +++ b/table/block_based/block_based_table_builder.cc @@ -68,7 +68,8 @@ FilterBlockBuilder* CreateFilterBlockBuilder( const BlockBasedTableOptions& table_opt = context.table_options; if (table_opt.filter_policy == nullptr) return nullptr; - FilterBitsBuilder* filter_bits_builder = context.GetBuilder(); + FilterBitsBuilder* filter_bits_builder = + BloomFilterPolicy::GetBuilderFromContext(context); if (filter_bits_builder == nullptr) { return new BlockBasedFilterBlockBuilder(mopt.prefix_extractor.get(), table_opt); diff --git a/table/block_based/filter_policy.cc b/table/block_based/filter_policy.cc index 709c7f28b92..7ce8e38589e 100644 --- a/table/block_based/filter_policy.cc +++ b/table/block_based/filter_policy.cc @@ -477,11 +477,12 @@ bool BloomFilterPolicy::KeyMayMatch(const Slice& key, 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 FilterBuildingContext::GetBuilder(), - // which will call BloomFilterPolicy::GetBuilderWithContext. - // RocksDB users have been warned (HISTORY.md) that they can no longer - // call this on the built-in BloomFilterPolicy (unlikely). + // BloomFilterPolicy. Internal to RocksDB and outside + // BloomFilterPolicy, only get a FilterBitsBuilder with + // BloomFilterPolicy::GetBuilderFromContext(), which will call + // BloomFilterPolicy::GetBuilderWithContext(). RocksDB users have + // been warned (HISTORY.md) that they can no longer call this on + // the built-in BloomFilterPolicy (unlikely). assert(false); return GetBuilderWithContext(FilterBuildingContext(BlockBasedTableOptions())); } @@ -512,6 +513,14 @@ FilterBitsBuilder* BloomFilterPolicy::GetBuilderWithContext( return nullptr; // something legal } +FilterBitsBuilder* BloomFilterPolicy::GetBuilderFromContext(const FilterBuildingContext& context) { + if (context.table_options.filter_policy) { + return context.table_options.filter_policy->GetBuilderWithContext(context); + } else { + return nullptr; + } +} + // Read metadata to determine what kind of FilterBitsReader is needed // and return a new one. FilterBitsReader* BloomFilterPolicy::GetFilterBitsReader( @@ -666,14 +675,6 @@ FilterBuildingContext::FilterBuildingContext( const BlockBasedTableOptions& _table_options) : table_options(_table_options) {} -FilterBitsBuilder* FilterBuildingContext::GetBuilder() const { - if (table_options.filter_policy) { - return table_options.filter_policy->GetBuilderWithContext(*this); - } else { - return nullptr; - } -} - FilterPolicy::~FilterPolicy() { } } // namespace rocksdb diff --git a/table/block_based/filter_policy_internal.h b/table/block_based/filter_policy_internal.h index d0a0bdc0ba1..9443f6c2b00 100644 --- a/table/block_based/filter_policy_internal.h +++ b/table/block_based/filter_policy_internal.h @@ -87,13 +87,19 @@ class BloomFilterPolicy : public FilterPolicy { FilterBitsBuilder* GetFilterBitsBuilder() const override; - // To use this function, call FilterBuildingContext::GetBuilder(). + // To use this function, call GetBuilderFromContext(). // // Neither the context nor any objects therein should be saved beyond // the call to this function, unless it's shared_ptr. FilterBitsBuilder* GetBuilderWithContext( const FilterBuildingContext&) const override; + // Returns a new FilterBitsBuilder from the filter_policy in + // table_options of a context, or nullptr if not applicable. + // (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 diff --git a/table/block_based/mock_block_based_table.h b/table/block_based/mock_block_based_table.h index e81ccc58732..901207365f1 100644 --- a/table/block_based/mock_block_based_table.h +++ b/table/block_based/mock_block_based_table.h @@ -7,6 +7,7 @@ #include "table/block_based/block_based_filter_block.h" #include "rocksdb/filter_policy.h" #include "table/block_based/block_based_table_reader.h" +#include "table/block_based/filter_policy_internal.h" namespace rocksdb { namespace mock { @@ -46,7 +47,7 @@ class MockBlockBasedTableTester { context.column_family_name = "mock_cf"; context.compaction_style = ioptions_.compaction_style; context.level_at_creation = kMockLevel; - return context.GetBuilder(); + return BloomFilterPolicy::GetBuilderFromContext(context); } }; diff --git a/table/block_based/partitioned_filter_block_test.cc b/table/block_based/partitioned_filter_block_test.cc index b621b58893f..70ba9a17ccc 100644 --- a/table/block_based/partitioned_filter_block_test.cc +++ b/table/block_based/partitioned_filter_block_test.cc @@ -126,7 +126,7 @@ class PartitionedFilterBlockTest const bool kValueDeltaEncoded = true; return new PartitionedFilterBlockBuilder( prefix_extractor, table_options_.whole_key_filtering, - FilterBuildingContext(table_options_).GetBuilder(), + BloomFilterPolicy::GetBuilderFromContext(FilterBuildingContext(table_options_)), table_options_.index_block_restart_interval, !kValueDeltaEncoded, p_index_builder, partition_size); } diff --git a/util/bloom_test.cc b/util/bloom_test.cc index 3ab3e071b1e..a2115cd3cf2 100644 --- a/util/bloom_test.cc +++ b/util/bloom_test.cc @@ -254,7 +254,7 @@ class FullBloomTest : public testing::TestWithParam { } void Reset() { - bits_builder_.reset(FilterBuildingContext(table_options_).GetBuilder()); + bits_builder_.reset(BloomFilterPolicy::GetBuilderFromContext(FilterBuildingContext(table_options_))); bits_reader_.reset(nullptr); buf_.reset(nullptr); filter_size_ = 0; From ca7735201822c3ac323dbceffd37c3138424f450 Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Tue, 26 Nov 2019 14:51:56 -0800 Subject: [PATCH 4/7] make format --- table/block_based/filter_policy.cc | 3 ++- table/block_based/filter_policy_internal.h | 3 +-- table/block_based/mock_block_based_table.h | 2 +- table/block_based/partitioned_filter_block_test.cc | 3 ++- util/bloom_test.cc | 3 ++- 5 files changed, 8 insertions(+), 6 deletions(-) diff --git a/table/block_based/filter_policy.cc b/table/block_based/filter_policy.cc index 7ce8e38589e..f1219c7e213 100644 --- a/table/block_based/filter_policy.cc +++ b/table/block_based/filter_policy.cc @@ -513,7 +513,8 @@ FilterBitsBuilder* BloomFilterPolicy::GetBuilderWithContext( return nullptr; // something legal } -FilterBitsBuilder* BloomFilterPolicy::GetBuilderFromContext(const FilterBuildingContext& context) { +FilterBitsBuilder* BloomFilterPolicy::GetBuilderFromContext( + const FilterBuildingContext& context) { if (context.table_options.filter_policy) { return context.table_options.filter_policy->GetBuilderWithContext(context); } else { diff --git a/table/block_based/filter_policy_internal.h b/table/block_based/filter_policy_internal.h index 9443f6c2b00..9dd863f2054 100644 --- a/table/block_based/filter_policy_internal.h +++ b/table/block_based/filter_policy_internal.h @@ -97,8 +97,7 @@ class BloomFilterPolicy : public FilterPolicy { // Returns a new FilterBitsBuilder from the filter_policy in // table_options of a context, or nullptr if not applicable. // (An internal convenience function to save boilerplate.) - static FilterBitsBuilder* GetBuilderFromContext( - const FilterBuildingContext&); + 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 diff --git a/table/block_based/mock_block_based_table.h b/table/block_based/mock_block_based_table.h index 901207365f1..52891b1bdad 100644 --- a/table/block_based/mock_block_based_table.h +++ b/table/block_based/mock_block_based_table.h @@ -4,8 +4,8 @@ // (found in the LICENSE.Apache file in the root directory). #pragma once -#include "table/block_based/block_based_filter_block.h" #include "rocksdb/filter_policy.h" +#include "table/block_based/block_based_filter_block.h" #include "table/block_based/block_based_table_reader.h" #include "table/block_based/filter_policy_internal.h" diff --git a/table/block_based/partitioned_filter_block_test.cc b/table/block_based/partitioned_filter_block_test.cc index 70ba9a17ccc..315789f55b7 100644 --- a/table/block_based/partitioned_filter_block_test.cc +++ b/table/block_based/partitioned_filter_block_test.cc @@ -126,7 +126,8 @@ class PartitionedFilterBlockTest const bool kValueDeltaEncoded = true; return new PartitionedFilterBlockBuilder( prefix_extractor, table_options_.whole_key_filtering, - BloomFilterPolicy::GetBuilderFromContext(FilterBuildingContext(table_options_)), + BloomFilterPolicy::GetBuilderFromContext( + FilterBuildingContext(table_options_)), table_options_.index_block_restart_interval, !kValueDeltaEncoded, p_index_builder, partition_size); } diff --git a/util/bloom_test.cc b/util/bloom_test.cc index a2115cd3cf2..c7ac0f41263 100644 --- a/util/bloom_test.cc +++ b/util/bloom_test.cc @@ -254,7 +254,8 @@ class FullBloomTest : public testing::TestWithParam { } void Reset() { - bits_builder_.reset(BloomFilterPolicy::GetBuilderFromContext(FilterBuildingContext(table_options_))); + bits_builder_.reset(BloomFilterPolicy::GetBuilderFromContext( + FilterBuildingContext(table_options_))); bits_reader_.reset(nullptr); buf_.reset(nullptr); filter_size_ = 0; From ed3936fe4b6d54f1ed336eea901eef91a25458e0 Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Tue, 26 Nov 2019 15:45:44 -0800 Subject: [PATCH 5/7] Fix lints --- db/db_bloom_filter_test.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/db/db_bloom_filter_test.cc b/db/db_bloom_filter_test.cc index 63e79b9df4d..da676bbafa0 100644 --- a/db/db_bloom_filter_test.cc +++ b/db/db_bloom_filter_test.cc @@ -747,7 +747,7 @@ class LevelAndStyleCustomFilterPolicy : public FilterPolicy { } } - FilterBitsReader* GetFilterBitsReader(const Slice& contents) const { + FilterBitsReader* GetFilterBitsReader(const Slice& contents) const override { // OK to defer to any of them; they all can parse built-in filters // from any settings. return policy_fifo_->GetFilterBitsReader(contents); @@ -808,8 +808,7 @@ TEST_F(DBBloomFilterTest, ContextCustomFilterPolicy) { fifo ? kCompactionStyleFIFO : kCompactionStyleLevel; BlockBasedTableOptions table_options; - std::shared_ptr policy( - new TestingContextCustomFilterPolicy(15, 8, 5)); + auto policy = std::make_shared(15, 8, 5); table_options.filter_policy = policy; table_options.format_version = 5; options.table_factory.reset(NewBlockBasedTableFactory(table_options)); From 93a57370a7f4bb6ee6d0138790373746ef3e4808 Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Tue, 26 Nov 2019 16:02:12 -0800 Subject: [PATCH 6/7] Add explicit (leftover from #6092) --- table/block_based/filter_policy.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/table/block_based/filter_policy.cc b/table/block_based/filter_policy.cc index b8166b141cb..38a6e9f83b2 100644 --- a/table/block_based/filter_policy.cc +++ b/table/block_based/filter_policy.cc @@ -27,7 +27,7 @@ namespace { // See description in FastLocalBloomImpl class FastLocalBloomBitsBuilder : public BuiltinFilterBitsBuilder { public: - FastLocalBloomBitsBuilder(const int millibits_per_key) + explicit FastLocalBloomBitsBuilder(const int millibits_per_key) : millibits_per_key_(millibits_per_key), num_probes_(FastLocalBloomImpl::ChooseNumProbes(millibits_per_key_)) { assert(millibits_per_key >= 1000); From d1bcf386df7c988d90646abf6cb923336737f00b Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Tue, 26 Nov 2019 16:57:47 -0800 Subject: [PATCH 7/7] Update test after merge --- db/db_bloom_filter_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/db/db_bloom_filter_test.cc b/db/db_bloom_filter_test.cc index da676bbafa0..b31c935e85c 100644 --- a/db/db_bloom_filter_test.cc +++ b/db/db_bloom_filter_test.cc @@ -851,8 +851,8 @@ TEST_F(DBBloomFilterTest, ContextCustomFilterPolicy) { { auto useful_count = TestGetAndResetTickerCount(options, BLOOM_FILTER_USEFUL); - EXPECT_GE(useful_count, maxKey * 2 * (fifo ? 0.998 : 0.975)); - EXPECT_LE(useful_count, maxKey * 2 * (fifo ? 0.999 : 0.98)); + EXPECT_GE(useful_count, maxKey * 2 * (fifo ? 0.9980 : 0.975)); + EXPECT_LE(useful_count, maxKey * 2 * (fifo ? 0.9995 : 0.98)); } if (!fifo) { // FIFO only has L0