Optimize bloom filter cache misses

Summary:
This optimizes the case when (cache_index_and_filter_blocks=1) and bloom filter is not present in the cache.

Previously we did:
1. Read meta block from file
2. Read the filter position from the meta block
3. Read the filter

Now, we pre-load the filter position on Table::Open(), so we can skip steps (1) and (2) on bloom filter cache miss. Instead of 2 IOs, we do only 1.

Test Plan: make check

Reviewers: sdong

Reviewed By: sdong

Subscribers: dhruba, leveldb

Differential Revision: https://reviews.facebook.net/D46047

table/block_based_table_reader.cc

@@ -339,6 +339,7 @@ struct BlockBasedTable::Rep {
         table_options(_table_opt),
         filter_policy(_table_opt.filter_policy.get()),
         internal_comparator(_internal_comparator),
+        filter_type(FilterType::kNoFilter),
         whole_key_filtering(_table_opt.whole_key_filtering),
         prefix_filtering(true) {}
 
@@ -362,6 +363,14 @@ struct BlockBasedTable::Rep {
   unique_ptr<IndexReader> index_reader;
   unique_ptr<FilterBlockReader> filter;
 
+  enum class FilterType {
+    kNoFilter,
+    kFullFilter,
+    kBlockFilter,
+  };
+  FilterType filter_type;
+  BlockHandle filter_handle;
+
   std::shared_ptr<const TableProperties> table_properties;
   BlockBasedTableOptions::IndexType index_type;
   bool hash_index_allow_collision;
@@ -506,6 +515,21 @@ Status BlockBasedTable::Open(const ImmutableCFOptions& ioptions,
     return s;
   }
 
+  // Find filter handle and filter type
+  if (rep->filter_policy) {
+    for (auto prefix : {kFullFilterBlockPrefix, kFilterBlockPrefix}) {
+      std::string filter_block_key = prefix;
+      filter_block_key.append(rep->filter_policy->Name());
+      if (FindMetaBlock(meta_iter.get(), filter_block_key, &rep->filter_handle)
+              .ok()) {
+        rep->filter_type = (prefix == kFullFilterBlockPrefix)
+                               ? Rep::FilterType::kFullFilter
+                               : Rep::FilterType::kBlockFilter;
+        break;
+      }
+    }
+  }
+
   // Read the properties
   bool found_properties_block = true;
   s = SeekToPropertiesBlock(meta_iter.get(), &found_properties_block);
@@ -573,7 +597,7 @@ Status BlockBasedTable::Open(const ImmutableCFOptions& ioptions,
 
         // Set filter block
         if (rep->filter_policy) {
-          rep->filter.reset(ReadFilter(rep, meta_iter.get(), nullptr));
+          rep->filter.reset(ReadFilter(rep, nullptr));
         }
       } else {
         delete index_reader;
@@ -778,18 +802,16 @@ Status BlockBasedTable::PutDataBlockToCache(
   return s;
 }
 
-FilterBlockReader* BlockBasedTable::ReadFilter(
-    Rep* rep, Iterator* meta_index_iter, size_t* filter_size) {
+FilterBlockReader* BlockBasedTable::ReadFilter(Rep* rep, size_t* filter_size) {
   // TODO: We might want to unify with ReadBlockFromFile() if we start
   // requiring checksum verification in Table::Open.
-  for (auto prefix : {kFullFilterBlockPrefix, kFilterBlockPrefix}) {
-    std::string filter_block_key = prefix;
-    filter_block_key.append(rep->filter_policy->Name());
-    BlockHandle handle;
-    if (FindMetaBlock(meta_index_iter, filter_block_key, &handle).ok()) {
+  if (rep->filter_type == Rep::FilterType::kNoFilter) {
+    return nullptr;
+  }
   BlockContents block;
   if (!ReadBlockContents(rep->file.get(), rep->footer, ReadOptions(),
-                             handle, &block, rep->ioptions.env, false).ok()) {
+                         rep->filter_handle, &block, rep->ioptions.env,
+                         false).ok()) {
     // Error reading the block
     return nullptr;
   }
@@ -799,24 +821,24 @@ FilterBlockReader* BlockBasedTable::ReadFilter(
   }
 
   assert(rep->filter_policy);
-      if (kFilterBlockPrefix == prefix) {
+
+  if (rep->filter_type == Rep::FilterType::kBlockFilter) {
     return new BlockBasedFilterBlockReader(
         rep->prefix_filtering ? rep->ioptions.prefix_extractor : nullptr,
         rep->table_options, rep->whole_key_filtering, std::move(block));
-      } else if (kFullFilterBlockPrefix == prefix) {
-        auto filter_bits_reader = rep->filter_policy->
-            GetFilterBitsReader(block.data);
+  } else if (rep->filter_type == Rep::FilterType::kFullFilter) {
+    auto filter_bits_reader =
+        rep->filter_policy->GetFilterBitsReader(block.data);
     if (filter_bits_reader != nullptr) {
       return new FullFilterBlockReader(
           rep->prefix_filtering ? rep->ioptions.prefix_extractor : nullptr,
           rep->whole_key_filtering, std::move(block), filter_bits_reader);
     }
-      } else {
-        assert(false);
-        return nullptr;
-      }
-    }
   }
+
+  // filter_type is either kNoFilter (exited the function at the first if),
+  // kBlockFilter or kFullFilter. there is no way for the execution to come here
+  assert(false);
   return nullptr;
 }
 
@@ -858,20 +880,14 @@ BlockBasedTable::CachableEntry<FilterBlockReader> BlockBasedTable::GetFilter(
     return CachableEntry<FilterBlockReader>();
   } else {
     size_t filter_size = 0;
-    std::unique_ptr<Block> meta;
-    std::unique_ptr<Iterator> iter;
-    auto s = ReadMetaBlock(rep_, &meta, &iter);
-
-    if (s.ok()) {
-      filter = ReadFilter(rep_, iter.get(), &filter_size);
+    filter = ReadFilter(rep_, &filter_size);
     if (filter != nullptr) {
       assert(filter_size > 0);
-        cache_handle = block_cache->Insert(
-            key, filter, filter_size, &DeleteCachedEntry<FilterBlockReader>);
+      cache_handle = block_cache->Insert(key, filter, filter_size,
+                                         &DeleteCachedEntry<FilterBlockReader>);
       RecordTick(statistics, BLOCK_CACHE_ADD);
     }
   }
-  }
 
   return { filter, cache_handle };
 }

table/block_based_table_reader.h

@@ -199,10 +199,7 @@ class BlockBasedTable : public TableReader {
       std::unique_ptr<Iterator>* iter);
 
   // Create the filter from the filter block.
-  static FilterBlockReader* ReadFilter(
-      Rep* rep,
-      Iterator* meta_index_iter,
-      size_t* filter_size = nullptr);
+  static FilterBlockReader* ReadFilter(Rep* rep, size_t* filter_size = nullptr);
 
   static void SetupCacheKeyPrefix(Rep* rep);
 

table/table_test.cc

@@ -26,6 +26,7 @@
 #include "rocksdb/env.h"
 #include "rocksdb/iterator.h"
 #include "rocksdb/memtablerep.h"
+#include "rocksdb/perf_context.h"
 #include "rocksdb/slice_transform.h"
 #include "rocksdb/statistics.h"
 #include "table/block.h"
@@ -1635,6 +1636,79 @@ TEST_F(BlockBasedTableTest, FilterBlockInBlockCache) {
   props.AssertFilterBlockStat(0, 0);
 }
 
+TEST_F(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) {
+    for (int index_and_filter_in_cache = 0; index_and_filter_in_cache < 2;
+         ++index_and_filter_in_cache) {
+      Options options;
+      options.create_if_missing = true;
+
+      BlockBasedTableOptions table_options;
+      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));
+      options.table_factory.reset(new BlockBasedTableFactory(table_options));
+      std::vector<std::string> keys;
+      KVMap kvmap;
+
+      TableConstructor c(BytewiseComparator());
+      std::string user_key = "k04";
+      InternalKey internal_key(user_key, 0, kTypeValue);
+      std::string encoded_key = internal_key.Encode().ToString();
+      c.Add(encoded_key, "hello");
+      ImmutableCFOptions ioptions(options);
+      // Generate table with filter policy
+      c.Finish(options, ioptions, table_options,
+               GetPlainInternalComparator(options.comparator), &keys, &kvmap);
+      auto reader = c.GetTableReader();
+      std::string value;
+      GetContext get_context(options.comparator, nullptr, nullptr, nullptr,
+                             GetContext::kNotFound, user_key, &value, nullptr,
+                             nullptr, nullptr);
+      perf_context.Reset();
+      ASSERT_OK(reader->Get(ReadOptions(), encoded_key, &get_context));
+      if (index_and_filter_in_cache) {
+        // data, index and filter block
+        ASSERT_EQ(perf_context.block_read_count, 3);
+      } else {
+        // just the data block
+        ASSERT_EQ(perf_context.block_read_count, 1);
+      }
+      ASSERT_EQ(get_context.State(), GetContext::kFound);
+      ASSERT_EQ(value, "hello");
+
+      // Get non-existing key
+      user_key = "does-not-exist";
+      internal_key = InternalKey(user_key, 0, kTypeValue);
+      encoded_key = internal_key.Encode().ToString();
+
+      get_context = GetContext(options.comparator, nullptr, nullptr, nullptr,
+                               GetContext::kNotFound, user_key, &value, nullptr,
+                               nullptr, nullptr);
+      perf_context.Reset();
+      ASSERT_OK(reader->Get(ReadOptions(), encoded_key, &get_context));
+      ASSERT_EQ(get_context.State(), GetContext::kNotFound);
+
+      if (index_and_filter_in_cache) {
+        if (bloom_filter_type == 0) {
+          // with block-based, we read index and then the filter
+          ASSERT_EQ(perf_context.block_read_count, 2);
+        } else {
+          // with full-filter, we read filter first and then we stop
+          ASSERT_EQ(perf_context.block_read_count, 1);
+        }
+      } else {
+        // filter is already in memory and it figures out that the key doesn't
+        // exist
+        ASSERT_EQ(perf_context.block_read_count, 0);
+      }
+    }
+  }
+}
+
 TEST_F(BlockBasedTableTest, BlockCacheLeak) {
   // Check that when we reopen a table we don't lose access to blocks already
   // in the cache. This test checks whether the Table actually makes use of the