Shorten critical section in findEviction

cachelib/allocator/CacheAllocator.h

@@ -1455,7 +1455,7 @@ class CacheAllocator : public CacheBase {
   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
+  // slab release after the item's exclusive bit has been set. The user supplied
   // callback is responsible for copying the contents and fixing the semantics
   // of chained item.
   //
@@ -1478,7 +1478,7 @@ class CacheAllocator : public CacheBase {
   folly::IOBuf convertToIOBufT(Handle& handle);
 
   // Moves a chained 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
+  // slab release after the item's exclusive bit has been set. The user supplied
   // callback is responsible for copying the contents and fixing the semantics
   // of chained item.
   //
@@ -1609,25 +1609,6 @@ class CacheAllocator : public CacheBase {
 
   using EvictionIterator = typename MMContainer::Iterator;
 
-  // Advance the current iterator and try to evict a regular item
-  //
-  // @param  mmContainer  the container to look for evictions.
-  // @param  itr          iterator holding the item
-  //
-  // @return  valid handle to regular item on success. This will be the last
-  //          handle to the item. On failure an empty handle.
-  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
-  //
-  // @param  itr          iterator holding the item
-  //
-  // @return  valid handle to the parent item on success. This will be the last
-  //          handle to the item
-  WriteHandle advanceIteratorAndTryEvictChainedItem(EvictionIterator& itr);
-
   // Deserializer CacheAllocatorMetadata and verify the version
   //
   // @param  deserializer   Deserializer object
@@ -1703,9 +1684,9 @@ class CacheAllocator : public CacheBase {
 
   // @return  true when successfully marked as moving,
   //          fasle when this item has already been freed
-  bool markMovingForSlabRelease(const SlabReleaseContext& ctx,
-                                void* alloc,
-                                util::Throttler& throttler);
+  bool markExclusiveForSlabRelease(const SlabReleaseContext& ctx,
+                                   void* alloc,
+                                   util::Throttler& throttler);
 
   // "Move" (by copying) the content in this item to another memory
   // location by invoking the move callback.
@@ -1745,7 +1726,7 @@ class CacheAllocator : public CacheBase {
   //
   // @return last handle for corresponding to item on success. empty handle on
   // failure. caller can retry if needed.
-  WriteHandle evictNormalItemForSlabRelease(Item& item);
+  WriteHandle evictNormalItem(Item& item);
 
   // Helper function to evict a child item for slab release
   // As a side effect, the parent item is also evicted
@@ -1874,20 +1855,16 @@ class CacheAllocator : public CacheBase {
   std::optional<bool> saveNvmCache();
   void saveRamCache();
 
-  static bool itemMovingPredicate(const Item& item) {
+  static bool itemExlusivePredicate(const Item& item) {
     return item.getRefCount() == 0;
   }
 
-  static bool itemEvictionPredicate(const Item& item) {
-    return item.getRefCount() == 0 && !item.isMoving();
-  }
-
   static bool itemExpiryPredicate(const Item& item) {
     return item.getRefCount() == 1 && item.isExpired();
   }
 
   static bool parentEvictForSlabReleasePredicate(const Item& item) {
-    return item.getRefCount() == 1 && !item.isMoving();
+    return item.getRefCount() == 1 && !item.isExclusive();
   }
 
   std::unique_ptr<Deserializer> createDeserializer();

cachelib/allocator/CacheItem-inl.h

@@ -141,12 +141,13 @@ std::string CacheItem<CacheTrait>::toString() const {
     return folly::sformat(
         "item: "
         "memory={}:raw-ref={}:size={}:key={}:hex-key={}:"
-        "isInMMContainer={}:isAccessible={}:isMoving={}:references={}:ctime={}:"
+        "isInMMContainer={}:isAccessible={}:isExclusive={}:references={}:ctime="
+        "{}:"
         "expTime={}:updateTime={}:isNvmClean={}:isNvmEvicted={}:hasChainedItem="
         "{}",
         this, getRefCountAndFlagsRaw(), getSize(),
         folly::humanify(getKey().str()), folly::hexlify(getKey()),
-        isInMMContainer(), isAccessible(), isMoving(), getRefCount(),
+        isInMMContainer(), isAccessible(), isExclusive(), getRefCount(),
         getCreationTime(), getExpiryTime(), getLastAccessTime(), isNvmClean(),
         isNvmEvicted(), hasChainedItem());
   }
@@ -178,11 +179,6 @@ bool CacheItem<CacheTrait>::isDrained() const noexcept {
   return ref_.isDrained();
 }
 
-template <typename CacheTrait>
-bool CacheItem<CacheTrait>::isExclusive() const noexcept {
-  return ref_.isExclusive();
-}
-
 template <typename CacheTrait>
 void CacheItem<CacheTrait>::markAccessible() noexcept {
   ref_.markAccessible();
@@ -214,23 +210,23 @@ bool CacheItem<CacheTrait>::isInMMContainer() const noexcept {
 }
 
 template <typename CacheTrait>
-bool CacheItem<CacheTrait>::markMoving() noexcept {
-  return ref_.markMoving();
+bool CacheItem<CacheTrait>::markExclusive() noexcept {
+  return ref_.markExclusive();
 }
 
 template <typename CacheTrait>
-void CacheItem<CacheTrait>::unmarkMoving() noexcept {
-  ref_.unmarkMoving();
+RefcountWithFlags::Value CacheItem<CacheTrait>::unmarkExclusive() noexcept {
+  return ref_.unmarkExclusive();
 }
 
 template <typename CacheTrait>
-bool CacheItem<CacheTrait>::isMoving() const noexcept {
-  return ref_.isMoving();
+bool CacheItem<CacheTrait>::isExclusive() const noexcept {
+  return ref_.isExclusive();
 }
 
 template <typename CacheTrait>
-bool CacheItem<CacheTrait>::isOnlyMoving() const noexcept {
-  return ref_.isOnlyMoving();
+bool CacheItem<CacheTrait>::isOnlyExclusive() const noexcept {
+  return ref_.isOnlyExclusive();
 }
 
 template <typename CacheTrait>
@@ -332,7 +328,7 @@ bool CacheItem<CacheTrait>::updateExpiryTime(uint32_t expiryTimeSecs) noexcept {
   // check for moving to make sure we are not updating the expiry time while at
   // the same time re-allocating the item with the old state of the expiry time
   // in moveRegularItem(). See D6852328
-  if (isMoving() || !isInMMContainer() || isChainedItem()) {
+  if (isExclusive() || !isInMMContainer() || isChainedItem()) {
     return false;
   }
   // attempt to atomically update the value of expiryTime
@@ -448,10 +444,11 @@ std::string CacheChainedItem<CacheTrait>::toString() const {
   return folly::sformat(
       "chained item: "
       "memory={}:raw-ref={}:size={}:parent-compressed-ptr={}:"
-      "isInMMContainer={}:isAccessible={}:isMoving={}:references={}:ctime={}:"
+      "isInMMContainer={}:isAccessible={}:isExclusive={}:references={}:ctime={}"
+      ":"
       "expTime={}:updateTime={}",
       this, Item::getRefCountAndFlagsRaw(), Item::getSize(), cPtr.getRaw(),
-      Item::isInMMContainer(), Item::isAccessible(), Item::isMoving(),
+      Item::isInMMContainer(), Item::isAccessible(), Item::isExclusive(),
       Item::getRefCount(), Item::getCreationTime(), Item::getExpiryTime(),
       Item::getLastAccessTime());
 }

cachelib/allocator/CacheItem.h

@@ -243,23 +243,23 @@ class CACHELIB_PACKED_ATTR CacheItem {
    *
    * This API will only succeed when an item is a regular item, and user
    * has already inserted it into the cache (via @insert or @insertOrReplace).
-   * In addition, the item cannot be in a "moving" state.
+   * In addition, the item cannot be in a "exclusive" state.
    *
    * @param expiryTime the expiryTime value to update to
    *
    * @return boolean indicating whether expiry time was successfully updated
-   *         false when item is not linked in cache, or in moving state, or a
+   *         false when item is not linked in cache, or in exclusive state, or a
    *         chained item
    */
   bool updateExpiryTime(uint32_t expiryTimeSecs) noexcept;
 
   // Same as @updateExpiryTime, but sets expiry time to @ttl seconds from now.
   // It has the same restrictions as @updateExpiryTime. An item must be a
-  // regular item and is part of the cache and NOT in the moving state.
+  // regular item and is part of the cache and NOT in the exclusive state.
   //
   // @param ttl   TTL (from now)
   // @return boolean indicating whether expiry time was successfully updated
-  //         false when item is not linked in cache, or in moving state, or a
+  //         false when item is not linked in cache, or in exclusive state, or a
   //         chained item
   bool extendTTL(std::chrono::seconds ttl) noexcept;
 
@@ -317,13 +317,9 @@ class CACHELIB_PACKED_ATTR CacheItem {
   // Whether or not an item is completely drained of all references including
   // the internal ones. This means there is no access refcount bits and zero
   // admin bits set. I.e. refcount is 0 and the item is not linked, accessible,
-  // nor moving
+  // nor exclusive
   bool isDrained() const noexcept;
 
-  // Whether or not we hold the last exclusive access to this item
-  // Refcount is 1 and the item is not linked, accessible, nor moving
-  bool isExclusive() const noexcept;
-
   /**
    * The following three functions correspond to the state of the allocation
    * in the memory management container. This is protected by the
@@ -346,20 +342,20 @@ class CACHELIB_PACKED_ATTR CacheItem {
   /**
    * The following two functions corresond to whether or not an item is
    * currently in the process of being moved. This happens during a slab
-   * rebalance or resize operation.
+   * rebalance, eviction or resize operation.
    *
-   * An item can only be marked moving when `isInMMContainer` returns true.
+   * An item can only be marked exclusive when `isInMMContainer` returns true.
    * This operation is atomic.
    *
-   * User can also query if an item "isOnlyMoving". This returns true only
-   * if the refcount is 0 and only the moving bit is set.
+   * User can also query if an item "isOnlyExclusive". This returns true only
+   * if the refcount is 0 and only the exclusive bit is set.
    *
-   * Unmarking moving does not depend on `isInMMContainer`
+   * Unmarking exclusive does not depend on `isInMMContainer`
    */
-  bool markMoving() noexcept;
-  void unmarkMoving() noexcept;
-  bool isMoving() const noexcept;
-  bool isOnlyMoving() const noexcept;
+  bool markExclusive() noexcept;
+  RefcountWithFlags::Value unmarkExclusive() noexcept;
+  bool isExclusive() const noexcept;
+  bool isOnlyExclusive() const noexcept;
 
   /**
    * Item cannot be marked both chained allocation and

cachelib/allocator/MM2Q-inl.h

@@ -241,29 +241,9 @@ bool MM2Q::Container<T, HookPtr>::add(T& node) noexcept {
 }
 
 template <typename T, MM2Q::Hook<T> T::*HookPtr>
-typename MM2Q::Container<T, HookPtr>::Iterator
-MM2Q::Container<T, HookPtr>::getEvictionIterator() const noexcept {
-  // we cannot use combined critical sections with folly::DistributedMutex here
-  // because the lock is held for the lifetime of the eviction iterator.  In
-  // other words, the abstraction of the iterator just does not lend itself well
-  // to combinable critical sections as the user can hold the lock for an
-  // arbitrary amount of time outside a lambda-friendly piece of code (eg. they
-  // can return the iterator from functions, pass it to functions, etc)
-  //
-  // it would be theoretically possible to refactor this interface into
-  // something like the following to allow combining
-  //
-  //    mm2q.withEvictionIterator([&](auto iterator) {
-  //      // user code
-  //    });
-  //
-  // at the time of writing it is unclear if the gains from combining are
-  // reasonable justification for the codemod required to achieve combinability
-  // as we don't expect this critical section to be the hotspot in user code.
-  // This is however subject to change at some time in the future as and when
-  // this assertion becomes false.
-  LockHolder l(*lruMutex_);
-  return Iterator{std::move(l), lru_.rbegin()};
+template <typename F>
+void MM2Q::Container<T, HookPtr>::withEvictionIterator(F&& fun) {
+  lruMutex_->lock_combine([this, &fun]() { fun(Iterator{lru_.rbegin()}); });
 }
 
 template <typename T, MM2Q::Hook<T> T::*HookPtr>
@@ -460,10 +440,5 @@ void MM2Q::Container<T, HookPtr>::reconfigureLocked(const Time& currTime) {
   lruRefreshTime_.store(lruRefreshTime, std::memory_order_relaxed);
 }
 
-// Iterator Context Implementation
-template <typename T, MM2Q::Hook<T> T::*HookPtr>
-MM2Q::Container<T, HookPtr>::Iterator::Iterator(
-    LockHolder l, const typename LruList::Iterator& iter) noexcept
-    : LruList::Iterator(iter), l_(std::move(l)) {}
 } // namespace cachelib
 } // namespace facebook

cachelib/allocator/MM2Q.h

@@ -347,48 +347,7 @@ class MM2Q {
     Container(const Container&) = delete;
     Container& operator=(const Container&) = delete;
 
-    // context for iterating the MM container. At any given point of time,
-    // there can be only one iterator active since we need to lock the LRU for
-    // iteration. we can support multiple iterators at same time, by using a
-    // shared ptr in the context for the lock holder in the future.
-    class Iterator : public LruList::Iterator {
-     public:
-      // noncopyable but movable.
-      Iterator(const Iterator&) = delete;
-      Iterator& operator=(const Iterator&) = delete;
-
-      Iterator(Iterator&&) noexcept = default;
-
-      // 1. Invalidate this iterator
-      // 2. Unlock
-      void destroy() {
-        LruList::Iterator::reset();
-        if (l_.owns_lock()) {
-          l_.unlock();
-        }
-      }
-
-      // Reset this iterator to the beginning
-      void resetToBegin() {
-        if (!l_.owns_lock()) {
-          l_.lock();
-        }
-        LruList::Iterator::resetToBegin();
-      }
-
-     private:
-      // private because it's easy to misuse and cause deadlock for MM2Q
-      Iterator& operator=(Iterator&&) noexcept = default;
-
-      // create an lru iterator with the lock being held.
-      Iterator(LockHolder l, const typename LruList::Iterator& iter) noexcept;
-
-      // only the container can create iterators
-      friend Container<T, HookPtr>;
-
-      // lock protecting the validity of the iterator
-      LockHolder l_;
-    };
+    using Iterator = typename LruList::Iterator;
 
     // records the information that the node was accessed. This could bump up
     // the node to the head of the lru depending on the time when the node was
@@ -442,10 +401,10 @@ class MM2Q {
     //               source node already existed.
     bool replace(T& oldNode, T& newNode) noexcept;
 
-    // Obtain an iterator that start from the tail and can be used
-    // to search for evictions. This iterator holds a lock to this
-    // container and only one such iterator can exist at a time
-    Iterator getEvictionIterator() const noexcept;
+    // Execute provided function under container lock. Function gets
+    // iterator passed as parameter.
+    template <typename F>
+    void withEvictionIterator(F&& f);
 
     // get the current config as a copy
     Config getConfig() const;

cachelib/allocator/MMLru-inl.h

@@ -212,10 +212,9 @@ bool MMLru::Container<T, HookPtr>::add(T& node) noexcept {
 }
 
 template <typename T, MMLru::Hook<T> T::*HookPtr>
-typename MMLru::Container<T, HookPtr>::Iterator
-MMLru::Container<T, HookPtr>::getEvictionIterator() const noexcept {
-  LockHolder l(*lruMutex_);
-  return Iterator{std::move(l), lru_.rbegin()};
+template <typename F>
+void MMLru::Container<T, HookPtr>::withEvictionIterator(F&& fun) {
+  lruMutex_->lock_combine([this, &fun]() { fun(Iterator{lru_.rbegin()}); });
 }
 
 template <typename T, MMLru::Hook<T> T::*HookPtr>
@@ -358,10 +357,5 @@ void MMLru::Container<T, HookPtr>::reconfigureLocked(const Time& currTime) {
   lruRefreshTime_.store(lruRefreshTime, std::memory_order_relaxed);
 }
 
-// Iterator Context Implementation
-template <typename T, MMLru::Hook<T> T::*HookPtr>
-MMLru::Container<T, HookPtr>::Iterator::Iterator(
-    LockHolder l, const typename LruList::Iterator& iter) noexcept
-    : LruList::Iterator(iter), l_(std::move(l)) {}
 } // namespace cachelib
 } // namespace facebook