Towards a production-quality ClockCache #10418

guidotag · 2022-07-26T04:45:26Z

Summary: In this PR we bring ClockCache closer to production quality. We implement the following changes:

Fixed a few bugs in ClockCache.
ClockCache now fully supports strict_capacity_limit == false: When an insertion over capacity is commanded, we allocate a handle separately from the hash table.
ClockCache now runs on almost every test in cache_test. The only exceptions are a test where either the LRU policy is required, and a test that dynamically increases the table capacity.
ClockCache now supports dynamically decreasing capacity via SetCapacity. (This is easy: we shrink the capacity upper bound and run the clock algorithm.)
Old FastLRUCache tests in lru_cache_test.cc are now also used on ClockCache.

As a byproduct of 1. and 2. we are able to turn on ClockCache in the stress tests.

Test plan:

make -j24 USE_CLANG=1 COMPILE_WITH_ASAN=1 COMPILE_WITH_UBSAN=1 check
make -j24 USE_CLANG=1 COMPILE_WITH_TSAN=1 check
make -j24 USE_CLANG=1 COMPILE_WITH_ASAN=1 COMPILE_WITH_UBSAN=1 CRASH_TEST_EXT_ARGS="--duration=960 --cache_type=clock_cache" blackbox_crash_test_with_atomic_flush
make -j24 USE_CLANG=1 COMPILE_WITH_TSAN=1 CRASH_TEST_EXT_ARGS="--duration=960 --cache_type=clock_cache" blackbox_crash_test_with_atomic_flush

pdillinger · 2022-07-26T14:55:22Z

cache/cache_test.cc

-  if (type == kFast || type == kClock) {
-    ROCKSDB_GTEST_BYPASS("FastLRUCache and ClockCache require 16-byte keys.");
-    return;
-  }

  // cache is std::shared_ptr and will be automatically cleaned up.
  const uint64_t kCapacity = 200000;


Use size_t, to keep 32-bit compilers happy

pdillinger · 2022-07-26T15:05:25Z

cache/clock_cache.cc

  autovector<ClockHandle> deleted;
  uint32_t max_iterations =
-      1 + static_cast<uint32_t>(GetTableSize() * kLoadFactor);
+      ClockHandle::ClockPriority::HIGH *


It may take up to HIGH passes over the array to evict an item. I will add a comment.

But the shift by kClockPriorityOffset seems unnecessarily and confusingly intermingled.

pdillinger · 2022-07-26T16:02:26Z

cache/clock_cache.h

@@ -274,14 +278,18 @@ struct ClockHandle {

  std::atomic<uint32_t> refs;

+  // True iff the handle is allocated separately from hash table.
+  bool dangling;


I don't like "dangling" to describe this kind of handle, because something "dangling" is loosely attached. To me that sounds more like erased entries. How about "stopgap"?

What about "detached"?

Fine with me. To me, "stopgap" is more associated with rare and degraded functionality (can't lookup the entry) than "detached." But not a big difference.

pdillinger · 2022-07-26T16:07:51Z

cache/clock_cache.cc

+    if (occupancy_local + 1 > table_.GetOccupancyLimit()) {
+      // Even if the user wishes to overload the cache, we can't insert into
+      // the hash table. Instead, we dynamically allocate a new handle.
+      h = reinterpret_cast<ClockHandle*>(new ClockHandle());


Unnecessary cast

Test comment.

guidotag · 2022-07-26T18:21:26Z

Test comment.

guidotag · 2022-07-26T04:50:23Z

cache/cache_test.cc

-  double load_factor =
-      std::min(fast_lru_cache::kLoadFactor, clock_cache::kLoadFactor);
-  for (int i = 0; i < 2 * static_cast<int>(kCacheSize / load_factor); i++) {
+  for (int i = 0; i < 100 * kCacheSize; i++) {


I removed the load_factor thing so we don't rely so specifically on implementation details.

guidotag · 2022-07-26T04:52:01Z

cache/clock_cache.cc

    e->SetHit();
    // The handle is now referenced, so we take it out of clock.
    ClockOff(e);
+    e->InternalToExternalRef();


This is not for correctness. I want the following invariant to hold: with an external reference, no field is modified.

guidotag · 2022-07-26T04:52:42Z

cache/clock_cache.cc

@@ -358,6 +356,7 @@ ClockCacheShard::ClockCacheShard(
    size_t capacity, size_t estimated_value_size, bool strict_capacity_limit,
    CacheMetadataChargePolicy metadata_charge_policy)
    : strict_capacity_limit_(strict_capacity_limit),
+      dangling_usage_(0),


This is the usage by elements that have been dynamically allocated separately from the table.

guidotag · 2022-07-26T04:54:56Z

cache/clock_cache.cc

    if (handle == nullptr) {
      // Don't insert the entry but still return ok, as if the entry inserted
      // into cache and get evicted immediately.
-      deleted.push_back(tmp);
+      tmp.FreeData();


guidotag · 2022-07-26T04:56:54Z

cache/clock_cache.cc

  // Free space with the clock policy until enough space is freed or there are
  // no evictable elements.
-  table_.ClockRun(tmp.total_charge);
+  table_.ClockRun(tmp.total_charge + dangling_usage);


The ClockHandleTable is oblivious to dangling handles.

guidotag · 2022-07-26T05:12:22Z

cache/lru_cache_test.cc

-    new (cache_) fast_lru_cache::LRUCacheShard(
-        capacity, 1 /*estimated_value_size*/, false /*strict_capacity_limit*/,
-        kDontChargeCacheMetadata);
+    cache_ = reinterpret_cast<LRUCacheShard*>(


FastLRUCache tests didn't change. Only cosmetic changes.

guidotag · 2022-07-26T05:13:46Z

cache/lru_cache_test.cc

-  // ASSERT_TRUE(in_high_pri_pool);
-  // ASSERT_EQ(num_high_pri_pool_keys, high_pri_pool_keys);
-  // }
+  size_t CalcEstimatedHandleChargeWrapper(


Ported tests from FastLRUCache.

guidotag · 2022-07-26T16:30:04Z

cache/clock_cache.cc

  autovector<ClockHandle> deleted;
  uint32_t max_iterations =
-      1 + static_cast<uint32_t>(GetTableSize() * kLoadFactor);
+      ClockHandle::ClockPriority::HIGH *


It may take up to HIGH passes over the array to evict an item. I will add a comment.

guidotag · 2022-07-26T16:45:56Z

cache/clock_cache.h

@@ -274,14 +278,18 @@ struct ClockHandle {

  std::atomic<uint32_t> refs;

+  // True iff the handle is allocated separately from hash table.
+  bool dangling;


What about "detached"?

guidotag · 2022-07-26T18:21:38Z

cache/clock_cache.cc

+    if (occupancy_local + 1 > table_.GetOccupancyLimit()) {
+      // Even if the user wishes to overload the cache, we can't insert into
+      // the hash table. Instead, we dynamically allocate a new handle.
+      h = reinterpret_cast<ClockHandle*>(new ClockHandle());


Test comment.

pdillinger

Overall LGTM

pdillinger · 2022-07-26T18:29:32Z

cache/clock_cache.cc

  autovector<ClockHandle> deleted;
  uint32_t max_iterations =
-      1 + static_cast<uint32_t>(GetTableSize() * kLoadFactor);
+      ClockHandle::ClockPriority::HIGH *


But the shift by kClockPriorityOffset seems unnecessarily and confusingly intermingled.

pdillinger · 2022-07-26T18:36:32Z

cache/clock_cache.cc

@@ -530,6 +550,20 @@ bool ClockCacheShard::Release(Cache::Handle* handle, bool erase_if_last_ref) {
  }

  ClockHandle* h = reinterpret_cast<ClockHandle*>(handle);
+
+  if (h->IsDangling()) {


Nice observation, like other immutable fields. And in normal operation, it's a very predictable branch.

Nit: without historical information, IIRC default branch prediction usually goes into an if rather than skipping over it (because on average, jumps are quite in the negative direction). We have a hint mechanism for cases like this: if (UNLIKELY(...)). Feel free to use LIKELY and UNLIKELY in more places when there's an "almost always" direction.

pdillinger · 2022-07-26T18:38:55Z

cache/clock_cache.h

@@ -274,14 +278,18 @@ struct ClockHandle {

  std::atomic<uint32_t> refs;

+  // True iff the handle is allocated separately from hash table.
+  bool dangling;


Fine with me. To me, "stopgap" is more associated with rare and degraded functionality (can't lookup the entry) than "detached." But not a big difference.

facebook-github-bot · 2022-07-26T19:32:35Z