diff --git a/table/block_based_table_reader.cc b/table/block_based_table_reader.cc index bc9efd68a..08c3f2055 100644 --- a/table/block_based_table_reader.cc +++ b/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 index_reader; unique_ptr filter; + enum class FilterType { + kNoFilter, + kFullFilter, + kBlockFilter, + }; + FilterType filter_type; + BlockHandle filter_handle; + std::shared_ptr 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,45 +802,43 @@ 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()) { - BlockContents block; - if (!ReadBlockContents(rep->file.get(), rep->footer, ReadOptions(), - handle, &block, rep->ioptions.env, false).ok()) { - // Error reading the block - return nullptr; - } + if (rep->filter_type == Rep::FilterType::kNoFilter) { + return nullptr; + } + BlockContents block; + if (!ReadBlockContents(rep->file.get(), rep->footer, ReadOptions(), + rep->filter_handle, &block, rep->ioptions.env, + false).ok()) { + // Error reading the block + return nullptr; + } - if (filter_size) { - *filter_size = block.data.size(); - } + if (filter_size) { + *filter_size = block.data.size(); + } - assert(rep->filter_policy); - if (kFilterBlockPrefix == prefix) { - 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); - 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; - } + assert(rep->filter_policy); + + 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 (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); } } + + // 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,18 +880,12 @@ BlockBasedTable::CachableEntry BlockBasedTable::GetFilter( return CachableEntry(); } else { size_t filter_size = 0; - std::unique_ptr meta; - std::unique_ptr iter; - auto s = ReadMetaBlock(rep_, &meta, &iter); - - if (s.ok()) { - filter = ReadFilter(rep_, iter.get(), &filter_size); - if (filter != nullptr) { - assert(filter_size > 0); - cache_handle = block_cache->Insert( - key, filter, filter_size, &DeleteCachedEntry); - RecordTick(statistics, BLOCK_CACHE_ADD); - } + filter = ReadFilter(rep_, &filter_size); + if (filter != nullptr) { + assert(filter_size > 0); + cache_handle = block_cache->Insert(key, filter, filter_size, + &DeleteCachedEntry); + RecordTick(statistics, BLOCK_CACHE_ADD); } } diff --git a/table/block_based_table_reader.h b/table/block_based_table_reader.h index 6b5a63a23..d81f610b8 100644 --- a/table/block_based_table_reader.h +++ b/table/block_based_table_reader.h @@ -199,10 +199,7 @@ class BlockBasedTable : public TableReader { std::unique_ptr* 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); diff --git a/table/table_test.cc b/table/table_test.cc index 311361d09..4daa5a8a0 100644 --- a/table/table_test.cc +++ b/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 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