critical section inside combined_lock

Author: igchor

cachelib/allocator/CacheAllocator-inl.h

@@ -1482,27 +1482,40 @@ CacheAllocator<CacheTrait>::findEviction(TierId tid, PoolId pid, ClassId cid) {
   // Keep searching for a candidate until we were able to evict it
   // or until the search limit has been exhausted
   unsigned int searchTries = 0;
-  auto itr = mmContainer.getEvictionIterator();
   while ((config_.evictionSearchTries == 0 ||
-          config_.evictionSearchTries > searchTries) &&
-         itr) {
+          config_.evictionSearchTries > searchTries)) {
     ++searchTries;
     (*stats_.evictionAttempts)[pid][cid].inc();
 
-    Item* toRecycle = itr.get();
+    Item* toRecycle = nullptr;
+    Item* candidate = nullptr;
 
-    Item* candidate =
-        toRecycle->isChainedItem()
-            ? &toRecycle->asChainedItem().getParentItem(compressor_)
-            : toRecycle;
+    mmContainer.withEvictionIterator([this, &candidate, &toRecycle, &searchTries](auto &&itr){
+      while ((config_.evictionSearchTries == 0 ||
+          config_.evictionSearchTries > searchTries) && itr) {
+        ++searchTries;
 
-    // make sure no other thead is evicting the item
-    if (candidate->getRefCount() != 0 || !candidate->markMoving()) {
-      ++itr;
+        auto *toRecycle_ = itr.get();
+        auto *candidate_ = toRecycle_->isChainedItem()
+            ? &toRecycle_->asChainedItem().getParentItem(compressor_)
+            : toRecycle_;
+
+        // make sure no other thead is evicting the item
+        if (candidate_->getRefCount() == 0 && candidate_->markMoving()) {
+          toRecycle = toRecycle_;
+          candidate = candidate_;
+          return;
+        }
+
+        ++itr;
+      }
+    });
+
+    if (!toRecycle)
       continue;
-    }
-  
-    itr.destroy();
+
+    XDCHECK(toRecycle);
+    XDCHECK(candidate);
 
     // for chained items, the ownership of the parent can change. We try to
     // evict what we think as parent and see if the eviction of parent
@@ -1563,8 +1576,6 @@ CacheAllocator<CacheTrait>::findEviction(TierId tid, PoolId pid, ClassId cid) {
         return toRecycle;
       }
     }
-
-    itr.resetToBegin();
   }
   return nullptr;
 }

cachelib/allocator/MM2Q-inl.h

@@ -250,22 +250,21 @@ MM2Q::Container<T, HookPtr>::getEvictionIterator() const noexcept {
   // 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.
+  // to get advantage of combining, use withEvictionIterator
   LockHolder l(*lruMutex_);
   return Iterator{std::move(l), lru_.rbegin()};
 }
 
+template <typename T, MM2Q::Hook<T> T::*HookPtr>
+template <typename F>
+void
+MM2Q::Container<T, HookPtr>::withEvictionIterator(F&& fun) {
+  lruMutex_->lock_combine([this, &fun]() {
+    fun(Iterator{LockHolder{}, lru_.rbegin()});
+  });
+}
+
+
 template <typename T, MM2Q::Hook<T> T::*HookPtr>
 void MM2Q::Container<T, HookPtr>::removeLocked(T& node,
                                                bool doRebalance) noexcept {

cachelib/allocator/MM2Q.h

@@ -447,6 +447,11 @@ class MM2Q {
     // 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

@@ -218,6 +218,15 @@ MMLru::Container<T, HookPtr>::getEvictionIterator() const noexcept {
   return Iterator{std::move(l), lru_.rbegin()};
 }
 
+template <typename T, MMLru::Hook<T> T::*HookPtr>
+template <typename F>
+void
+MMLru::Container<T, HookPtr>::withEvictionIterator(F&& fun) {
+  lruMutex_->lock_combine([this, &fun]() {
+    fun(Iterator{LockHolder{}, lru_.rbegin()});
+  });
+}
+
 template <typename T, MMLru::Hook<T> T::*HookPtr>
 void MMLru::Container<T, HookPtr>::ensureNotInsertionPoint(T& node) noexcept {
   // If we are removing the insertion point node, grow tail before we remove

cachelib/allocator/MMLru.h

@@ -332,6 +332,11 @@ class MMLru {
     // 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 copy of current config
     Config getConfig() const;
 

cachelib/allocator/MMTinyLFU-inl.h

@@ -220,6 +220,15 @@ MMTinyLFU::Container<T, HookPtr>::getEvictionIterator() const noexcept {
   return Iterator{std::move(l), *this};
 }
 
+template <typename T, MMTinyLFU::Hook<T> T::*HookPtr>
+template <typename F>
+void
+MMTinyLFU::Container<T, HookPtr>::withEvictionIterator(F&& fun) {
+  LockHolder l(lruMutex_);
+  fun(Iterator{LockHolder{}, *this});
+}
+
+
 template <typename T, MMTinyLFU::Hook<T> T::*HookPtr>
 void MMTinyLFU::Container<T, HookPtr>::removeLocked(T& node) noexcept {
   if (isTiny(node)) {

cachelib/allocator/MMTinyLFU.h

@@ -491,6 +491,11 @@ class MMTinyLFU {
     // 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);
+
     // for saving the state of the lru
     //
     // precondition:  serialization must happen without any reader or writer