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 #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: #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

@@ -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<Slice>) 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

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 {

include/rocksdb/filter_policy.h

@@ -208,19 +208,27 @@ 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.
 // When format_version < 5, the value will be rounded to the nearest
 // 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

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<const BloomFilterPolicy*>(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<const BloomFilterPolicy*>(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<const BloomFilterPolicy*>(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<const BloomFilterPolicy*>(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<const BloomFilterPolicy*>(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;",

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<BuiltinFilterPolicy>();
+  } 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<std::string> 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<BloomFilterPolicy>(
+          bits_per_key, BloomFilterPolicy::kDeprecatedBlock);
+    } else if (name == "rocksdb.internal.LegacyBloomFilter") {
+      *policy = std::make_shared<BloomFilterPolicy>(
+          bits_per_key, BloomFilterPolicy::kLegacyBloom);
+    } else if (name == "rocksdb.internal.FastLocalBloomFilter") {
+      *policy = std::make_shared<BloomFilterPolicy>(
+          bits_per_key, BloomFilterPolicy::kFastLocalBloom);
+    } else if (name == "rocksdb.internal.Standard128RibbonFilter") {
+      *policy = std::make_shared<BloomFilterPolicy>(
+          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

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<std::string> keys;
@@ -3021,27 +3023,27 @@ 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;
   expected_records.push_back(record);
   // 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<std::string> keys;
       stl_wrappers::KVMap kvmap;

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 "