Refactor ClockCache ApplyToEntries (#11609)

Summary:
... ahead of planned dynamic HCC variant. This changes
simplifies some logic while still enabling future code sharing between
implementation variants.

Detail: For complicated reasons, using a std::function parameter to
`ConstApplyToEntriesRange` with a lambda argument does not play
nice with templated HandleImpl. An explicit conversion to std::function
would be needed for it to compile. Templating the function type is the
easy work-around.

Also made some functions from https://github.com/facebook/rocksdb/issues/11572 private as recommended

Pull Request resolved: https://github.com/facebook/rocksdb/pull/11609

Test Plan: existing tests

Reviewed By: jowlyzhang

Differential Revision: D47407415

Pulled By: pdillinger

fbshipit-source-id: 0f65954db16335999b78fb7d2563ec627624cef0

cache/clock_cache.cc

@@ -289,6 +289,46 @@ bool TryInsert(const ClockHandleBasicData& proto, ClockHandle& h,
   return b;
 }
 
+template <class HandleImpl, class Func>
+void ConstApplyToEntriesRange(Func /*const HandleImpl& -> void*/ func,
+                              const HandleImpl* begin, const HandleImpl* end,
+                              bool apply_if_will_be_deleted) {
+  uint64_t check_state_mask = ClockHandle::kStateShareableBit;
+  if (!apply_if_will_be_deleted) {
+    check_state_mask |= ClockHandle::kStateVisibleBit;
+  }
+
+  for (const HandleImpl* h = begin; h < end; ++h) {
+    // Note: to avoid using compare_exchange, we have to be extra careful.
+    uint64_t old_meta = h->meta.load(std::memory_order_relaxed);
+    // Check if it's an entry visible to lookups
+    if ((old_meta >> ClockHandle::kStateShift) & check_state_mask) {
+      // Increment acquire counter. Note: it's possible that the entry has
+      // completely changed since we loaded old_meta, but incrementing acquire
+      // count is always safe. (Similar to optimistic Lookup here.)
+      old_meta = h->meta.fetch_add(ClockHandle::kAcquireIncrement,
+                                   std::memory_order_acquire);
+      // Check whether we actually acquired a reference.
+      if ((old_meta >> ClockHandle::kStateShift) &
+          ClockHandle::kStateShareableBit) {
+        // Apply func if appropriate
+        if ((old_meta >> ClockHandle::kStateShift) & check_state_mask) {
+          func(*h);
+        }
+        // Pretend we never took the reference
+        h->meta.fetch_sub(ClockHandle::kAcquireIncrement,
+                          std::memory_order_release);
+        // No net change, so don't need to check for overflow
+      } else {
+        // For other states, incrementing the acquire counter has no effect
+        // so we don't need to undo it. Furthermore, we cannot safely undo
+        // it because we did not acquire a read reference to lock the
+        // entry in a Shareable state.
+      }
+    }
+  }
+}
+
 }  // namespace
 
 void ClockHandleBasicData::FreeData(MemoryAllocator* allocator) const {
@@ -944,47 +984,6 @@ void HyperClockTable::Erase(const UniqueId64x2& hashed_key) {
       [&](HandleImpl* /*h*/, bool /*is_last*/) {});
 }
 
-void HyperClockTable::ConstApplyToEntriesRange(
-    std::function<void(const HandleImpl&)> func, size_t index_begin,
-    size_t index_end, bool apply_if_will_be_deleted) const {
-  uint64_t check_state_mask = ClockHandle::kStateShareableBit;
-  if (!apply_if_will_be_deleted) {
-    check_state_mask |= ClockHandle::kStateVisibleBit;
-  }
-
-  for (size_t i = index_begin; i < index_end; i++) {
-    HandleImpl& h = array_[i];
-
-    // Note: to avoid using compare_exchange, we have to be extra careful.
-    uint64_t old_meta = h.meta.load(std::memory_order_relaxed);
-    // Check if it's an entry visible to lookups
-    if ((old_meta >> ClockHandle::kStateShift) & check_state_mask) {
-      // Increment acquire counter. Note: it's possible that the entry has
-      // completely changed since we loaded old_meta, but incrementing acquire
-      // count is always safe. (Similar to optimistic Lookup here.)
-      old_meta = h.meta.fetch_add(ClockHandle::kAcquireIncrement,
-                                  std::memory_order_acquire);
-      // Check whether we actually acquired a reference.
-      if ((old_meta >> ClockHandle::kStateShift) &
-          ClockHandle::kStateShareableBit) {
-        // Apply func if appropriate
-        if ((old_meta >> ClockHandle::kStateShift) & check_state_mask) {
-          func(h);
-        }
-        // Pretend we never took the reference
-        h.meta.fetch_sub(ClockHandle::kAcquireIncrement,
-                         std::memory_order_release);
-        // No net change, so don't need to check for overflow
-      } else {
-        // For other states, incrementing the acquire counter has no effect
-        // so we don't need to undo it. Furthermore, we cannot safely undo
-        // it because we did not acquire a read reference to lock the
-        // entry in a Shareable state.
-      }
-    }
-  }
-}
-
 void HyperClockTable::EraseUnRefEntries() {
   for (size_t i = 0; i <= this->length_bits_mask_; i++) {
     HandleImpl& h = array_[i];
@@ -1150,35 +1149,32 @@ void ClockCacheShard<Table>::ApplyToSomeEntries(
                              size_t charge,
                              const Cache::CacheItemHelper* helper)>& callback,
     size_t average_entries_per_lock, size_t* state) {
-  // The state is essentially going to be the starting hash, which works
+  // The state will be a simple index into the table. Even with a dynamic
-  // nicely even if we resize between calls because we use upper-most
+  // hyper clock cache, entries will generally stay in their existing
-  // hash bits for table indexes.
+  // slots, so we don't need to be aware of the high-level organization
-  size_t length_bits = table_.GetLengthBits();
+  // that makes lookup efficient.
   size_t length = table_.GetTableSize();
 
   assert(average_entries_per_lock > 0);
-  // Assuming we are called with same average_entries_per_lock repeatedly,
-  // this simplifies some logic (index_end will not overflow).
-  assert(average_entries_per_lock < length || *state == 0);
 
-  size_t index_begin = *state >> (sizeof(size_t) * 8u - length_bits);
+  size_t index_begin = *state;
   size_t index_end = index_begin + average_entries_per_lock;
   if (index_end >= length) {
     // Going to end.
     index_end = length;
     *state = SIZE_MAX;
   } else {
-    *state = index_end << (sizeof(size_t) * 8u - length_bits);
+    *state = index_end;
   }
 
   auto hash_seed = table_.GetHashSeed();
-  table_.ConstApplyToEntriesRange(
+  ConstApplyToEntriesRange(
       [callback, hash_seed](const HandleImpl& h) {
         UniqueId64x2 unhashed;
         callback(ReverseHash(h.hashed_key, &unhashed, hash_seed), h.value,
                  h.GetTotalCharge(), h.helper);
       },
-      index_begin, index_end, false);
+      table_.HandlePtr(index_begin), table_.HandlePtr(index_end), false);
 }
 
 int HyperClockTable::CalcHashBits(
@@ -1335,7 +1331,7 @@ size_t ClockCacheShard<Table>::GetPinnedUsage() const {
   size_t table_pinned_usage = 0;
   const bool charge_metadata =
       metadata_charge_policy_ == kFullChargeCacheMetadata;
-  table_.ConstApplyToEntriesRange(
+  ConstApplyToEntriesRange(
       [&table_pinned_usage, charge_metadata](const HandleImpl& h) {
         uint64_t meta = h.meta.load(std::memory_order_relaxed);
         uint64_t refcount = GetRefcount(meta);
@@ -1348,7 +1344,7 @@ size_t ClockCacheShard<Table>::GetPinnedUsage() const {
           }
         }
       },
-      0, table_.GetTableSize(), true);
+      table_.HandlePtr(0), table_.HandlePtr(table_.GetTableSize()), true);
 
   return table_pinned_usage + table_.GetStandaloneUsage();
 }

cache/clock_cache.h

@@ -371,8 +371,8 @@ struct ClockHandle : public ClockHandleBasicData {
   static constexpr uint8_t kMaxCountdown = kHighCountdown;
   // TODO: make these coundown values tuning parameters for eviction?
 
-  // See above
+  // See above. Mutable for read reference counting.
-  std::atomic<uint64_t> meta{};
+  mutable std::atomic<uint64_t> meta{};
 
   // Whether this is a "deteched" handle that is independently allocated
   // with `new` (so must be deleted with `delete`).
@@ -397,42 +397,12 @@ class BaseClockTable {
         eviction_callback_(*eviction_callback),
         hash_seed_(*hash_seed) {}
 
-  // Creates a "standalone" handle for returning from an Insert operation that
-  // cannot be completed by actually inserting into the table.
-  // Updates `standalone_usage_` but not `usage_` nor `occupancy_`.
-  template <class HandleImpl>
-  HandleImpl* StandaloneInsert(const ClockHandleBasicData& proto);
-
   template <class Table>
   typename Table::HandleImpl* CreateStandalone(ClockHandleBasicData& proto,
                                                size_t capacity,
                                                bool strict_capacity_limit,
                                                bool allow_uncharged);
 
-  // Helper for updating `usage_` for new entry with given `total_charge`
-  // and evicting if needed under strict_capacity_limit=true rules. This
-  // means the operation might fail with Status::MemoryLimit. If
-  // `need_evict_for_occupancy`, then eviction of at least one entry is
-  // required, and the operation should fail if not possible.
-  // NOTE: Otherwise, occupancy_ is not managed in this function
-  template <class Table>
-  Status ChargeUsageMaybeEvictStrict(size_t total_charge, size_t capacity,
-                                     bool need_evict_for_occupancy,
-                                     typename Table::InsertState& state);
-
-  // Helper for updating `usage_` for new entry with given `total_charge`
-  // and evicting if needed under strict_capacity_limit=false rules. This
-  // means that updating `usage_` always succeeds even if forced to exceed
-  // capacity. If `need_evict_for_occupancy`, then eviction of at least one
-  // entry is required, and the operation should return false if such eviction
-  // is not possible. `usage_` is not updated in that case. Otherwise, returns
-  // true, indicating success.
-  // NOTE: occupancy_ is not managed in this function
-  template <class Table>
-  bool ChargeUsageMaybeEvictNonStrict(size_t total_charge, size_t capacity,
-                                      bool need_evict_for_occupancy,
-                                      typename Table::InsertState& state);
-
   template <class Table>
   Status Insert(const ClockHandleBasicData& proto,
                 typename Table::HandleImpl** handle, Cache::Priority priority,
@@ -459,7 +429,38 @@ class BaseClockTable {
   void TEST_ReleaseNMinus1(ClockHandle* handle, size_t n);
 #endif
 
- protected:
+ private:  // fns
+  // Creates a "standalone" handle for returning from an Insert operation that
+  // cannot be completed by actually inserting into the table.
+  // Updates `standalone_usage_` but not `usage_` nor `occupancy_`.
+  template <class HandleImpl>
+  HandleImpl* StandaloneInsert(const ClockHandleBasicData& proto);
+
+  // Helper for updating `usage_` for new entry with given `total_charge`
+  // and evicting if needed under strict_capacity_limit=true rules. This
+  // means the operation might fail with Status::MemoryLimit. If
+  // `need_evict_for_occupancy`, then eviction of at least one entry is
+  // required, and the operation should fail if not possible.
+  // NOTE: Otherwise, occupancy_ is not managed in this function
+  template <class Table>
+  Status ChargeUsageMaybeEvictStrict(size_t total_charge, size_t capacity,
+                                     bool need_evict_for_occupancy,
+                                     typename Table::InsertState& state);
+
+  // Helper for updating `usage_` for new entry with given `total_charge`
+  // and evicting if needed under strict_capacity_limit=false rules. This
+  // means that updating `usage_` always succeeds even if forced to exceed
+  // capacity. If `need_evict_for_occupancy`, then eviction of at least one
+  // entry is required, and the operation should return false if such eviction
+  // is not possible. `usage_` is not updated in that case. Otherwise, returns
+  // true, indicating success.
+  // NOTE: occupancy_ is not managed in this function
+  template <class Table>
+  bool ChargeUsageMaybeEvictNonStrict(size_t total_charge, size_t capacity,
+                                      bool need_evict_for_occupancy,
+                                      typename Table::InsertState& state);
+
+ protected:  // data
   // We partition the following members into different cache lines
   // to avoid false sharing among Lookup, Release, Erase and Insert
   // operations in ClockCacheShard.
@@ -536,18 +537,14 @@ class HyperClockTable : public BaseClockTable {
 
   void Erase(const UniqueId64x2& hashed_key);
 
-  void ConstApplyToEntriesRange(std::function<void(const HandleImpl&)> func,
-                                size_t index_begin, size_t index_end,
-                                bool apply_if_will_be_deleted) const;
-
   void EraseUnRefEntries();
 
   size_t GetTableSize() const { return size_t{1} << length_bits_; }
 
-  int GetLengthBits() const { return length_bits_; }
-
   size_t GetOccupancyLimit() const { return occupancy_limit_; }
 
+  const HandleImpl* HandlePtr(size_t idx) const { return &array_[idx]; }
+
 #ifndef NDEBUG
   size_t& TEST_MutableOccupancyLimit() const {
     return const_cast<size_t&>(occupancy_limit_);