ItemHandle -> WriteHandle for CacheAllocator private functions

jiayuebao · facebook-github-bot · commit c8dc4079ff17 · 2022-04-27T16:42:37.000-07:00
Summary: Clean up `ItemHandle` in CacheAllocator

Reviewed By: therealgymmy

Differential Revision: D34334392

fbshipit-source-id: 0ba983dc0e4f2614b8074ad70585f91d1af3e5e9
diff --git a/cachelib/allocator/CacheAllocator-inl.h b/cachelib/allocator/CacheAllocator-inl.h
@@ -37,11 +37,11 @@ CacheAllocator<CacheTrait>::CacheAllocator(Config config)
       accessContainer_(std::make_unique<AccessContainer>(
           config_.accessConfig,
           compressor_,
-          [this](Item* it) -> ItemHandle { return acquire(it); })),
+          [this](Item* it) -> WriteHandle { return acquire(it); })),
       chainedItemAccessContainer_(std::make_unique<AccessContainer>(
           config_.chainedItemAccessConfig,
           compressor_,
-          [this](Item* it) -> ItemHandle { return acquire(it); })),
+          [this](Item* it) -> WriteHandle { return acquire(it); })),
       chainedItemLocks_(config_.chainedItemsLockPower,
                         std::make_shared<MurmurHash2>()),
       cacheCreationTime_{util::getCurrentTimeSec()},
@@ -75,7 +75,7 @@ CacheAllocator<CacheTrait>::CacheAllocator(SharedMemNewT, Config config)
                           ShmSegmentOpts(config_.accessConfig.getPageSize()))
               .addr,
           compressor_,
-          [this](Item* it) -> ItemHandle { return acquire(it); })),
+          [this](Item* it) -> WriteHandle { return acquire(it); })),
       chainedItemAccessContainer_(std::make_unique<AccessContainer>(
           config_.chainedItemAccessConfig,
           shmManager_
@@ -86,7 +86,7 @@ CacheAllocator<CacheTrait>::CacheAllocator(SharedMemNewT, Config config)
                           ShmSegmentOpts(config_.accessConfig.getPageSize()))
               .addr,
           compressor_,
-          [this](Item* it) -> ItemHandle { return acquire(it); })),
+          [this](Item* it) -> WriteHandle { return acquire(it); })),
       chainedItemLocks_(config_.chainedItemsLockPower,
                         std::make_shared<MurmurHash2>()),
       cacheCreationTime_{util::getCurrentTimeSec()},
@@ -119,13 +119,13 @@ CacheAllocator<CacheTrait>::CacheAllocator(SharedMemAttachT, Config config)
           config_.accessConfig,
           shmManager_->attachShm(detail::kShmHashTableName),
           compressor_,
-          [this](Item* it) -> ItemHandle { return acquire(it); })),
+          [this](Item* it) -> WriteHandle { return acquire(it); })),
       chainedItemAccessContainer_(std::make_unique<AccessContainer>(
           deserializer_->deserialize<AccessSerializationType>(),
           config_.chainedItemAccessConfig,
           shmManager_->attachShm(detail::kShmChainedItemHashTableName),
           compressor_,
-          [this](Item* it) -> ItemHandle { return acquire(it); })),
+          [this](Item* it) -> WriteHandle { return acquire(it); })),
       chainedItemLocks_(config_.chainedItemsLockPower,
                         std::make_shared<MurmurHash2>()),
       cacheCreationTime_{
@@ -871,16 +871,16 @@ RefcountWithFlags::Value CacheAllocator<CacheTrait>::decRef(Item& it) {
 }
 
 template <typename CacheTrait>
-typename CacheAllocator<CacheTrait>::ItemHandle
+typename CacheAllocator<CacheTrait>::WriteHandle
 CacheAllocator<CacheTrait>::acquire(Item* it) {
   if (UNLIKELY(!it)) {
-    return ItemHandle{};
+    return WriteHandle{};
   }
 
   SCOPE_FAIL { stats_.numRefcountOverflow.inc(); };
 
   incRef(*it);
-  return ItemHandle{it, *this};
+  return WriteHandle{it, *this};
 }
 
 template <typename CacheTrait>
@@ -1069,7 +1069,7 @@ CacheAllocator<CacheTrait>::insertOrReplace(const WriteHandle& handle) {
 
 template <typename CacheTrait>
 bool CacheAllocator<CacheTrait>::moveRegularItem(Item& oldItem,
-                                                 ItemHandle& newItemHdl) {
+                                                 WriteHandle& newItemHdl) {
   XDCHECK(config_.moveCb);
   util::LatencyTracker tracker{stats_.moveRegularLatency_};
 
@@ -1102,7 +1102,7 @@ bool CacheAllocator<CacheTrait>::moveRegularItem(Item& oldItem,
   // there is no point to replace it since it had already been removed
   // or in the process of being removed. If the item is in cache but the
   // refcount is non-zero, it means user could be attempting to remove
-  // this item through an API such as remove(ItemHandle). In this case,
+  // this item through an API such as remove(itemHandle). In this case,
   // it is unsafe to replace the old item with a new one, so we should
   // also abort.
   if (!accessContainer_->replaceIf(oldItem, *newItemHdl, itemMovingPredicate)) {
@@ -1322,7 +1322,7 @@ bool CacheAllocator<CacheTrait>::shouldWriteToNvmCacheExclusive(
 }
 
 template <typename CacheTrait>
-typename CacheAllocator<CacheTrait>::ItemHandle
+typename CacheAllocator<CacheTrait>::WriteHandle
 CacheAllocator<CacheTrait>::advanceIteratorAndTryEvictRegularItem(
     MMContainer& mmContainer, EvictionIterator& itr) {
   // we should flush this to nvmcache if it is not already present in nvmcache
@@ -1337,7 +1337,7 @@ CacheAllocator<CacheTrait>::advanceIteratorAndTryEvictRegularItem(
   if (evictToNvmCache && !token.isValid()) {
     ++itr;
     stats_.evictFailConcurrentFill.inc();
-    return ItemHandle{};
+    return WriteHandle{};
   }
 
   // If there are other accessors, we should abort. Acquire a handle here since
@@ -1364,7 +1364,7 @@ CacheAllocator<CacheTrait>::advanceIteratorAndTryEvictRegularItem(
   // is set. Iterator was already advance by the remove call above.
   if (evictHandle->isMoving()) {
     stats_.evictFailMove.inc();
-    return ItemHandle{};
+    return WriteHandle{};
   }
 
   // Invalidate iterator since later on if we are not evicting this
@@ -1388,7 +1388,7 @@ CacheAllocator<CacheTrait>::advanceIteratorAndTryEvictRegularItem(
 }
 
 template <typename CacheTrait>
-typename CacheAllocator<CacheTrait>::ItemHandle
+typename CacheAllocator<CacheTrait>::WriteHandle
 CacheAllocator<CacheTrait>::advanceIteratorAndTryEvictChainedItem(
     EvictionIterator& itr) {
   XDCHECK(itr->isChainedItem());
@@ -1410,7 +1410,7 @@ CacheAllocator<CacheTrait>::advanceIteratorAndTryEvictChainedItem(
   // if token is invalid, return. iterator is already advanced.
   if (evictToNvmCache && !token.isValid()) {
     stats_.evictFailConcurrentFill.inc();
-    return ItemHandle{};
+    return WriteHandle{};
   }
 
   // check if the parent exists in the hashtable and refcount is drained.
@@ -1446,7 +1446,7 @@ CacheAllocator<CacheTrait>::advanceIteratorAndTryEvictChainedItem(
   // here since moving bit is set.
   if (parentHandle->isMoving()) {
     stats_.evictFailParentMove.inc();
-    return ItemHandle{};
+    return WriteHandle{};
   }
 
   if (evictToNvmCache && shouldWriteToNvmCacheExclusive(*parentHandle)) {
@@ -1695,7 +1695,7 @@ CacheAllocator<CacheTrait>::inspectCache(typename Item::Key key) {
 // CacheAllocator. Hence the sprinkling of UNLIKELY/LIKELY to tell the
 // compiler which executions we don't want to optimize on.
 template <typename CacheTrait>
-typename CacheAllocator<CacheTrait>::ItemHandle
+typename CacheAllocator<CacheTrait>::WriteHandle
 CacheAllocator<CacheTrait>::findFastInternal(typename Item::Key key,
                                              AccessMode mode) {
   auto handle = findInternal(key);
@@ -1711,7 +1711,7 @@ CacheAllocator<CacheTrait>::findFastInternal(typename Item::Key key,
 }
 
 template <typename CacheTrait>
-typename CacheAllocator<CacheTrait>::ItemHandle
+typename CacheAllocator<CacheTrait>::WriteHandle
 CacheAllocator<CacheTrait>::findFastImpl(typename Item::Key key,
                                          AccessMode mode) {
   auto handle = findFastInternal(key, mode);
@@ -1751,7 +1751,7 @@ CacheAllocator<CacheTrait>::findFastToWrite(typename Item::Key key,
 }
 
 template <typename CacheTrait>
-typename CacheAllocator<CacheTrait>::ItemHandle
+typename CacheAllocator<CacheTrait>::WriteHandle
 CacheAllocator<CacheTrait>::findImpl(typename Item::Key key, AccessMode mode) {
   auto handle = findFastInternal(key, mode);
 
@@ -1765,7 +1765,7 @@ CacheAllocator<CacheTrait>::findImpl(typename Item::Key key, AccessMode mode) {
         eventTracker->record(AllocatorApiEvent::FIND, key,
                              AllocatorApiResult::NOT_FOUND);
       }
-      ItemHandle ret;
+      WriteHandle ret;
       ret.markExpired();
       return ret;
     }
@@ -1920,8 +1920,8 @@ folly::IOBuf CacheAllocator<CacheTrait>::convertToIOBufT(Handle& handle) {
   ConvertChainedItem converter;
 
   // based on current refcount and threshold from config
-  // determine to use a new ItemHandle for each chain items
-  // or use shared ItemHandle for all chain items
+  // determine to use a new Item Handle for each chain items
+  // or use shared Item Handle for all chain items
   if (item->getRefCount() > config_.thresholdForConvertingToIOBuf) {
     auto sharedHdl = std::make_shared<Handle>(std::move(handle));
 
@@ -2568,7 +2568,7 @@ CacheAllocator<CacheTrait>::allocateNewItemForOldItem(const Item& oldItem) {
 
 template <typename CacheTrait>
 bool CacheAllocator<CacheTrait>::tryMovingForSlabRelease(
-    Item& oldItem, ItemHandle& newItemHdl) {
+    Item& oldItem, WriteHandle& newItemHdl) {
   // By holding onto a user-level synchronization object, we ensure moving
   // a regular item or chained item is synchronized with any potential
   // user-side mutation.
@@ -2685,12 +2685,12 @@ void CacheAllocator<CacheTrait>::evictForSlabRelease(
 }
 
 template <typename CacheTrait>
-typename CacheAllocator<CacheTrait>::ItemHandle
+typename CacheAllocator<CacheTrait>::WriteHandle
 CacheAllocator<CacheTrait>::evictNormalItemForSlabRelease(Item& item) {
   XDCHECK(item.isMoving());
 
   if (item.isOnlyMoving()) {
-    return ItemHandle{};
+    return WriteHandle{};
   }
 
   auto predicate = [](const Item& it) { return it.getRefCount() == 0; };
@@ -2723,7 +2723,7 @@ CacheAllocator<CacheTrait>::evictNormalItemForSlabRelease(Item& item) {
 }
 
 template <typename CacheTrait>
-typename CacheAllocator<CacheTrait>::ItemHandle
+typename CacheAllocator<CacheTrait>::WriteHandle
 CacheAllocator<CacheTrait>::evictChainedItemForSlabRelease(ChainedItem& child) {
   XDCHECK(child.isMoving());
 
diff --git a/cachelib/allocator/CacheAllocator.h b/cachelib/allocator/CacheAllocator.h
@@ -186,10 +186,11 @@ class CacheAllocator : public CacheBase {
 
   // the holder for the item when we hand it to the caller. This ensures
   // that the reference count is maintained when the caller is done with the
-  // item. The ItemHandle provides a getMemory() and getKey() interface. The
-  // caller is free to use the result of these two as long as the handle is
-  // active/alive. Using the result of the above interfaces after destroying
-  // the ItemHandle is UB. The ItemHandle safely wraps a pointer to the Item.
+  // item. The ReadHandle/WriteHandle provides a getMemory() and getKey()
+  // interface. The caller is free to use the result of these two as long as the
+  // handle is active/alive. Using the result of the above interfaces after
+  // destroying the ReadHandle/WriteHandle is UB. The ReadHandle/WriteHandle
+  // safely wraps a pointer to the "const Item"/"Item".
   using ReadHandle = typename Item::ReadHandle;
   using WriteHandle = typename Item::WriteHandle;
   using ItemHandle = WriteHandle;
@@ -564,7 +565,7 @@ class CacheAllocator : public CacheBase {
   // removes the allocation corresponding to the key, if present in the hash
   // table. The key will not be accessible through find() after this returns
   // success. The allocation for the key will be recycled once all active
-  // ItemHandles are released.
+  // Item handles are released.
   //
   // @param key   the key for the allocation.
   // @return      kSuccess if the key exists and was successfully removed.
@@ -1272,18 +1273,18 @@ class CacheAllocator : public CacheBase {
 
   // acquires an handle on the item. returns an empty handle if it is null.
   // @param it    pointer to an item
-  // @return ItemHandle   return a handle to this item
+  // @return WriteHandle   return a handle to this item
   // @throw std::overflow_error is the maximum item refcount is execeeded by
   //        creating this item handle.
-  ItemHandle acquire(Item* it);
+  WriteHandle acquire(Item* it);
 
   // creates an item handle with wait context.
-  ItemHandle createNvmCacheFillHandle() { return ItemHandle{*this}; }
+  WriteHandle createNvmCacheFillHandle() { return WriteHandle{*this}; }
 
   // acquires the wait context for the handle. This is used by NvmCache to
   // maintain a list of waiters
   std::shared_ptr<WaitContext<ReadHandle>> getWaitContext(
-      ItemHandle& hdl) const {
+      ReadHandle& hdl) const {
     return hdl.getItemWaitContext();
   }
 
@@ -1386,7 +1387,7 @@ class CacheAllocator : public CacheBase {
   //
   // @throw std::overflow_error is the maximum item refcount is execeeded by
   //        creating this item handle.
-  ItemHandle findInternal(Key key) {
+  WriteHandle findInternal(Key key) {
     // Note: this method can not be const because we need  a non-const
     // reference to create the ItemReleaser.
     return accessContainer_->find(key);
@@ -1401,7 +1402,7 @@ class CacheAllocator : public CacheBase {
   //
   // @return      the handle for the item or a handle to nullptr if the key does
   //              not exist.
-  FOLLY_ALWAYS_INLINE ItemHandle findFastInternal(Key key, AccessMode mode);
+  FOLLY_ALWAYS_INLINE WriteHandle findFastInternal(Key key, AccessMode mode);
 
   // look up an item by its key across the nvm cache as well if enabled.
   //
@@ -1411,7 +1412,7 @@ class CacheAllocator : public CacheBase {
   //
   // @return      the handle for the item or a handle to nullptr if the key does
   //              not exist.
-  FOLLY_ALWAYS_INLINE ItemHandle findImpl(Key key, AccessMode mode);
+  FOLLY_ALWAYS_INLINE WriteHandle findImpl(Key key, AccessMode mode);
 
   // look up an item by its key. This ignores the nvm cache and only does RAM
   // lookup.
@@ -1422,7 +1423,7 @@ class CacheAllocator : public CacheBase {
   //
   // @return      the handle for the item or a handle to nullptr if the key does
   //              not exist.
-  FOLLY_ALWAYS_INLINE ItemHandle findFastImpl(Key key, AccessMode mode);
+  FOLLY_ALWAYS_INLINE WriteHandle findFastImpl(Key key, AccessMode mode);
 
   // Moves a regular item to a different slab. This should only be used during
   // slab release after the item's moving bit has been set. The user supplied
@@ -1434,7 +1435,7 @@ class CacheAllocator : public CacheBase {
   //
   // @return true  If the move was completed, and the containers were updated
   //               successfully.
-  bool moveRegularItem(Item& oldItem, ItemHandle& newItemHdl);
+  bool moveRegularItem(Item& oldItem, WriteHandle& newItemHdl);
 
   // template class for viewAsChainedAllocs that takes either ReadHandle or
   // WriteHandle
@@ -1586,8 +1587,8 @@ class CacheAllocator : public CacheBase {
   //
   // @return  valid handle to regular item on success. This will be the last
   //          handle to the item. On failure an empty handle.
-  ItemHandle advanceIteratorAndTryEvictRegularItem(MMContainer& mmContainer,
-                                                   EvictionIterator& itr);
+  WriteHandle advanceIteratorAndTryEvictRegularItem(MMContainer& mmContainer,
+                                                    EvictionIterator& itr);
 
   // Advance the current iterator and try to evict a chained item
   // Iterator may also be reset during the course of this function
@@ -1596,7 +1597,7 @@ class CacheAllocator : public CacheBase {
   //
   // @return  valid handle to the parent item on success. This will be the last
   //          handle to the item
-  ItemHandle advanceIteratorAndTryEvictChainedItem(EvictionIterator& itr);
+  WriteHandle advanceIteratorAndTryEvictChainedItem(EvictionIterator& itr);
 
   // Deserializer CacheAllocatorMetadata and verify the version
   //
@@ -1707,7 +1708,7 @@ class CacheAllocator : public CacheBase {
   //
   // @return    true  if the item has been moved
   //            false if we have exhausted moving attempts
-  bool tryMovingForSlabRelease(Item& item, ItemHandle& newItemHdl);
+  bool tryMovingForSlabRelease(Item& item, WriteHandle& newItemHdl);
 
   // Evict an item from access and mm containers and
   // ensure it is safe for freeing.
@@ -1723,14 +1724,14 @@ class CacheAllocator : public CacheBase {
   //
   // @return last handle for corresponding to item on success. empty handle on
   // failure. caller can retry if needed.
-  ItemHandle evictNormalItemForSlabRelease(Item& item);
+  WriteHandle evictNormalItemForSlabRelease(Item& item);
 
   // Helper function to evict a child item for slab release
   // As a side effect, the parent item is also evicted
   //
   // @return  last handle to the parent item of the child on success. empty
   // handle on failure. caller can retry.
-  ItemHandle evictChainedItemForSlabRelease(ChainedItem& item);
+  WriteHandle evictChainedItemForSlabRelease(ChainedItem& item);
 
   // Helper function to remove a item if expired.
   //
