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

@@ -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

cache/cache_bench_tool.cc

@@ -13,6 +13,7 @@
 #include <set>
 #include <sstream>
 
+#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_) {

cache/cache_test.cc

@@ -111,7 +111,7 @@ class CacheTest : public testing::TestWithParam<std::string> {
       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<std::string> {
       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<Cache> (*new_clock_cache_func)(
-    size_t, size_t, int, bool, CacheMetadataChargePolicy) = NewClockCache;
+std::shared_ptr<Cache> (*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,

cache/clock_cache.cc

@@ -584,6 +584,14 @@ void ClockCache::DisownData() {
 }  // namespace clock_cache
 
 std::shared_ptr<Cache> 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<Cache> ExperimentalNewClockCache(
     size_t capacity, size_t estimated_value_size, int num_shard_bits,
     bool strict_capacity_limit,
     CacheMetadataChargePolicy metadata_charge_policy) {

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<Cache> ExperimentalNewClockCache(
+    size_t capacity, size_t estimated_value_size, int num_shard_bits,
+    bool strict_capacity_limit,
+    CacheMetadataChargePolicy metadata_charge_policy);
+
 }  // namespace ROCKSDB_NAMESPACE

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<Cache> 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)}) {

include/rocksdb/cache.h

@@ -174,11 +174,18 @@ extern std::shared_ptr<SecondaryCache> NewCompressedSecondaryCache(
 extern std::shared_ptr<SecondaryCache> 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<Cache> NewClockCache(
     size_t capacity, size_t estimated_value_size, int num_shard_bits,
     bool strict_capacity_limit,

tools/db_bench_tool.cc

@@ -37,6 +37,7 @@
 #include <thread>
 #include <unordered_map>
 
+#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<size_t>(capacity),
-                                 FLAGS_block_size, FLAGS_cache_numshardbits,
-                                 false /*strict_capacity_limit*/,
-                                 kDefaultCacheMetadataChargePolicy);
+      auto cache = ExperimentalNewClockCache(
+          static_cast<size_t>(capacity), FLAGS_block_size,
+          FLAGS_cache_numshardbits, false /*strict_capacity_limit*/,
+          kDefaultCacheMetadataChargePolicy);
       if (!cache) {
         fprintf(stderr, "Clock cache not supported.");
         exit(1);