From 9110685e8c7092d99619e62912e452c5848089fd Mon Sep 17 00:00:00 2001 From: Hui Xiao Date: Fri, 7 Jan 2022 11:24:14 -0800 Subject: [PATCH] Release cache reservation of hash entries of the fall-back Ribbon Filter earlier (#9345) Summary: Note: rebase on and merge after https://github.com/facebook/rocksdb/pull/9349, as part of https://github.com/facebook/rocksdb/pull/9342 **Context:** https://github.com/facebook/rocksdb/pull/9073 charged the hash entries' memory in block cache with `CacheReservationHandle`. However, in the edge case where Ribbon Filter falls back to Bloom Filter and swaps its hash entries to the embedded bloom filter object, the handles associated with those entries are not swapped and thus not released as soon as those entries are cleared during Bloom Filter's finish process. Although this is a minor issue since RocksDB internal calls `FilterBitsBuilder->Reset()` right after `FilterBitsBuilder->Finish()` on the main path, which releases all the cache reservation related to both the Ribbon Filter and its embedded Bloom Filter, it still worths this fix to avoid confusion. **Summary:** - Swapped the `CacheReservationHandle` associated with the hash entries on Ribbon Filter's fallback Pull Request resolved: https://github.com/facebook/rocksdb/pull/9345 Test Plan: - Added a unit test to verify the number of cache reservation after clearing hash entries, which failed before the change and now succeeds Reviewed By: pdillinger Differential Revision: D33377225 Pulled By: hx235 fbshipit-source-id: 7487f4c40dfb6ee7928232021f93ef2c5329cffa --- table/block_based/filter_policy.cc | 4 ++++ util/bloom_test.cc | 12 ++++++++++++ 2 files changed, 16 insertions(+) diff --git a/table/block_based/filter_policy.cc b/table/block_based/filter_policy.cc index a2a0728b9..ae5da2d7b 100644 --- a/table/block_based/filter_policy.cc +++ b/table/block_based/filter_policy.cc @@ -97,6 +97,10 @@ class XXPH3FilterBitsBuilder : public BuiltinFilterBitsBuilder { // For delegating between XXPH3FilterBitsBuilders void SwapEntriesWith(XXPH3FilterBitsBuilder* other) { std::swap(hash_entries_, other->hash_entries_); + if (cache_res_mgr_) { + std::swap(hash_entry_cache_res_bucket_handles_, + other->hash_entry_cache_res_bucket_handles_); + } } virtual size_t RoundDownUsableSpace(size_t available_size) = 0; diff --git a/util/bloom_test.cc b/util/bloom_test.cc index 4cb774fb0..8c3466219 100644 --- a/util/bloom_test.cc +++ b/util/bloom_test.cc @@ -644,6 +644,18 @@ TEST(FullBloomFilterConstructionReserveMemTest, } else { EXPECT_EQ(filter.data()[filter.size() - 5], static_cast(-2)); } + + if (reserve_builder_mem) { + const size_t dummy_entry_num = static_cast(std::ceil( + filter.size() * 1.0 / CacheReservationManager::GetDummyEntrySize())); + EXPECT_GE(cache->GetPinnedUsage(), + dummy_entry_num * CacheReservationManager::GetDummyEntrySize()); + EXPECT_LT( + cache->GetPinnedUsage(), + (dummy_entry_num + 1) * CacheReservationManager::GetDummyEntrySize()); + } else { + EXPECT_EQ(cache->GetPinnedUsage(), 0); + } } }