From 8102690a52183fcdb63708045d96532e3b52387a Mon Sep 17 00:00:00 2001 From: gitbw95 <95719937+gitbw95@users.noreply.github.com> Date: Tue, 22 Mar 2022 10:22:18 -0700 Subject: [PATCH] Update Cache::Release param from force_erase to erase_if_last_ref (#9728) Summary: The param name force_erase may be misleading, since the handle is erased only if it has last reference even if the param is set true. Pull Request resolved: https://github.com/facebook/rocksdb/pull/9728 Reviewed By: pdillinger Differential Revision: D35038673 Pulled By: gitbw95 fbshipit-source-id: 0d16d1e8fed17b97eba7fb53207119332f659a5f --- cache/clock_cache.cc | 10 +++++----- cache/lru_cache.cc | 6 +++--- cache/lru_cache.h | 6 +++--- cache/sharded_cache.cc | 9 +++++---- cache/sharded_cache.h | 8 ++++---- db/db_bloom_filter_test.cc | 4 ++-- db/db_test_util.h | 4 ++-- include/rocksdb/cache.h | 14 +++++++------- table/block_based/block_based_table_reader.cc | 2 +- table/block_based/reader_common.cc | 2 +- utilities/simulator_cache/sim_cache.cc | 4 ++-- 11 files changed, 35 insertions(+), 34 deletions(-) diff --git a/cache/clock_cache.cc b/cache/clock_cache.cc index 3c1a1cf5f..76bf37ada 100644 --- a/cache/clock_cache.cc +++ b/cache/clock_cache.cc @@ -286,8 +286,8 @@ class ClockCacheShard final : public CacheShard { return Lookup(key, hash); } bool Release(Cache::Handle* handle, bool /*useful*/, - bool force_erase) override { - return Release(handle, force_erase); + bool erase_if_last_ref) override { + return Release(handle, erase_if_last_ref); } bool IsReady(Cache::Handle* /*handle*/) override { return true; } void Wait(Cache::Handle* /*handle*/) override {} @@ -297,7 +297,7 @@ class ClockCacheShard final : public CacheShard { // // Not necessary to hold mutex_ before being called. bool Ref(Cache::Handle* handle) override; - bool Release(Cache::Handle* handle, bool force_erase = false) override; + bool Release(Cache::Handle* handle, bool erase_if_last_ref = false) override; void Erase(const Slice& key, uint32_t hash) override; bool EraseAndConfirm(const Slice& key, uint32_t hash, CleanupContext* context); @@ -725,11 +725,11 @@ Cache::Handle* ClockCacheShard::Lookup(const Slice& key, uint32_t hash) { return reinterpret_cast(handle); } -bool ClockCacheShard::Release(Cache::Handle* h, bool force_erase) { +bool ClockCacheShard::Release(Cache::Handle* h, bool erase_if_last_ref) { CleanupContext context; CacheHandle* handle = reinterpret_cast(h); bool erased = Unref(handle, true, &context); - if (force_erase && !erased) { + if (erase_if_last_ref && !erased) { erased = EraseAndConfirm(handle->key, handle->hash, &context); } Cleanup(context); diff --git a/cache/lru_cache.cc b/cache/lru_cache.cc index bb9186f1e..afd4e3e12 100644 --- a/cache/lru_cache.cc +++ b/cache/lru_cache.cc @@ -507,7 +507,7 @@ void LRUCacheShard::SetHighPriorityPoolRatio(double high_pri_pool_ratio) { MaintainPoolSize(); } -bool LRUCacheShard::Release(Cache::Handle* handle, bool force_erase) { +bool LRUCacheShard::Release(Cache::Handle* handle, bool erase_if_last_ref) { if (handle == nullptr) { return false; } @@ -518,9 +518,9 @@ bool LRUCacheShard::Release(Cache::Handle* handle, bool force_erase) { last_reference = e->Unref(); if (last_reference && e->InCache()) { // The item is still in cache, and nobody else holds a reference to it - if (usage_ > capacity_ || force_erase) { + if (usage_ > capacity_ || erase_if_last_ref) { // The LRU list must be empty since the cache is full - assert(lru_.next == &lru_ || force_erase); + assert(lru_.next == &lru_ || erase_if_last_ref); // Take this opportunity and remove the item table_.Remove(e->key(), e->hash); e->SetInCache(false); diff --git a/cache/lru_cache.h b/cache/lru_cache.h index 7013c9328..669264cbd 100644 --- a/cache/lru_cache.h +++ b/cache/lru_cache.h @@ -326,14 +326,14 @@ class ALIGN_AS(CACHE_LINE_SIZE) LRUCacheShard final : public CacheShard { nullptr); } virtual bool Release(Cache::Handle* handle, bool /*useful*/, - bool force_erase) override { - return Release(handle, force_erase); + bool erase_if_last_ref) override { + return Release(handle, erase_if_last_ref); } virtual bool IsReady(Cache::Handle* /*handle*/) override; virtual void Wait(Cache::Handle* /*handle*/) override {} virtual bool Ref(Cache::Handle* handle) override; virtual bool Release(Cache::Handle* handle, - bool force_erase = false) override; + bool erase_if_last_ref = false) override; virtual void Erase(const Slice& key, uint32_t hash) override; // Although in some platforms the update of size_t is atomic, to make sure diff --git a/cache/sharded_cache.cc b/cache/sharded_cache.cc index d2876ce36..6a5fbebdc 100644 --- a/cache/sharded_cache.cc +++ b/cache/sharded_cache.cc @@ -104,14 +104,15 @@ bool ShardedCache::Ref(Handle* handle) { return GetShard(Shard(hash))->Ref(handle); } -bool ShardedCache::Release(Handle* handle, bool force_erase) { +bool ShardedCache::Release(Handle* handle, bool erase_if_last_ref) { uint32_t hash = GetHash(handle); - return GetShard(Shard(hash))->Release(handle, force_erase); + return GetShard(Shard(hash))->Release(handle, erase_if_last_ref); } -bool ShardedCache::Release(Handle* handle, bool useful, bool force_erase) { +bool ShardedCache::Release(Handle* handle, bool useful, + bool erase_if_last_ref) { uint32_t hash = GetHash(handle); - return GetShard(Shard(hash))->Release(handle, useful, force_erase); + return GetShard(Shard(hash))->Release(handle, useful, erase_if_last_ref); } void ShardedCache::Erase(const Slice& key) { diff --git a/cache/sharded_cache.h b/cache/sharded_cache.h index 8e6f60d03..6263b24df 100644 --- a/cache/sharded_cache.h +++ b/cache/sharded_cache.h @@ -37,11 +37,11 @@ class CacheShard { Cache::Priority priority, bool wait, Statistics* stats) = 0; virtual bool Release(Cache::Handle* handle, bool useful, - bool force_erase) = 0; + bool erase_if_last_ref) = 0; virtual bool IsReady(Cache::Handle* handle) = 0; virtual void Wait(Cache::Handle* handle) = 0; virtual bool Ref(Cache::Handle* handle) = 0; - virtual bool Release(Cache::Handle* handle, bool force_erase) = 0; + virtual bool Release(Cache::Handle* handle, bool erase_if_last_ref) = 0; virtual void Erase(const Slice& key, uint32_t hash) = 0; virtual void SetCapacity(size_t capacity) = 0; virtual void SetStrictCapacityLimit(bool strict_capacity_limit) = 0; @@ -94,11 +94,11 @@ class ShardedCache : public Cache { const CreateCallback& create_cb, Priority priority, bool wait, Statistics* stats = nullptr) override; virtual bool Release(Handle* handle, bool useful, - bool force_erase = false) override; + bool erase_if_last_ref = false) override; virtual bool IsReady(Handle* handle) override; virtual void Wait(Handle* handle) override; virtual bool Ref(Handle* handle) override; - virtual bool Release(Handle* handle, bool force_erase = false) override; + virtual bool Release(Handle* handle, bool erase_if_last_ref = false) override; virtual void Erase(const Slice& key) override; virtual uint64_t NewId() override; virtual size_t GetCapacity() const override; diff --git a/db/db_bloom_filter_test.cc b/db/db_bloom_filter_test.cc index 9129ca180..eea41bc0c 100644 --- a/db/db_bloom_filter_test.cc +++ b/db/db_bloom_filter_test.cc @@ -919,7 +919,7 @@ class FilterConstructResPeakTrackingCache : public CacheWrapper { } using Cache::Release; - bool Release(Handle* handle, bool force_erase = false) override { + bool Release(Handle* handle, bool erase_if_last_ref = false) override { auto deleter = GetDeleter(handle); if (deleter == kNoopDeleterForFilterConstruction) { if (!last_peak_tracked_) { @@ -929,7 +929,7 @@ class FilterConstructResPeakTrackingCache : public CacheWrapper { } cur_cache_res_ -= GetCharge(handle); } - bool is_successful = target_->Release(handle, force_erase); + bool is_successful = target_->Release(handle, erase_if_last_ref); return is_successful; } diff --git a/db/db_test_util.h b/db/db_test_util.h index eec6983bd..f8a798c91 100644 --- a/db/db_test_util.h +++ b/db/db_test_util.h @@ -894,8 +894,8 @@ class CacheWrapper : public Cache { bool Ref(Handle* handle) override { return target_->Ref(handle); } using Cache::Release; - bool Release(Handle* handle, bool force_erase = false) override { - return target_->Release(handle, force_erase); + bool Release(Handle* handle, bool erase_if_last_ref = false) override { + return target_->Release(handle, erase_if_last_ref); } void* Value(Handle* handle) override { return target_->Value(handle); } diff --git a/include/rocksdb/cache.h b/include/rocksdb/cache.h index 4fa6e44e8..15cd82543 100644 --- a/include/rocksdb/cache.h +++ b/include/rocksdb/cache.h @@ -329,16 +329,15 @@ class Cache { /** * Release a mapping returned by a previous Lookup(). A released entry might * still remain in cache in case it is later looked up by others. If - * force_erase is set then it also erase it from the cache if there is no - * other reference to it. Erasing it should call the deleter function that - * was provided when the - * entry was inserted. + * erase_if_last_ref is set then it also erase it from the cache if there is + * no other reference to it. Erasing it should call the deleter function that + * was provided when the entry was inserted. * * Returns true if the entry was also erased. */ // REQUIRES: handle must not have been released yet. // REQUIRES: handle must have been returned by a method on *this. - virtual bool Release(Handle* handle, bool force_erase = false) = 0; + virtual bool Release(Handle* handle, bool erase_if_last_ref = false) = 0; // Return the value encapsulated in a handle returned by a // successful Lookup(). @@ -517,8 +516,9 @@ class Cache { // parameter specifies whether the data was actually used or not, // which may be used by the cache implementation to decide whether // to consider it as a hit for retention purposes. - virtual bool Release(Handle* handle, bool /*useful*/, bool force_erase) { - return Release(handle, force_erase); + virtual bool Release(Handle* handle, bool /*useful*/, + bool erase_if_last_ref) { + return Release(handle, erase_if_last_ref); } // Determines if the handle returned by Lookup() has a valid value yet. The diff --git a/table/block_based/block_based_table_reader.cc b/table/block_based/block_based_table_reader.cc index b6e512c24..76010bc05 100644 --- a/table/block_based/block_based_table_reader.cc +++ b/table/block_based/block_based_table_reader.cc @@ -119,7 +119,7 @@ Status ReadBlockFromFile( void ReleaseCachedEntry(void* arg, void* h) { Cache* cache = reinterpret_cast(arg); Cache::Handle* handle = reinterpret_cast(h); - cache->Release(handle, false /* force_erase */); + cache->Release(handle, false /* erase_if_last_ref */); } // For hash based index, return false if table_properties->prefix_extractor_name diff --git a/table/block_based/reader_common.cc b/table/block_based/reader_common.cc index cfaa6791c..d0f47c779 100644 --- a/table/block_based/reader_common.cc +++ b/table/block_based/reader_common.cc @@ -19,7 +19,7 @@ namespace ROCKSDB_NAMESPACE { void ForceReleaseCachedEntry(void* arg, void* h) { Cache* cache = reinterpret_cast(arg); Cache::Handle* handle = reinterpret_cast(h); - cache->Release(handle, true /* force_erase */); + cache->Release(handle, true /* erase_if_last_ref */); } // WART: this is specific to block-based table diff --git a/utilities/simulator_cache/sim_cache.cc b/utilities/simulator_cache/sim_cache.cc index 46dbb6bf3..a883b52e7 100644 --- a/utilities/simulator_cache/sim_cache.cc +++ b/utilities/simulator_cache/sim_cache.cc @@ -213,8 +213,8 @@ class SimCacheImpl : public SimCache { bool Ref(Handle* handle) override { return cache_->Ref(handle); } using Cache::Release; - bool Release(Handle* handle, bool force_erase = false) override { - return cache_->Release(handle, force_erase); + bool Release(Handle* handle, bool erase_if_last_ref = false) override { + return cache_->Release(handle, erase_if_last_ref); } void Erase(const Slice& key) override {