From 479eb1aad62b816fdf74c72e47fec59edea3fe0a Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Sat, 12 Feb 2022 07:04:09 -0800 Subject: [PATCH] Hide deprecated, inefficient block-based filter from public API (#9535) Summary: This change removes the ability to configure the deprecated, inefficient block-based filter in the public API. Options that would have enabled it now use "full" (and optionally partitioned) filters. Existing block-based filters can still be read and used, and a "back door" way to build them still exists, for testing and in case of trouble. About the only way this removal would cause an issue for users is if temporary memory for filter construction greatly increases. In HISTORY.md we suggest a few possible mitigations: partitioned filters, smaller SST files, or setting reserve_table_builder_memory=true. Or users who have customized a FilterPolicy using the CreateFilter/KeyMayMatch mechanism removed in https://github.com/facebook/rocksdb/issues/9501 will have to upgrade their code. (It's long past time for people to move to the new builder/reader customization interface.) This change also introduces some internal-use-only configuration strings for testing specific filter implementations while bypassing some compatibility / intelligence logic. This is intended to hint at a path toward making FilterPolicy Customizable, but it also gives us a "back door" way to configure block-based filter. Aside: updated db_bench so that -readonly implies -use_existing_db Pull Request resolved: https://github.com/facebook/rocksdb/pull/9535 Test Plan: Unit tests updated. Specifically, * BlockBasedTableTest.BlockReadCountTest is tweaked to validate the back door configuration interface and ignoring of `use_block_based_builder`. * BlockBasedTableTest.TracingGetTest is migrated from testing block-based filter access pattern to full filter access patter, by re-ordering some things. * Options test (pretty self-explanatory) Performance test - create with `./db_bench -db=/dev/shm/rocksdb1 -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` with and without `-use_block_based_filter`, which creates a DB with 21 SST files in L0. Read with `./db_bench -db=/dev/shm/rocksdb1 -readonly -bloom_bits=10 -cache_index_and_filter_blocks=1 -benchmarks=readrandom -num=10000000 -compaction_style=2 -fifo_compaction_max_table_files_size_mb=10000 -fifo_compaction_allow_compaction=0 -duration=30` Without -use_block_based_filter: readrandom 464 ops/sec, 689280 KB DB With -use_block_based_filter: readrandom 169 ops/sec, 690996 KB DB No consistent difference with fillrandom Reviewed By: jay-zhuang Differential Revision: D34153871 Pulled By: pdillinger fbshipit-source-id: 31f4a933c542f8f09aca47fa64aec67832a69738 --- HISTORY.md | 12 +++--- db/c_test.c | 8 ++-- include/rocksdb/filter_policy.h | 20 ++++++--- options/options_test.cc | 50 ++++++++++++++++++++-- table/block_based/filter_policy.cc | 68 +++++++++++++++--------------- table/table_test.cc | 38 ++++++++++++----- tools/db_bench_tool.cc | 22 ++++++++-- 7 files changed, 151 insertions(+), 67 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index f2a73bec4..5c95c807c 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -28,17 +28,17 @@ * Remove deprecated API AdvancedColumnFamilyOptions::rate_limit_delay_max_milliseconds. * Removed timestamp from WriteOptions. Accordingly, added to DB APIs Put, Delete, SingleDelete, etc. accepting an additional argument 'timestamp'. Added Put, Delete, SingleDelete, etc to WriteBatch accepting an additional argument 'timestamp'. Removed WriteBatch::AssignTimestamps(vector) API. Renamed WriteBatch::AssignTimestamp() to WriteBatch::UpdateTimestamps() with clarified comments. * Significant updates to FilterPolicy-related APIs and configuration: + * Remove public API support for deprecated, inefficient block-based filter (use_block_based_builder=true). + * Old code and configuration strings that would enable it now quietly enable full filters instead, though any built-in FilterPolicy can still read block-based filters. + * Remove deprecated FilterPolicy::CreateFilter() and FilterPolicy::KeyMayMatch() + * Remove `rocksdb_filterpolicy_create()` from C API, as the only C API support for custom filter policies is now obsolete. + * If temporary memory usage in full filter creation is a problem, consider using partitioned filters, smaller SST files, or setting reserve_table_builder_memory=true. * Remove support for "filter_policy=experimental_ribbon" configuration string. Use something like "filter_policy=ribbonfilter:10" instead. * Allow configuration string like "filter_policy=bloomfilter:10" without - bool, to minimize acknowledgement of inefficient block-based filter. + bool, to minimize acknowledgement of obsolete block-based filter. * A `filter_policy` loaded from an OPTIONS file can read existing filters but still does not support writing new filters. - * Inefficient block-based filter is no longer customizable in the public - API, though (for now) can still be enabled. - * Remove deprecated FilterPolicy::CreateFilter() and - FilterPolicy::KeyMayMatch() - * Remove `rocksdb_filterpolicy_create()` from C API * Change meaning of nullptr return from GetBuilderWithContext() from "use block-based filter" to "generate no filter in this case." * Also, when user specifies bits_per_key < 0.5, we now round this down diff --git a/db/c_test.c b/db/c_test.c index 8b224e049..044bbfd1b 100644 --- a/db/c_test.c +++ b/db/c_test.c @@ -1079,10 +1079,10 @@ int main(int argc, char** argv) { if (run == 0) { // Due to half true, half false with fake filter result CheckCondition(hits == keys_to_query / 2); - } else if (run == 1) { - // Essentially a fingerprint of the block-based Bloom schema - CheckCondition(hits == 241); - } else if (run == 2 || run == 4) { + } else if (run == 1 || run == 2 || run == 4) { + // For run == 1, block-based Bloom is no longer available in public + // API; attempting to enable it enables full Bloom instead. + // // Essentially a fingerprint of full Bloom schema, format_version=5 CheckCondition(hits == 188); } else { diff --git a/include/rocksdb/filter_policy.h b/include/rocksdb/filter_policy.h index 796ee0b78..311974482 100644 --- a/include/rocksdb/filter_policy.h +++ b/include/rocksdb/filter_policy.h @@ -208,7 +208,8 @@ class FilterPolicy { }; // Return a new filter policy that uses a bloom filter with approximately -// the specified number of bits per key. +// the specified number of bits per key. See +// https://github.com/facebook/rocksdb/wiki/RocksDB-Bloom-Filter // // bits_per_key: average bits allocated per key in bloom filter. A good // choice is 9.9, which yields a filter with ~ 1% false positive rate. @@ -216,11 +217,18 @@ class FilterPolicy { // integer. Recommend using no more than three decimal digits after the // decimal point, as in 6.667. // -// use_block_based_builder: use deprecated block based filter (true) rather -// than full or partitioned filter (false). +// To avoid configurations that are unlikely to produce good filtering +// value for the CPU overhead, bits_per_key < 0.5 is rounded down to 0.0 +// which means "generate no filter", and 0.5 <= bits_per_key < 1.0 is +// rounded up to 1.0, for a 62% FP rate. // -// Callers must delete the result after any database that is using the -// result has been closed. +// The caller is responsible for eventually deleting the result, though +// this is typically handled automatically with BlockBasedTableOptions: +// table_options.filter_policy.reset(NewBloomFilterPolicy(...)); +// +// As of RocksDB 7.0, the use_block_based_builder parameter is ignored. +// (The old, inefficient block-based filter is no longer accessible in +// the public API.) // // Note: if you are using a custom comparator that ignores some parts // of the keys being compared, you must not use NewBloomFilterPolicy() @@ -230,7 +238,7 @@ class FilterPolicy { // FilterPolicy (like NewBloomFilterPolicy) that does not ignore // trailing spaces in keys. extern const FilterPolicy* NewBloomFilterPolicy( - double bits_per_key, bool use_block_based_builder = false); + double bits_per_key, bool IGNORED_use_block_based_builder = false); // A new Bloom alternative that saves about 30% space compared to // Bloom filters, with similar query times but roughly 3-4x CPU time diff --git a/options/options_test.cc b/options/options_test.cc index f1733beaa..9869a0ae1 100644 --- a/options/options_test.cc +++ b/options/options_test.cc @@ -919,14 +919,16 @@ TEST_F(OptionsTest, GetBlockBasedTableOptionsFromString) { // unrecognized filter policy name s = GetBlockBasedTableOptionsFromString(config_options, table_opt, - "cache_index_and_filter_blocks=1;" "filter_policy=bloomfilterxx:4:true", &new_opt); ASSERT_NOK(s); ASSERT_TRUE(s.IsInvalidArgument()); - ASSERT_EQ(table_opt.cache_index_and_filter_blocks, - new_opt.cache_index_and_filter_blocks); - ASSERT_EQ(table_opt.filter_policy, new_opt.filter_policy); + + // missing bits per key + s = GetBlockBasedTableOptionsFromString( + config_options, table_opt, "filter_policy=bloomfilter", &new_opt); + ASSERT_NOK(s); + ASSERT_TRUE(s.IsInvalidArgument()); // Used to be rejected, now accepted ASSERT_OK(GetBlockBasedTableOptionsFromString( @@ -936,6 +938,46 @@ TEST_F(OptionsTest, GetBlockBasedTableOptionsFromString) { EXPECT_EQ(bfp->GetWholeBitsPerKey(), 4); EXPECT_EQ(bfp->GetMode(), BloomFilterPolicy::kAutoBloom); + // use_block_based_builder=true now ignored in public API (same as false) + ASSERT_OK(GetBlockBasedTableOptionsFromString( + config_options, table_opt, "filter_policy=bloomfilter:4:true", &new_opt)); + bfp = dynamic_cast(new_opt.filter_policy.get()); + EXPECT_EQ(bfp->GetMillibitsPerKey(), 4000); + EXPECT_EQ(bfp->GetWholeBitsPerKey(), 4); + EXPECT_EQ(bfp->GetMode(), BloomFilterPolicy::kAutoBloom); + + // Back door way of enabling deprecated block-based Bloom + ASSERT_OK(GetBlockBasedTableOptionsFromString( + config_options, table_opt, + "filter_policy=rocksdb.internal.DeprecatedBlockBasedBloomFilter:4", + &new_opt)); + bfp = dynamic_cast(new_opt.filter_policy.get()); + EXPECT_EQ(bfp->GetWholeBitsPerKey(), 4); // Only whole bits used + EXPECT_EQ(bfp->GetMode(), BloomFilterPolicy::kDeprecatedBlock); + + // Test configuring using other internal names + ASSERT_OK(GetBlockBasedTableOptionsFromString( + config_options, table_opt, + "filter_policy=rocksdb.internal.LegacyBloomFilter:3", &new_opt)); + bfp = dynamic_cast(new_opt.filter_policy.get()); + EXPECT_EQ(bfp->GetWholeBitsPerKey(), 3); // Only whole bits used + EXPECT_EQ(bfp->GetMode(), BloomFilterPolicy::kLegacyBloom); + + ASSERT_OK(GetBlockBasedTableOptionsFromString( + config_options, table_opt, + "filter_policy=rocksdb.internal.FastLocalBloomFilter:1.234", &new_opt)); + bfp = dynamic_cast(new_opt.filter_policy.get()); + EXPECT_EQ(bfp->GetMillibitsPerKey(), 1234); + EXPECT_EQ(bfp->GetMode(), BloomFilterPolicy::kFastLocalBloom); + + ASSERT_OK(GetBlockBasedTableOptionsFromString( + config_options, table_opt, + "filter_policy=rocksdb.internal.Standard128RibbonFilter:1.234", + &new_opt)); + bfp = dynamic_cast(new_opt.filter_policy.get()); + EXPECT_EQ(bfp->GetMillibitsPerKey(), 1234); + EXPECT_EQ(bfp->GetMode(), BloomFilterPolicy::kStandard128Ribbon); + // Ribbon filter policy (no Bloom hybrid) ASSERT_OK(GetBlockBasedTableOptionsFromString( config_options, table_opt, "filter_policy=ribbonfilter:5.678:-1;", diff --git a/table/block_based/filter_policy.cc b/table/block_based/filter_policy.cc index bd24e3a2d..2e03779af 100644 --- a/table/block_based/filter_policy.cc +++ b/table/block_based/filter_policy.cc @@ -1676,13 +1676,10 @@ BuiltinFilterBitsReader* BuiltinFilterPolicy::GetBloomBitsReader( } const FilterPolicy* NewBloomFilterPolicy(double bits_per_key, - bool use_block_based_builder) { - BloomFilterPolicy::Mode m; - if (use_block_based_builder) { - m = BloomFilterPolicy::kDeprecatedBlock; - } else { - m = BloomFilterPolicy::kAutoBloom; - } + bool /*use_block_based_builder*/) { + // NOTE: use_block_based_builder now ignored so block-based filter is no + // longer accessible in public API. + BloomFilterPolicy::Mode m = BloomFilterPolicy::kAutoBloom; assert(std::find(BloomFilterPolicy::kAllUserModes.begin(), BloomFilterPolicy::kAllUserModes.end(), m) != BloomFilterPolicy::kAllUserModes.end()); @@ -1763,37 +1760,42 @@ Status FilterPolicy::CreateFromString( policy->reset(); } else if (value == "rocksdb.BuiltinBloomFilter") { *policy = std::make_shared(); + } else { #ifndef ROCKSDB_LITE - } else if (value.compare(0, kBloomName.size(), kBloomName) == 0) { - size_t pos = value.find(':', kBloomName.size()); - bool use_block_based_builder; - if (pos == std::string::npos) { - pos = value.size(); - use_block_based_builder = false; - } else { - use_block_based_builder = - ParseBoolean("use_block_based_builder", trim(value.substr(pos + 1))); + const std::vector vals = StringSplit(value, ':'); + if (vals.size() < 2) { + return Status::NotFound("Invalid filter policy name ", value); } - double bits_per_key = ParseDouble( - trim(value.substr(kBloomName.size(), pos - kBloomName.size()))); - policy->reset(NewBloomFilterPolicy(bits_per_key, use_block_based_builder)); - } else if (value.compare(0, kRibbonName.size(), kRibbonName) == 0) { - size_t pos = value.find(':', kRibbonName.size()); - int bloom_before_level; - if (pos == std::string::npos) { - pos = value.size(); - bloom_before_level = 0; + const std::string& name = vals[0]; + double bits_per_key = ParseDouble(trim(vals[1])); + if (name == "bloomfilter") { // TODO: constants for names + // NOTE: ignoring obsolete bool for "use_block_based_builder" + policy->reset(NewBloomFilterPolicy(bits_per_key)); + } else if (name == "ribbonfilter") { + int bloom_before_level; + if (vals.size() < 3) { + bloom_before_level = 0; + } else { + bloom_before_level = ParseInt(trim(vals[2])); + } + policy->reset(NewRibbonFilterPolicy(/*bloom_equivalent*/ bits_per_key, + bloom_before_level)); + } else if (name == "rocksdb.internal.DeprecatedBlockBasedBloomFilter") { + *policy = std::make_shared( + bits_per_key, BloomFilterPolicy::kDeprecatedBlock); + } else if (name == "rocksdb.internal.LegacyBloomFilter") { + *policy = std::make_shared( + bits_per_key, BloomFilterPolicy::kLegacyBloom); + } else if (name == "rocksdb.internal.FastLocalBloomFilter") { + *policy = std::make_shared( + bits_per_key, BloomFilterPolicy::kFastLocalBloom); + } else if (name == "rocksdb.internal.Standard128RibbonFilter") { + *policy = std::make_shared( + bits_per_key, BloomFilterPolicy::kStandard128Ribbon); } else { - bloom_before_level = ParseInt(trim(value.substr(pos + 1))); + return Status::NotFound("Invalid filter policy name ", value); } - double bloom_equivalent_bits_per_key = ParseDouble( - trim(value.substr(kRibbonName.size(), pos - kRibbonName.size()))); - policy->reset(NewRibbonFilterPolicy(bloom_equivalent_bits_per_key, - bloom_before_level)); - } else { - return Status::NotFound("Invalid filter policy name ", value); #else - } else { return Status::NotSupported("Cannot load filter policy in LITE mode ", value); #endif // ROCKSDB_LITE diff --git a/table/table_test.cc b/table/table_test.cc index 1f314ec74..3b8d59500 100644 --- a/table/table_test.cc +++ b/table/table_test.cc @@ -32,6 +32,7 @@ #include "port/stack_trace.h" #include "rocksdb/cache.h" #include "rocksdb/compression_type.h" +#include "rocksdb/convenience.h" #include "rocksdb/db.h" #include "rocksdb/env.h" #include "rocksdb/file_checksum.h" @@ -51,6 +52,7 @@ #include "table/block_based/block_based_table_factory.h" #include "table/block_based/block_based_table_reader.h" #include "table/block_based/block_builder.h" +#include "table/block_based/filter_policy_internal.h" #include "table/block_based/flush_block_policy.h" #include "table/block_fetcher.h" #include "table/format.h" @@ -2982,7 +2984,7 @@ TEST_P(BlockBasedTableTest, TracingGetTest) { options.create_if_missing = true; table_options.block_cache = NewLRUCache(1024 * 1024, 0); table_options.cache_index_and_filter_blocks = true; - table_options.filter_policy.reset(NewBloomFilterPolicy(10, true)); + table_options.filter_policy.reset(NewBloomFilterPolicy(10)); options.table_factory.reset(new BlockBasedTableFactory(table_options)); SetupTracingTest(&c); std::vector keys; @@ -3021,14 +3023,14 @@ TEST_P(BlockBasedTableTest, TracingGetTest) { // Then we should have three records for one index, one filter, and one data // block access. record.get_id = 1; - record.block_type = TraceType::kBlockTraceIndexBlock; + record.block_type = TraceType::kBlockTraceFilterBlock; record.caller = TableReaderCaller::kUserGet; record.get_from_user_specified_snapshot = Boolean::kFalse; record.referenced_key = encoded_key; record.referenced_key_exist_in_block = Boolean::kTrue; record.is_cache_hit = Boolean::kTrue; expected_records.push_back(record); - record.block_type = TraceType::kBlockTraceFilterBlock; + record.block_type = TraceType::kBlockTraceIndexBlock; expected_records.push_back(record); record.is_cache_hit = Boolean::kFalse; record.block_type = TraceType::kBlockTraceDataBlock; @@ -3036,12 +3038,12 @@ TEST_P(BlockBasedTableTest, TracingGetTest) { // The second get should all observe cache hits. record.is_cache_hit = Boolean::kTrue; record.get_id = 2; - record.block_type = TraceType::kBlockTraceIndexBlock; + record.block_type = TraceType::kBlockTraceFilterBlock; record.caller = TableReaderCaller::kUserGet; record.get_from_user_specified_snapshot = Boolean::kFalse; record.referenced_key = encoded_key; expected_records.push_back(record); - record.block_type = TraceType::kBlockTraceFilterBlock; + record.block_type = TraceType::kBlockTraceIndexBlock; expected_records.push_back(record); record.block_type = TraceType::kBlockTraceDataBlock; expected_records.push_back(record); @@ -3487,9 +3489,11 @@ TEST_P(BlockBasedTableTest, InvalidOptions) { } TEST_P(BlockBasedTableTest, BlockReadCountTest) { - // bloom_filter_type = 0 -- block-based filter - // bloom_filter_type = 0 -- full filter - for (int bloom_filter_type = 0; bloom_filter_type < 2; ++bloom_filter_type) { + // bloom_filter_type = 0 -- block-based filter (not available in public API) + // bloom_filter_type = 1 -- full filter using use_block_based_builder=false + // bloom_filter_type = 2 -- full filter using use_block_based_builder=true + // because of API change to hide block-based filter + for (int bloom_filter_type = 0; bloom_filter_type <= 2; ++bloom_filter_type) { for (int index_and_filter_in_cache = 0; index_and_filter_in_cache < 2; ++index_and_filter_in_cache) { Options options; @@ -3498,8 +3502,22 @@ TEST_P(BlockBasedTableTest, BlockReadCountTest) { BlockBasedTableOptions table_options = GetBlockBasedTableOptions(); table_options.block_cache = NewLRUCache(1, 0); table_options.cache_index_and_filter_blocks = index_and_filter_in_cache; - table_options.filter_policy.reset( - NewBloomFilterPolicy(10, bloom_filter_type == 0)); + if (bloom_filter_type == 0) { +#ifndef ROCKSDB_LITE + // Use back-door way of enabling obsolete block-based Bloom + ASSERT_OK(FilterPolicy::CreateFromString( + ConfigOptions(), + "rocksdb.internal.DeprecatedBlockBasedBloomFilter:10", + &table_options.filter_policy)); +#else + // Skip this case in LITE build + continue; +#endif + } else { + // Public API + table_options.filter_policy.reset( + NewBloomFilterPolicy(10, bloom_filter_type == 2)); + } options.table_factory.reset(new BlockBasedTableFactory(table_options)); std::vector keys; stl_wrappers::KVMap kvmap; diff --git a/tools/db_bench_tool.cc b/tools/db_bench_tool.cc index fa606e961..c35940398 100644 --- a/tools/db_bench_tool.cc +++ b/tools/db_bench_tool.cc @@ -4217,12 +4217,22 @@ class Benchmark { table_options->filter_policy = BlockBasedTableOptions().filter_policy; } else if (FLAGS_bloom_bits == 0) { table_options->filter_policy.reset(); + } else if (FLAGS_use_block_based_filter) { + // Use back-door way of enabling obsolete block-based Bloom + Status s = FilterPolicy::CreateFromString( + ConfigOptions(), + "rocksdb.internal.DeprecatedBlockBasedBloomFilter:" + + ROCKSDB_NAMESPACE::ToString(FLAGS_bloom_bits), + &table_options->filter_policy); + if (!s.ok()) { + fprintf(stderr, "failure creating obsolete block-based filter: %s\n", + s.ToString().c_str()); + exit(1); + } } else { table_options->filter_policy.reset( - FLAGS_use_ribbon_filter - ? NewRibbonFilterPolicy(FLAGS_bloom_bits) - : NewBloomFilterPolicy(FLAGS_bloom_bits, - FLAGS_use_block_based_filter)); + FLAGS_use_ribbon_filter ? NewRibbonFilterPolicy(FLAGS_bloom_bits) + : NewBloomFilterPolicy(FLAGS_bloom_bits)); } } if (FLAGS_row_cache_size) { @@ -7929,7 +7939,11 @@ int db_bench_tool(int argc, char** argv) { /*is_full_fs_warm=*/FLAGS_simulate_hdd)); FLAGS_env = composite_env.get(); } + + // Let -readonly imply -use_existing_db + FLAGS_use_existing_db |= FLAGS_readonly; #endif // ROCKSDB_LITE + if (FLAGS_use_existing_keys && !FLAGS_use_existing_db) { fprintf(stderr, "`-use_existing_db` must be true for `-use_existing_keys` to be "