From e6f86cfb36c66bb912ec278007e4622e4529c567 Mon Sep 17 00:00:00 2001 From: Levi Tamasi Date: Tue, 31 Mar 2020 16:09:11 -0700 Subject: [PATCH] Revert the recent cache deleter change (#6620) Summary: Revert "Use function objects as deleters in the block cache (https://github.com/facebook/rocksdb/issues/6545)" This reverts commit 6301dbe7a71d3663b87f66b3201ff8745bc48742. Revert "Call out the cache deleter related interface change in HISTORY.md (https://github.com/facebook/rocksdb/issues/6606)" This reverts commit 3a35542f8639cc9721db9fd71e964001f7291505. Pull Request resolved: https://github.com/facebook/rocksdb/pull/6620 Test Plan: `make check` Reviewed By: zhichao-cao Differential Revision: D20773311 Pulled By: ltamasi fbshipit-source-id: 7637a761f718f323ef0e7da959462e8fb06e7a2b --- HISTORY.md | 1 - cache/cache_bench.cc | 10 +- cache/cache_test.cc | 97 +++++++++---------- cache/clock_cache.cc | 27 +++--- cache/lru_cache.cc | 3 +- cache/lru_cache.h | 8 +- cache/sharded_cache.cc | 4 +- cache/sharded_cache.h | 11 +-- cache/simple_deleter.h | 47 --------- db/db_basic_test.cc | 3 +- db/db_block_cache_test.cc | 5 +- db/table_cache.cc | 15 ++- include/rocksdb/cache.h | 13 +-- .../block_based/block_based_table_builder.cc | 8 +- table/block_based/block_based_table_reader.cc | 18 ++-- table/block_based/partitioned_index_reader.cc | 2 - utilities/simulator_cache/sim_cache.cc | 16 +-- 17 files changed, 122 insertions(+), 166 deletions(-) delete mode 100644 cache/simple_deleter.h diff --git a/HISTORY.md b/HISTORY.md index 7ba988295..fd9292bb5 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -7,7 +7,6 @@ ### Public API Change * Fix spelling so that API now has correctly spelled transaction state name `COMMITTED`, while the old misspelled `COMMITED` is still available as an alias. * Updated default format_version in BlockBasedTableOptions from 2 to 4. SST files generated with the new default can be read by RocksDB versions 5.16 and newer, and use more efficient encoding of keys in index blocks. -* `Cache::Insert` now expects clients to pass in function objects implementing the `Cache::Deleter` interface as deleters instead of plain function pointers. * A new parameter `CreateBackupOptions` is added to both `BackupEngine::CreateNewBackup` and `BackupEngine::CreateNewBackupWithMetadata`, you can decrease CPU priority of `BackupEngine`'s background threads by setting `decrease_background_thread_cpu_priority` and `background_thread_cpu_priority` in `CreateBackupOptions`. * Updated the public API of SST file checksum. Introduce the FileChecksumGenFactory to create the FileChecksumGenerator for each SST file, such that the FileChecksumGenerator is not shared and it can be more general for checksum implementations. Changed the FileChecksumGenerator interface from Value, Extend, and GetChecksum to Update, Finalize, and GetChecksum. Temproal data should be maintained by the FileChecksumGenerator object itself and finally it can return the checksum string. * Updated the public API of SST file checksum. Introduce the FileChecksumGenFactory to create the FileChecksumGenerator for each SST file, such that the FileChecksumGenerator is not shared and it can be more general for checksum implementations. Changed the FileChecksumGenerator interface from Value, Extend, and GetChecksum to Update, Finalize, and GetChecksum. Finalize should be only called once after all data is processed to generate the final checksum. Temproal data should be maintained by the FileChecksumGenerator object itself and finally it can return the checksum string. diff --git a/cache/cache_bench.cc b/cache/cache_bench.cc index f6271d062..6ff36a32d 100644 --- a/cache/cache_bench.cc +++ b/cache/cache_bench.cc @@ -15,7 +15,6 @@ int main() { #include #include -#include "cache/simple_deleter.h" #include "port/port.h" #include "rocksdb/cache.h" #include "rocksdb/db.h" @@ -50,6 +49,9 @@ namespace ROCKSDB_NAMESPACE { class CacheBench; namespace { +void deleter(const Slice& /*key*/, void* value) { + delete reinterpret_cast(value); +} // State shared by all concurrent executions of the same benchmark. class SharedState { @@ -147,8 +149,7 @@ class CacheBench { // Cast uint64* to be char*, data would be copied to cache Slice key(reinterpret_cast(&rand_key), 8); // do insert - cache_->Insert(key, new char[10], 1, - SimpleDeleter::GetInstance()); + cache_->Insert(key, new char[10], 1, &deleter); } } @@ -226,8 +227,7 @@ class CacheBench { int32_t prob_op = thread->rnd.Uniform(100); if (prob_op >= 0 && prob_op < FLAGS_insert_percent) { // do insert - cache_->Insert(key, new char[10], 1, - SimpleDeleter::GetInstance()); + cache_->Insert(key, new char[10], 1, &deleter); } else if (prob_op -= FLAGS_insert_percent && prob_op < FLAGS_lookup_percent) { // do lookup diff --git a/cache/cache_test.cc b/cache/cache_test.cc index 8d2464276..ceafefe6f 100644 --- a/cache/cache_test.cc +++ b/cache/cache_test.cc @@ -16,7 +16,6 @@ #include #include "cache/clock_cache.h" #include "cache/lru_cache.h" -#include "cache/simple_deleter.h" #include "test_util/testharness.h" #include "util/coding.h" #include "util/string_util.h" @@ -41,17 +40,21 @@ static int DecodeValue(void* v) { const std::string kLRU = "lru"; const std::string kClock = "clock"; +void dumbDeleter(const Slice& /*key*/, void* /*value*/) {} + +void eraseDeleter(const Slice& /*key*/, void* value) { + Cache* cache = reinterpret_cast(value); + cache->Erase("foo"); +} + class CacheTest : public testing::TestWithParam { public: static CacheTest* current_; - class Deleter : public Cache::Deleter { - public: - void operator()(const Slice& key, void* v) override { - current_->deleted_keys_.push_back(DecodeKey(key)); - current_->deleted_values_.push_back(DecodeValue(v)); - } - }; + static void Deleter(const Slice& key, void* v) { + current_->deleted_keys_.push_back(DecodeKey(key)); + current_->deleted_values_.push_back(DecodeValue(v)); + } static const int kCacheSize = 1000; static const int kNumShardBits = 4; @@ -61,7 +64,6 @@ class CacheTest : public testing::TestWithParam { std::vector deleted_keys_; std::vector deleted_values_; - Deleter deleter_; std::shared_ptr cache_; std::shared_ptr cache2_; @@ -115,7 +117,8 @@ class CacheTest : public testing::TestWithParam { void Insert(std::shared_ptr cache, int key, int value, int charge = 1) { - cache->Insert(EncodeKey(key), EncodeValue(value), charge, &deleter_); + cache->Insert(EncodeKey(key), EncodeValue(value), charge, + &CacheTest::Deleter); } void Erase(std::shared_ptr cache, int key) { @@ -164,9 +167,9 @@ TEST_P(CacheTest, UsageTest) { for (int i = 1; i < 100; ++i) { std::string key(i, 'a'); auto kv_size = key.size() + 5; - cache->Insert(key, reinterpret_cast(value), kv_size, nullptr); + cache->Insert(key, reinterpret_cast(value), kv_size, dumbDeleter); precise_cache->Insert(key, reinterpret_cast(value), kv_size, - nullptr); + dumbDeleter); usage += kv_size; ASSERT_EQ(usage, cache->GetUsage()); ASSERT_LT(usage, precise_cache->GetUsage()); @@ -180,9 +183,10 @@ TEST_P(CacheTest, UsageTest) { // make sure the cache will be overloaded for (uint64_t i = 1; i < kCapacity; ++i) { auto key = ToString(i); - cache->Insert(key, reinterpret_cast(value), key.size() + 5, nullptr); + cache->Insert(key, reinterpret_cast(value), key.size() + 5, + dumbDeleter); precise_cache->Insert(key, reinterpret_cast(value), key.size() + 5, - nullptr); + dumbDeleter); } // the usage should be close to the capacity @@ -211,11 +215,11 @@ TEST_P(CacheTest, PinnedUsageTest) { auto kv_size = key.size() + 5; Cache::Handle* handle; Cache::Handle* handle_in_precise_cache; - cache->Insert(key, reinterpret_cast(value), kv_size, nullptr, + cache->Insert(key, reinterpret_cast(value), kv_size, dumbDeleter, &handle); assert(handle); - precise_cache->Insert(key, reinterpret_cast(value), kv_size, nullptr, - &handle_in_precise_cache); + precise_cache->Insert(key, reinterpret_cast(value), kv_size, + dumbDeleter, &handle_in_precise_cache); assert(handle_in_precise_cache); pinned_usage += kv_size; ASSERT_EQ(pinned_usage, cache->GetPinnedUsage()); @@ -250,9 +254,10 @@ TEST_P(CacheTest, PinnedUsageTest) { // check that overloading the cache does not change the pinned usage for (uint64_t i = 1; i < 2 * kCapacity; ++i) { auto key = ToString(i); - cache->Insert(key, reinterpret_cast(value), key.size() + 5, nullptr); + cache->Insert(key, reinterpret_cast(value), key.size() + 5, + dumbDeleter); precise_cache->Insert(key, reinterpret_cast(value), key.size() + 5, - nullptr); + dumbDeleter); } ASSERT_EQ(pinned_usage, cache->GetPinnedUsage()); ASSERT_EQ(precise_cache_pinned_usage, precise_cache->GetPinnedUsage()); @@ -445,25 +450,15 @@ TEST_P(CacheTest, EvictionPolicyRef) { TEST_P(CacheTest, EvictEmptyCache) { // Insert item large than capacity to trigger eviction on empty cache. auto cache = NewCache(1, 0, false); - ASSERT_OK(cache->Insert("foo", nullptr, 10, nullptr)); + ASSERT_OK(cache->Insert("foo", nullptr, 10, dumbDeleter)); } TEST_P(CacheTest, EraseFromDeleter) { // Have deleter which will erase item from cache, which will re-enter // the cache at that point. - class EraseDeleter : public Cache::Deleter { - public: - void operator()(const Slice& /*key*/, void* value) override { - Cache* const cache = static_cast(value); - cache->Erase("foo"); - } - }; - - EraseDeleter erase_deleter; - std::shared_ptr cache = NewCache(10, 0, false); - ASSERT_OK(cache->Insert("foo", nullptr, 1, nullptr)); - ASSERT_OK(cache->Insert("bar", cache.get(), 1, &erase_deleter)); + ASSERT_OK(cache->Insert("foo", nullptr, 1, dumbDeleter)); + ASSERT_OK(cache->Insert("bar", cache.get(), 1, eraseDeleter)); cache->Erase("bar"); ASSERT_EQ(nullptr, cache->Lookup("foo")); ASSERT_EQ(nullptr, cache->Lookup("bar")); @@ -532,11 +527,17 @@ class Value { size_t v_; }; +namespace { +void deleter(const Slice& /*key*/, void* value) { + delete static_cast(value); +} +} // namespace + TEST_P(CacheTest, ReleaseAndErase) { std::shared_ptr cache = NewCache(5, 0, false); Cache::Handle* handle; - Status s = - cache->Insert(EncodeKey(100), EncodeValue(100), 1, &deleter_, &handle); + Status s = cache->Insert(EncodeKey(100), EncodeValue(100), 1, + &CacheTest::Deleter, &handle); ASSERT_TRUE(s.ok()); ASSERT_EQ(5U, cache->GetCapacity()); ASSERT_EQ(1U, cache->GetUsage()); @@ -550,8 +551,8 @@ TEST_P(CacheTest, ReleaseAndErase) { TEST_P(CacheTest, ReleaseWithoutErase) { std::shared_ptr cache = NewCache(5, 0, false); Cache::Handle* handle; - Status s = - cache->Insert(EncodeKey(100), EncodeValue(100), 1, &deleter_, &handle); + Status s = cache->Insert(EncodeKey(100), EncodeValue(100), 1, + &CacheTest::Deleter, &handle); ASSERT_TRUE(s.ok()); ASSERT_EQ(5U, cache->GetCapacity()); ASSERT_EQ(1U, cache->GetUsage()); @@ -573,8 +574,7 @@ TEST_P(CacheTest, SetCapacity) { // Insert 5 entries, but not releasing. for (size_t i = 0; i < 5; i++) { std::string key = ToString(i+1); - Status s = cache->Insert(key, new Value(i + 1), 1, - SimpleDeleter::GetInstance(), &handles[i]); + Status s = cache->Insert(key, new Value(i + 1), 1, &deleter, &handles[i]); ASSERT_TRUE(s.ok()); } ASSERT_EQ(5U, cache->GetCapacity()); @@ -589,8 +589,7 @@ TEST_P(CacheTest, SetCapacity) { // and usage should be 7 for (size_t i = 5; i < 10; i++) { std::string key = ToString(i+1); - Status s = cache->Insert(key, new Value(i + 1), 1, - SimpleDeleter::GetInstance(), &handles[i]); + Status s = cache->Insert(key, new Value(i + 1), 1, &deleter, &handles[i]); ASSERT_TRUE(s.ok()); } ASSERT_EQ(10U, cache->GetCapacity()); @@ -618,8 +617,7 @@ TEST_P(LRUCacheTest, SetStrictCapacityLimit) { Status s; for (size_t i = 0; i < 10; i++) { std::string key = ToString(i + 1); - s = cache->Insert(key, new Value(i + 1), 1, - SimpleDeleter::GetInstance(), &handles[i]); + s = cache->Insert(key, new Value(i + 1), 1, &deleter, &handles[i]); ASSERT_OK(s); ASSERT_NE(nullptr, handles[i]); } @@ -630,8 +628,7 @@ TEST_P(LRUCacheTest, SetStrictCapacityLimit) { Value* extra_value = new Value(0); cache->SetStrictCapacityLimit(true); Cache::Handle* handle; - s = cache->Insert(extra_key, extra_value, 1, - SimpleDeleter::GetInstance(), &handle); + s = cache->Insert(extra_key, extra_value, 1, &deleter, &handle); ASSERT_TRUE(s.IsIncomplete()); ASSERT_EQ(nullptr, handle); ASSERT_EQ(10, cache->GetUsage()); @@ -644,18 +641,15 @@ TEST_P(LRUCacheTest, SetStrictCapacityLimit) { std::shared_ptr cache2 = NewCache(5, 0, true); for (size_t i = 0; i < 5; i++) { std::string key = ToString(i + 1); - s = cache2->Insert(key, new Value(i + 1), 1, - SimpleDeleter::GetInstance(), &handles[i]); + s = cache2->Insert(key, new Value(i + 1), 1, &deleter, &handles[i]); ASSERT_OK(s); ASSERT_NE(nullptr, handles[i]); } - s = cache2->Insert(extra_key, extra_value, 1, - SimpleDeleter::GetInstance(), &handle); + s = cache2->Insert(extra_key, extra_value, 1, &deleter, &handle); ASSERT_TRUE(s.IsIncomplete()); ASSERT_EQ(nullptr, handle); // test insert without handle - s = cache2->Insert(extra_key, extra_value, 1, - SimpleDeleter::GetInstance()); + s = cache2->Insert(extra_key, extra_value, 1, &deleter); // AS if the key have been inserted into cache but get evicted immediately. ASSERT_OK(s); ASSERT_EQ(5, cache2->GetUsage()); @@ -677,8 +671,7 @@ TEST_P(CacheTest, OverCapacity) { // Insert n+1 entries, but not releasing. for (size_t i = 0; i < n + 1; i++) { std::string key = ToString(i+1); - Status s = cache->Insert(key, new Value(i + 1), 1, - SimpleDeleter::GetInstance(), &handles[i]); + Status s = cache->Insert(key, new Value(i + 1), 1, &deleter, &handles[i]); ASSERT_TRUE(s.ok()); } diff --git a/cache/clock_cache.cc b/cache/clock_cache.cc index f699cacec..797a44fd9 100644 --- a/cache/clock_cache.cc +++ b/cache/clock_cache.cc @@ -175,13 +175,11 @@ namespace { // Cache entry meta data. struct CacheHandle { - using Deleter = Cache::Deleter; - Slice key; uint32_t hash; void* value; size_t charge; - Deleter* deleter; + void (*deleter)(const Slice&, void* value); // Flags and counters associated with the cache handle: // lowest bit: n-cache bit @@ -195,7 +193,8 @@ struct CacheHandle { CacheHandle(const CacheHandle& a) { *this = a; } - CacheHandle(const Slice& k, void* v, Deleter* del) + CacheHandle(const Slice& k, void* v, + void (*del)(const Slice& key, void* value)) : key(k), value(v), deleter(del) {} CacheHandle& operator=(const CacheHandle& a) { @@ -270,8 +269,8 @@ class ClockCacheShard final : public CacheShard { void SetCapacity(size_t capacity) override; void SetStrictCapacityLimit(bool strict_capacity_limit) override; Status Insert(const Slice& key, uint32_t hash, void* value, size_t charge, - Deleter* deleter, Cache::Handle** handle, - Cache::Priority priority) override; + void (*deleter)(const Slice& key, void* value), + Cache::Handle** handle, Cache::Priority priority) override; Cache::Handle* Lookup(const Slice& key, uint32_t hash) override; // If the entry in in cache, increase reference count and return true. // Return false otherwise. @@ -340,8 +339,9 @@ class ClockCacheShard final : public CacheShard { bool EvictFromCache(size_t charge, CleanupContext* context); CacheHandle* Insert(const Slice& key, uint32_t hash, void* value, - size_t charge, Deleter* deleter, bool hold_reference, - CleanupContext* context); + size_t change, + void (*deleter)(const Slice& key, void* value), + bool hold_reference, CleanupContext* context); // Guards list_, head_, and recycle_. In addition, updating table_ also has // to hold the mutex, to avoid the cache being in inconsistent state. @@ -561,10 +561,10 @@ void ClockCacheShard::SetStrictCapacityLimit(bool strict_capacity_limit) { std::memory_order_relaxed); } -CacheHandle* ClockCacheShard::Insert(const Slice& key, uint32_t hash, - void* value, size_t charge, - Deleter* deleter, bool hold_reference, - CleanupContext* context) { +CacheHandle* ClockCacheShard::Insert( + const Slice& key, uint32_t hash, void* value, size_t charge, + void (*deleter)(const Slice& key, void* value), bool hold_reference, + CleanupContext* context) { size_t total_charge = CacheHandle::CalcTotalCharge(key, charge, metadata_charge_policy_); MutexLock l(&mutex_); @@ -610,7 +610,8 @@ CacheHandle* ClockCacheShard::Insert(const Slice& key, uint32_t hash, } Status ClockCacheShard::Insert(const Slice& key, uint32_t hash, void* value, - size_t charge, Deleter* deleter, + size_t charge, + void (*deleter)(const Slice& key, void* value), Cache::Handle** out_handle, Cache::Priority /*priority*/) { CleanupContext context; diff --git a/cache/lru_cache.cc b/cache/lru_cache.cc index c41af68d3..987417806 100644 --- a/cache/lru_cache.cc +++ b/cache/lru_cache.cc @@ -337,7 +337,8 @@ bool LRUCacheShard::Release(Cache::Handle* handle, bool force_erase) { } Status LRUCacheShard::Insert(const Slice& key, uint32_t hash, void* value, - size_t charge, Deleter* deleter, + size_t charge, + void (*deleter)(const Slice& key, void* value), Cache::Handle** handle, Cache::Priority priority) { // Allocate the memory here outside of the mutex // If the cache is full, we'll have to release it diff --git a/cache/lru_cache.h b/cache/lru_cache.h index 720c5d040..827e0bece 100644 --- a/cache/lru_cache.h +++ b/cache/lru_cache.h @@ -48,10 +48,8 @@ namespace ROCKSDB_NAMESPACE { // (to move into state 3). struct LRUHandle { - using Deleter = Cache::Deleter; - void* value; - Deleter* deleter; + void (*deleter)(const Slice&, void* value); LRUHandle* next_hash; LRUHandle* next; LRUHandle* prev; @@ -211,7 +209,9 @@ class ALIGN_AS(CACHE_LINE_SIZE) LRUCacheShard final : public CacheShard { // Like Cache methods, but with an extra "hash" parameter. virtual Status Insert(const Slice& key, uint32_t hash, void* value, - size_t charge, Deleter* deleter, Cache::Handle** handle, + size_t charge, + void (*deleter)(const Slice& key, void* value), + Cache::Handle** handle, Cache::Priority priority) override; virtual Cache::Handle* Lookup(const Slice& key, uint32_t hash) override; virtual bool Ref(Cache::Handle* handle) override; diff --git a/cache/sharded_cache.cc b/cache/sharded_cache.cc index 1a7f4b78a..6c915df8c 100644 --- a/cache/sharded_cache.cc +++ b/cache/sharded_cache.cc @@ -44,8 +44,8 @@ void ShardedCache::SetStrictCapacityLimit(bool strict_capacity_limit) { } Status ShardedCache::Insert(const Slice& key, void* value, size_t charge, - Deleter* deleter, Handle** handle, - Priority priority) { + void (*deleter)(const Slice& key, void* value), + Handle** handle, Priority priority) { uint32_t hash = HashSlice(key); return GetShard(Shard(hash)) ->Insert(key, hash, value, charge, deleter, handle, priority); diff --git a/cache/sharded_cache.h b/cache/sharded_cache.h index b00854da4..ce9e459dc 100644 --- a/cache/sharded_cache.h +++ b/cache/sharded_cache.h @@ -21,14 +21,13 @@ namespace ROCKSDB_NAMESPACE { // Single cache shard interface. class CacheShard { public: - using Deleter = Cache::Deleter; - CacheShard() = default; virtual ~CacheShard() = default; virtual Status Insert(const Slice& key, uint32_t hash, void* value, - size_t charge, Deleter* deleter, Cache::Handle** handle, - Cache::Priority priority) = 0; + size_t charge, + void (*deleter)(const Slice& key, void* value), + Cache::Handle** handle, Cache::Priority priority) = 0; virtual Cache::Handle* Lookup(const Slice& key, uint32_t hash) = 0; virtual bool Ref(Cache::Handle* handle) = 0; virtual bool Release(Cache::Handle* handle, bool force_erase = false) = 0; @@ -71,8 +70,8 @@ class ShardedCache : public Cache { virtual void SetStrictCapacityLimit(bool strict_capacity_limit) override; virtual Status Insert(const Slice& key, void* value, size_t charge, - Deleter* deleter, Handle** handle, - Priority priority) override; + void (*deleter)(const Slice& key, void* value), + Handle** handle, Priority priority) override; virtual Handle* Lookup(const Slice& key, Statistics* stats) override; virtual bool Ref(Handle* handle) override; virtual bool Release(Handle* handle, bool force_erase = false) override; diff --git a/cache/simple_deleter.h b/cache/simple_deleter.h deleted file mode 100644 index b2ab11832..000000000 --- a/cache/simple_deleter.h +++ /dev/null @@ -1,47 +0,0 @@ -// Copyright (c) 2011-present, Facebook, Inc. All rights reserved. -// This source code is licensed under both the GPLv2 (found in the -// COPYING file in the root directory) and Apache 2.0 License -// (found in the LICENSE.Apache file in the root directory). - -#pragma once - -#include "rocksdb/cache.h" -#include "rocksdb/rocksdb_namespace.h" - -namespace ROCKSDB_NAMESPACE { - -template -class SimpleDeleter : public Cache::Deleter { - public: - static SimpleDeleter* GetInstance() { - static auto deleter = new SimpleDeleter; - return deleter; - } - - void operator()(const Slice& /* key */, void* value) override { - T* const t = static_cast(value); - delete t; - } - - private: - SimpleDeleter() = default; -}; - -template -class SimpleDeleter : public Cache::Deleter { - public: - static SimpleDeleter* GetInstance() { - static auto deleter = new SimpleDeleter; - return deleter; - } - - void operator()(const Slice& /* key */, void* value) override { - T* const t = static_cast(value); - delete[] t; - } - - private: - SimpleDeleter() = default; -}; - -} // namespace ROCKSDB_NAMESPACE diff --git a/db/db_basic_test.cc b/db/db_basic_test.cc index 9ad290c44..f2cfceae8 100644 --- a/db/db_basic_test.cc +++ b/db/db_basic_test.cc @@ -2053,7 +2053,8 @@ class DBBasicTestWithParallelIO virtual const char* Name() const override { return "MyBlockCache"; } virtual Status Insert(const Slice& key, void* value, size_t charge, - Deleter* deleter, Handle** handle = nullptr, + void (*deleter)(const Slice& key, void* value), + Handle** handle = nullptr, Priority priority = Priority::LOW) override { num_inserts_++; return target_->Insert(key, value, charge, deleter, handle, priority); diff --git a/db/db_block_cache_test.cc b/db/db_block_cache_test.cc index e411c46be..3031e56bb 100644 --- a/db/db_block_cache_test.cc +++ b/db/db_block_cache_test.cc @@ -443,8 +443,9 @@ class MockCache : public LRUCache { false /*strict_capacity_limit*/, 0.0 /*high_pri_pool_ratio*/) { } - Status Insert(const Slice& key, void* value, size_t charge, Deleter* deleter, - Handle** handle, Priority priority) override { + Status Insert(const Slice& key, void* value, size_t charge, + void (*deleter)(const Slice& key, void* value), Handle** handle, + Priority priority) override { if (priority == Priority::LOW) { low_pri_insert_count++; } else { diff --git a/db/table_cache.cc b/db/table_cache.cc index 118f903bc..411959a33 100644 --- a/db/table_cache.cc +++ b/db/table_cache.cc @@ -9,7 +9,6 @@ #include "db/table_cache.h" -#include "cache/simple_deleter.h" #include "db/dbformat.h" #include "db/range_tombstone_fragmenter.h" #include "db/snapshot_impl.h" @@ -34,6 +33,12 @@ namespace ROCKSDB_NAMESPACE { namespace { +template +static void DeleteEntry(const Slice& /*key*/, void* value) { + T* typed_value = reinterpret_cast(value); + delete typed_value; +} + static void UnrefEntry(void* arg1, void* arg2) { Cache* cache = reinterpret_cast(arg1); Cache::Handle* h = reinterpret_cast(arg2); @@ -161,8 +166,8 @@ Status TableCache::FindTable(const FileOptions& file_options, // We do not cache error results so that if the error is transient, // or somebody repairs the file, we recover automatically. } else { - s = cache_->Insert(key, table_reader.get(), 1, - SimpleDeleter::GetInstance(), handle); + s = cache_->Insert(key, table_reader.get(), 1, &DeleteEntry, + handle); if (s.ok()) { // Release ownership of table reader. table_reader.release(); @@ -420,7 +425,7 @@ Status TableCache::Get(const ReadOptions& options, row_cache_key.Size() + row_cache_entry->size() + sizeof(std::string); void* row_ptr = new std::string(std::move(*row_cache_entry)); ioptions_.row_cache->Insert(row_cache_key.GetUserKey(), row_ptr, charge, - SimpleDeleter::GetInstance()); + &DeleteEntry); } #endif // ROCKSDB_LITE @@ -540,7 +545,7 @@ Status TableCache::MultiGet(const ReadOptions& options, row_cache_key.Size() + row_cache_entry.size() + sizeof(std::string); void* row_ptr = new std::string(std::move(row_cache_entry)); ioptions_.row_cache->Insert(row_cache_key.GetUserKey(), row_ptr, charge, - SimpleDeleter::GetInstance()); + &DeleteEntry); } } } diff --git a/include/rocksdb/cache.h b/include/rocksdb/cache.h index 18b9bb826..77ddf525d 100644 --- a/include/rocksdb/cache.h +++ b/include/rocksdb/cache.h @@ -132,13 +132,6 @@ extern std::shared_ptr NewClockCache( kDefaultCacheMetadataChargePolicy); class Cache { public: - class Deleter { - public: - virtual ~Deleter() = default; - - virtual void operator()(const Slice& key, void* value) = 0; - }; - // Depending on implementation, cache entries with high priority could be less // likely to get evicted than low priority entries. enum class Priority { HIGH, LOW }; @@ -175,10 +168,10 @@ class Cache { // insert. In case of error value will be cleanup. // // When the inserted entry is no longer needed, the key and - // value will be passed to "deleter". It is the caller's responsibility to - // ensure that the deleter outlives the cache entries referring to it. + // value will be passed to "deleter". virtual Status Insert(const Slice& key, void* value, size_t charge, - Deleter* deleter, Handle** handle = nullptr, + void (*deleter)(const Slice& key, void* value), + Handle** handle = nullptr, Priority priority = Priority::LOW) = 0; // If the cache has no mapping for "key", returns nullptr. diff --git a/table/block_based/block_based_table_builder.cc b/table/block_based/block_based_table_builder.cc index 2c2d5c967..fda8125a6 100644 --- a/table/block_based/block_based_table_builder.cc +++ b/table/block_based/block_based_table_builder.cc @@ -18,7 +18,6 @@ #include #include -#include "cache/simple_deleter.h" #include "db/dbformat.h" #include "index_builder.h" @@ -796,6 +795,11 @@ Status BlockBasedTableBuilder::status() const { return rep_->status; } IOStatus BlockBasedTableBuilder::io_status() const { return rep_->io_status; } +static void DeleteCachedBlockContents(const Slice& /*key*/, void* value) { + BlockContents* bc = reinterpret_cast(value); + delete bc; +} + // // Make a copy of the block contents and insert into compressed block cache // @@ -830,7 +834,7 @@ Status BlockBasedTableBuilder::InsertBlockInCache(const Slice& block_contents, block_cache_compressed->Insert( key, block_contents_to_cache, block_contents_to_cache->ApproximateMemoryUsage(), - SimpleDeleter::GetInstance()); + &DeleteCachedBlockContents); // Invalidate OS cache. r->file->InvalidateCache(static_cast(r->offset), size); diff --git a/table/block_based/block_based_table_reader.cc b/table/block_based/block_based_table_reader.cc index 3bac71a2f..01557231b 100644 --- a/table/block_based/block_based_table_reader.cc +++ b/table/block_based/block_based_table_reader.cc @@ -14,8 +14,6 @@ #include #include -#include "cache/simple_deleter.h" - #include "db/dbformat.h" #include "db/pinned_iterators_manager.h" @@ -181,6 +179,13 @@ Status ReadBlockFromFile( return s; } +// Delete the entry resided in the cache. +template +void DeleteCachedEntry(const Slice& /*key*/, void* value) { + auto entry = reinterpret_cast(value); + delete entry; +} + // Release the cached entry and decrement its ref count. // Do not force erase void ReleaseCachedEntry(void* arg, void* h) { @@ -1166,8 +1171,7 @@ Status BlockBasedTable::GetDataBlockFromCache( size_t charge = block_holder->ApproximateMemoryUsage(); Cache::Handle* cache_handle = nullptr; s = block_cache->Insert(block_cache_key, block_holder.get(), charge, - SimpleDeleter::GetInstance(), - &cache_handle); + &DeleteCachedEntry, &cache_handle); if (s.ok()) { assert(cache_handle != nullptr); block->SetCachedValue(block_holder.release(), block_cache, @@ -1256,7 +1260,7 @@ Status BlockBasedTable::PutDataBlockToCache( s = block_cache_compressed->Insert( compressed_block_cache_key, block_cont_for_comp_cache, block_cont_for_comp_cache->ApproximateMemoryUsage(), - SimpleDeleter::GetInstance()); + &DeleteCachedEntry); if (s.ok()) { // Avoid the following code to delete this cached block. RecordTick(statistics, BLOCK_CACHE_COMPRESSED_ADD); @@ -1271,8 +1275,8 @@ Status BlockBasedTable::PutDataBlockToCache( size_t charge = block_holder->ApproximateMemoryUsage(); Cache::Handle* cache_handle = nullptr; s = block_cache->Insert(block_cache_key, block_holder.get(), charge, - SimpleDeleter::GetInstance(), - &cache_handle, priority); + &DeleteCachedEntry, &cache_handle, + priority); if (s.ok()) { assert(cache_handle != nullptr); cached_block->SetCachedValue(block_holder.release(), block_cache, diff --git a/table/block_based/partitioned_index_reader.cc b/table/block_based/partitioned_index_reader.cc index c11e56cbb..c22ec4364 100644 --- a/table/block_based/partitioned_index_reader.cc +++ b/table/block_based/partitioned_index_reader.cc @@ -7,8 +7,6 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. See the AUTHORS file for names of contributors. #include "table/block_based/partitioned_index_reader.h" - -#include "cache/simple_deleter.h" #include "table/block_based/partitioned_index_iterator.h" namespace ROCKSDB_NAMESPACE { diff --git a/utilities/simulator_cache/sim_cache.cc b/utilities/simulator_cache/sim_cache.cc index 47f8ce3a9..ec411cf9a 100644 --- a/utilities/simulator_cache/sim_cache.cc +++ b/utilities/simulator_cache/sim_cache.cc @@ -168,15 +168,19 @@ class SimCacheImpl : public SimCache { cache_->SetStrictCapacityLimit(strict_capacity_limit); } - Status Insert(const Slice& key, void* value, size_t charge, Deleter* deleter, - Handle** handle, Priority priority) override { + Status Insert(const Slice& key, void* value, size_t charge, + void (*deleter)(const Slice& key, void* value), Handle** handle, + Priority priority) override { // The handle and value passed in are for real cache, so we pass nullptr - // to key_only_cache_ for both instead. Also, the deleter should be invoked - // only once (on the actual value), so we pass nullptr to key_only_cache for - // that one as well. + // to key_only_cache_ for both instead. Also, the deleter function pointer + // will be called by user to perform some external operation which should + // be applied only once. Thus key_only_cache accepts an empty function. + // *Lambda function without capture can be assgined to a function pointer Handle* h = key_only_cache_->Lookup(key); if (h == nullptr) { - key_only_cache_->Insert(key, nullptr, charge, nullptr, nullptr, priority); + key_only_cache_->Insert(key, nullptr, charge, + [](const Slice& /*k*/, void* /*v*/) {}, nullptr, + priority); } else { key_only_cache_->Release(h); }