From 9645e66fc938cbde15324cdc63b571fa5f35a7e8 Mon Sep 17 00:00:00 2001 From: Guido Tagliavini Ponce Date: Wed, 13 Jul 2022 08:45:44 -0700 Subject: [PATCH] Temporarily return a LRUCache from NewClockCache (#10351) Summary: ClockCache is still in experimental stage, and currently fails some pre-release fbcode tests. See https://www.internalfb.com/diff/D37772011. API calls to construct ClockCache are done via the function NewClockCache. For now, NewClockCache calls will return an LRUCache (with appropriate arguments), which is stable. The idea that NewClockCache returns nullptr was also floated, but this would be interpreted as unsupported cache, and a default LRUCache would be constructed instead, potentially causing a performance regression that is harder to identify. A new version of the NewClockCache function was created for our internal tests. Pull Request resolved: https://github.com/facebook/rocksdb/pull/10351 Test Plan: ``make -j24 check`` and re-run the pre-release tests. Reviewed By: pdillinger Differential Revision: D37802685 Pulled By: guidotag fbshipit-source-id: 0a8d10612ff21e576f7360cb13e20bc36e244972 --- HISTORY.md | 1 + cache/cache_bench_tool.cc | 3 ++- cache/cache_test.cc | 12 +++++++----- cache/clock_cache.cc | 8 ++++++++ cache/clock_cache.h | 7 +++++++ db/db_block_cache_test.cc | 7 ++++--- include/rocksdb/cache.h | 9 ++++++++- tools/db_bench_tool.cc | 9 +++++---- 8 files changed, 42 insertions(+), 14 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index f90b460e1..1840dc7ac 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -17,6 +17,7 @@ * `rocksdb_file_metadata_t` and its and get functions & destroy functions. * Add suggest_compact_range() and suggest_compact_range_cf() to C API. * When using block cache strict capacity limit (`LRUCache` with `strict_capacity_limit=true`), DB operations now fail with Status code `kAborted` subcode `kMemoryLimit` (`IsMemoryLimit()`) instead of `kIncomplete` (`IsIncomplete()`) when the capacity limit is reached, because Incomplete can mean other specific things for some operations. In more detail, `Cache::Insert()` now returns the updated Status code and this usually propagates through RocksDB to the user on failure. +* NewClockCache calls temporarily return an LRUCache (with similar characteristics as the desired ClockCache). This is because ClockCache is being replaced by a new version (the old one had unknown bugs) but this is still under development. * Add two functions `int ReserveThreads(int threads_to_be_reserved)` and `int ReleaseThreads(threads_to_be_released)` into `Env` class. In the default implementation, both return 0. Newly added `xxxEnv` class that inherits `Env` should implement these two functions for thread reservation/releasing features. ### Bug Fixes diff --git a/cache/cache_bench_tool.cc b/cache/cache_bench_tool.cc index f2b32daba..61818cb34 100644 --- a/cache/cache_bench_tool.cc +++ b/cache/cache_bench_tool.cc @@ -13,6 +13,7 @@ #include #include +#include "cache/clock_cache.h" #include "cache/fast_lru_cache.h" #include "db/db_impl/db_impl.h" #include "monitoring/histogram.h" @@ -284,7 +285,7 @@ class CacheBench { } if (FLAGS_cache_type == "clock_cache") { - cache_ = NewClockCache( + cache_ = ExperimentalNewClockCache( FLAGS_cache_size, FLAGS_value_bytes, FLAGS_num_shard_bits, false /*strict_capacity_limit*/, kDefaultCacheMetadataChargePolicy); if (!cache_) { diff --git a/cache/cache_test.cc b/cache/cache_test.cc index f9acb88f0..0a008a1d9 100644 --- a/cache/cache_test.cc +++ b/cache/cache_test.cc @@ -111,7 +111,7 @@ class CacheTest : public testing::TestWithParam { return NewLRUCache(capacity); } if (type == kClock) { - return NewClockCache( + return ExperimentalNewClockCache( capacity, 1 /*estimated_value_size*/, -1 /*num_shard_bits*/, false /*strict_capacity_limit*/, kDefaultCacheMetadataChargePolicy); } @@ -137,8 +137,9 @@ class CacheTest : public testing::TestWithParam { return NewLRUCache(co); } if (type == kClock) { - return NewClockCache(capacity, 1 /*estimated_value_size*/, num_shard_bits, - strict_capacity_limit, charge_policy); + return ExperimentalNewClockCache(capacity, 1 /*estimated_value_size*/, + num_shard_bits, strict_capacity_limit, + charge_policy); } if (type == kFast) { return NewFastLRUCache(capacity, 1 /*estimated_value_size*/, @@ -954,8 +955,9 @@ TEST_P(CacheTest, GetChargeAndDeleter) { cache_->Release(h1); } -std::shared_ptr (*new_clock_cache_func)( - size_t, size_t, int, bool, CacheMetadataChargePolicy) = NewClockCache; +std::shared_ptr (*new_clock_cache_func)(size_t, size_t, int, bool, + CacheMetadataChargePolicy) = + ExperimentalNewClockCache; INSTANTIATE_TEST_CASE_P(CacheTestInstance, CacheTest, testing::Values(kLRU, kClock, kFast)); INSTANTIATE_TEST_CASE_P(CacheTestInstance, LRUCacheTest, diff --git a/cache/clock_cache.cc b/cache/clock_cache.cc index e9dd77f9d..86b4f0471 100644 --- a/cache/clock_cache.cc +++ b/cache/clock_cache.cc @@ -584,6 +584,14 @@ void ClockCache::DisownData() { } // namespace clock_cache std::shared_ptr NewClockCache( + size_t capacity, size_t /*estimated_value_size*/, int num_shard_bits, + bool strict_capacity_limit, + CacheMetadataChargePolicy metadata_charge_policy) { + return NewLRUCache(capacity, num_shard_bits, strict_capacity_limit, 0.5, + nullptr, kDefaultToAdaptiveMutex, metadata_charge_policy); +} + +std::shared_ptr ExperimentalNewClockCache( size_t capacity, size_t estimated_value_size, int num_shard_bits, bool strict_capacity_limit, CacheMetadataChargePolicy metadata_charge_policy) { diff --git a/cache/clock_cache.h b/cache/clock_cache.h index dadaa88f3..ca6205b83 100644 --- a/cache/clock_cache.h +++ b/cache/clock_cache.h @@ -472,4 +472,11 @@ class ClockCache } // namespace clock_cache +// Only for internal testing, temporarily replacing NewClockCache. +// TODO(Guido) Remove once NewClockCache constructs a ClockCache again. +extern std::shared_ptr ExperimentalNewClockCache( + size_t capacity, size_t estimated_value_size, int num_shard_bits, + bool strict_capacity_limit, + CacheMetadataChargePolicy metadata_charge_policy); + } // namespace ROCKSDB_NAMESPACE diff --git a/db/db_block_cache_test.cc b/db/db_block_cache_test.cc index d4ee1d8b2..d2cf32316 100644 --- a/db/db_block_cache_test.cc +++ b/db/db_block_cache_test.cc @@ -13,6 +13,7 @@ #include "cache/cache_entry_roles.h" #include "cache/cache_key.h" +#include "cache/clock_cache.h" #include "cache/fast_lru_cache.h" #include "cache/lru_cache.h" #include "db/column_family.h" @@ -935,9 +936,9 @@ TEST_F(DBBlockCacheTest, AddRedundantStats) { int iterations_tested = 0; for (std::shared_ptr base_cache : {NewLRUCache(capacity, num_shard_bits), - NewClockCache(capacity, 1 /*estimated_value_size*/, num_shard_bits, - false /*strict_capacity_limit*/, - kDefaultCacheMetadataChargePolicy), + ExperimentalNewClockCache( + capacity, 1 /*estimated_value_size*/, num_shard_bits, + false /*strict_capacity_limit*/, kDefaultCacheMetadataChargePolicy), NewFastLRUCache(capacity, 1 /*estimated_value_size*/, num_shard_bits, false /*strict_capacity_limit*/, kDefaultCacheMetadataChargePolicy)}) { diff --git a/include/rocksdb/cache.h b/include/rocksdb/cache.h index 6194b0c66..49f5b80bc 100644 --- a/include/rocksdb/cache.h +++ b/include/rocksdb/cache.h @@ -174,11 +174,18 @@ extern std::shared_ptr NewCompressedSecondaryCache( extern std::shared_ptr NewCompressedSecondaryCache( const CompressedSecondaryCacheOptions& opts); +// EXPERIMENTAL Currently ClockCache is under development, although it's +// already exposed in the public API. To avoid unreliable performance and +// correctness issues, NewClockCache will temporarily return an LRUCache +// constructed with the corresponding arguments. +// +// TODO(Guido) When ClockCache is complete, roll back to the old text: +// `` // Similar to NewLRUCache, but create a cache based on clock algorithm with // better concurrent performance in some cases. See util/clock_cache.cc for // more detail. -// // Return nullptr if it is not supported. +// `` extern std::shared_ptr NewClockCache( size_t capacity, size_t estimated_value_size, int num_shard_bits, bool strict_capacity_limit, diff --git a/tools/db_bench_tool.cc b/tools/db_bench_tool.cc index 6fe45bb40..61aa184d3 100644 --- a/tools/db_bench_tool.cc +++ b/tools/db_bench_tool.cc @@ -37,6 +37,7 @@ #include #include +#include "cache/clock_cache.h" #include "cache/fast_lru_cache.h" #include "db/db_impl/db_impl.h" #include "db/malloc_stats.h" @@ -2977,10 +2978,10 @@ class Benchmark { return nullptr; } if (FLAGS_cache_type == "clock_cache") { - auto cache = NewClockCache(static_cast(capacity), - FLAGS_block_size, FLAGS_cache_numshardbits, - false /*strict_capacity_limit*/, - kDefaultCacheMetadataChargePolicy); + auto cache = ExperimentalNewClockCache( + static_cast(capacity), FLAGS_block_size, + FLAGS_cache_numshardbits, false /*strict_capacity_limit*/, + kDefaultCacheMetadataChargePolicy); if (!cache) { fprintf(stderr, "Clock cache not supported."); exit(1);