Skip to content

Commit

Permalink
Clean up obsolete code in BlockBasedTable::PrefetchIndexAndFilterBloc…
Browse files Browse the repository at this point in the history
…ks (#13277)

Summary:
As advertised and recommended by original authors comment, we're removing the now-outdated special handling logic for bloom filters perf regression (timing ~release 7.0.X). I decided to keep the `CompatibilityName` as-is since 1) it's publicly exposed API and 2) it's generally useful to have a dedicated name used for identifying whether a filter on disk is readable by the FilterPolicy.

Pull Request resolved: #13277

Test Plan:
'Dead code' / tech debt. As a smoke test, I manually run a similar benchmark to the one in #9736, with ./db_bench built pre and post change.

**Generate DB:**

```hcl
./db_bench -db=/dev/shm/rocksdb.9.11 -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
```

**Before removing the 'if' block:**

```hcl
./db_bench -db=/dev/shm/rocksdb.9.11 -use_existing_db -readonly -bloom_bits=10 -benchmarks=readrandom -num=10000000 -compaction_style=2 -fifo_compaction_max_table_files_size_mb=10000 -fifo_compaction_allow_compaction=0 -duration=10 2>&1 | grep micros/op

readrandom   :      17.216 micros/op 58085 ops/sec 10.002 seconds 580999 operations;    4.1 MB/s (367256 of 580999 found)
```

**After removing the 'if' block:**

```hcl
./db_bench -db=/dev/shm/rocksdb.9.11 -use_existing_db -readonly -bloom_bits=10 -benchmarks=readrandom -num=10000000 -compaction_style=2 -fifo_compaction_max_table_files_size_mb=10000 -fifo_compaction_allow_compaction=0 -duration=10 2>&1 | grep micros/op

readrandom   :      16.776 micros/op 59607 ops/sec 10.015 seconds 596999 operations;    4.2 MB/s (377846 of 596999 found)
```

Reviewed By: jaykorean, pdillinger

Differential Revision: D67908020

Pulled By: mszeszko-meta

fbshipit-source-id: b904b8eaf9d106f0b47e4ff175242795ac1c5e73
  • Loading branch information
mszeszko-meta authored and facebook-github-bot committed Jan 9, 2025
1 parent b5e4162 commit 44b741e
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 63 deletions.
69 changes: 11 additions & 58 deletions table/block_based/block_based_table_reader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1107,71 +1107,24 @@ Status BlockBasedTable::PrefetchIndexAndFilterBlocks(
// Find filter handle and filter type
if (rep_->filter_policy) {
auto name = rep_->filter_policy->CompatibilityName();
bool builtin_compatible =
strcmp(name, BuiltinFilterPolicy::kCompatibilityName()) == 0;

for (const auto& [filter_type, prefix] :
{std::make_pair(Rep::FilterType::kFullFilter, kFullFilterBlockPrefix),
std::make_pair(Rep::FilterType::kPartitionedFilter,
kPartitionedFilterBlockPrefix),
std::make_pair(Rep::FilterType::kNoFilter,
kObsoleteFilterBlockPrefix)}) {
if (builtin_compatible) {
// This code is only here to deal with a hiccup in early 7.0.x where
// there was an unintentional name change in the SST files metadata.
// It should be OK to remove this in the future (late 2022) and just
// have the 'else' code.
// NOTE: the test:: names below are likely not needed but included
// out of caution
static const std::unordered_set<std::string> kBuiltinNameAndAliases = {
BuiltinFilterPolicy::kCompatibilityName(),
test::LegacyBloomFilterPolicy::kClassName(),
test::FastLocalBloomFilterPolicy::kClassName(),
test::Standard128RibbonFilterPolicy::kClassName(),
"rocksdb.internal.DeprecatedBlockBasedBloomFilter",
BloomFilterPolicy::kClassName(),
RibbonFilterPolicy::kClassName(),
};

// For efficiency, do a prefix seek and see if the first match is
// good.
meta_iter->Seek(prefix);
if (meta_iter->status().ok() && meta_iter->Valid()) {
Slice key = meta_iter->key();
if (key.starts_with(prefix)) {
key.remove_prefix(prefix.size());
if (kBuiltinNameAndAliases.find(key.ToString()) !=
kBuiltinNameAndAliases.end()) {
Slice v = meta_iter->value();
Status s = rep_->filter_handle.DecodeFrom(&v);
if (s.ok()) {
rep_->filter_type = filter_type;
if (filter_type == Rep::FilterType::kNoFilter) {
ROCKS_LOG_WARN(rep_->ioptions.logger,
"Detected obsolete filter type in %s. Read "
"performance might suffer until DB is fully "
"re-compacted.",
rep_->file->file_name().c_str());
}
break;
}
}
}
}
} else {
std::string filter_block_key = prefix + name;
if (FindMetaBlock(meta_iter, filter_block_key, &rep_->filter_handle)
.ok()) {
rep_->filter_type = filter_type;
if (filter_type == Rep::FilterType::kNoFilter) {
ROCKS_LOG_WARN(
rep_->ioptions.logger,
"Detected obsolete filter type in %s. Read performance might "
"suffer until DB is fully re-compacted.",
rep_->file->file_name().c_str());
}
break;
std::string filter_block_key = prefix + name;
if (FindMetaBlock(meta_iter, filter_block_key, &rep_->filter_handle)
.ok()) {
rep_->filter_type = filter_type;
if (filter_type == Rep::FilterType::kNoFilter) {
ROCKS_LOG_WARN(
rep_->ioptions.logger,
"Detected obsolete filter type in %s. Read performance might "
"suffer until DB is fully re-compacted.",
rep_->file->file_name().c_str());
}
break;
}
}
}
Expand Down
4 changes: 0 additions & 4 deletions table/block_based/filter_policy.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1410,10 +1410,6 @@ bool BuiltinFilterPolicy::IsInstanceOf(const std::string& name) const {

static const char* kBuiltinFilterMetadataName = "rocksdb.BuiltinBloomFilter";

const char* BuiltinFilterPolicy::kCompatibilityName() {
return kBuiltinFilterMetadataName;
}

const char* BuiltinFilterPolicy::CompatibilityName() const {
return kBuiltinFilterMetadataName;
}
Expand Down
1 change: 0 additions & 1 deletion table/block_based/filter_policy_internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,6 @@ class BuiltinFilterPolicy : public FilterPolicy {
bool IsInstanceOf(const std::string& id) const override;
// All variants of BuiltinFilterPolicy can read each others filters.
const char* CompatibilityName() const override;
static const char* kCompatibilityName();

public: // new
// An internal function for the implementation of
Expand Down

0 comments on commit 44b741e

Please sign in to comment.