Remove parameter for markMoving()

initial sync logic

just use parent

in progress - only mark parent as moving

fix chained items

fix moving cleanup on failure

currently fails asserts in move chained with sync

sync on parent

Author: igchor

cachelib/allocator/CacheAllocator-inl.h

@@ -1544,6 +1544,25 @@ class CacheAllocator : public CacheBase {
   WriteHandle allocateChainedItemInternal(const ReadHandle& parent,
                                           uint32_t size);
 
+  // Allocate a chained item to a specific tier
+  //
+  // The resulting chained item does not have a parent item and
+  // will be freed once the handle is dropped
+  //
+  // The parent handle parameter here is mainly used to find the
+  // correct pool to allocate memory for this chained item
+  //
+  // @param parent    handle to the cache item
+  // @param size      the size for the chained allocation
+  // @param tid       the tier to allocate on
+  //
+  // @return    handle to the chained allocation
+  // @throw     std::invalid_argument if the size requested is invalid or
+  //            if the item is invalid
+  WriteHandle allocateChainedItemInternalTier(const ReadHandle& parent,
+                                          uint32_t size,
+                                          TierId tid);
+
   // Given an item and its parentKey, validate that the parentKey
   // corresponds to an item that's the parent of the supplied item.
   //
@@ -1620,6 +1639,16 @@ class CacheAllocator : public CacheBase {
   // @return true  If the move was completed, and the containers were updated
   //               successfully.
   bool moveRegularItemWithSync(Item& oldItem, WriteHandle& newItemHdl);
+  
+  // Moves a chained item to a different memory tier.
+  //
+  // @param oldItem     Reference to the item being moved
+  // @param newItemHdl  Reference to the handle of the new item being moved into
+  // @param parentHandle Reference to the handle of the parent item
+  //
+  // @return true  If the move was completed, and the containers were updated
+  //               successfully.
+  bool moveChainedItemWithSync(ChainedItem& oldItem, WriteHandle& newItemHdl, WriteHandle& parentHandle);
 
   // Moves a regular item to a different slab. This should only be used during
   // slab release after the item's exclusive bit has been set. The user supplied
@@ -1689,6 +1718,9 @@ class CacheAllocator : public CacheBase {
                                        WriteHandle newItemHdl,
                                        const Item& parent);
 
+  void replaceChainedItemLockedForMoving(Item& oldItem,
+                                       WriteHandle& newItemHdl,
+                                       const Item& parent);
   // Insert an item into MM container. The caller must hold a valid handle for
   // the item.
   //
@@ -1979,7 +2011,7 @@ auto& mmContainer = getMMContainer(tid, pid, cid);
           throw std::runtime_error("Not supported for chained items");
         }
 
-        if (candidate->markMoving(true)) {
+        if (candidate->markMoving()) {
           mmContainer.remove(itr);
           candidates.push_back(candidate);
         } else {
@@ -2052,7 +2084,7 @@ auto& mmContainer = getMMContainer(tid, pid, cid);
 
         // TODO: only allow it for read-only items?
         // or implement mvcc
-        if (candidate->markMoving(true)) {
+        if (candidate->markMoving()) {
           // promotions should rarely fail since we already marked moving
           mmContainer.remove(itr);
           candidates.push_back(candidate);

cachelib/allocator/CacheAllocator.h

@@ -238,8 +238,8 @@ bool CacheItem<CacheTrait>::markForEvictionWhenMoving() {
 }
 
 template <typename CacheTrait>
-bool CacheItem<CacheTrait>::markMoving(bool failIfRefNotZero) {
-  return ref_.markMoving(failIfRefNotZero);
+bool CacheItem<CacheTrait>::markMoving() {
+  return ref_.markMoving();
 }
 
 template <typename CacheTrait>

cachelib/allocator/CacheItem-inl.h

@@ -381,7 +381,7 @@ class CACHELIB_PACKED_ATTR CacheItem {
    * Unmarking moving will also return the refcount at the moment of
    * unmarking.
    */
-  bool markMoving(bool failIfRefNotZero);
+  bool markMoving();
   RefcountWithFlags::Value unmarkMoving() noexcept;
   bool isMoving() const noexcept;
   bool isOnlyMoving() const noexcept;

cachelib/allocator/CacheItem.h

@@ -320,15 +320,23 @@ class FOLLY_PACK_ATTR RefcountWithFlags {
    *
    * Unmarking moving does not depend on `isInMMContainer`
    */
-  bool markMoving(bool failIfRefNotZero) {
-    auto predicate = [failIfRefNotZero](const Value curValue) {
+  bool markMoving() {
+    auto predicate = [](const Value curValue) {
       Value conditionBitMask = getAdminRef<kLinked>();
       const bool flagSet = curValue & conditionBitMask;
       const bool alreadyExclusive = curValue & getAdminRef<kExclusive>();
-      if (failIfRefNotZero && (curValue & kAccessRefMask) != 0) {
+      const bool isChained = curValue & getFlag<kIsChainedItem>();
+
+      // chained item can have ref count == 1, this just means it's linked in the chain
+      if (isChained && (curValue & kAccessRefMask) > 1) {
+        return false;
+      } else if (!isChained && (curValue & kAccessRefMask) != 0) {
         return false;
       }
-      if (!flagSet || alreadyExclusive) {
+      // when we mark a chained item as moving it - it may not
+      // be in the mmContainer
+      if ( (!isChained && (!flagSet || alreadyExclusive)) ||
+           ( isChained && alreadyExclusive) ) {
         return false;
       }
       if (UNLIKELY((curValue & kAccessRefMask) == (kAccessRefMask))) {

cachelib/allocator/Refcount.h

@@ -0,0 +1,40 @@
+// @nolint like default.json, but moves items during slab release instead of evicting them.
+{
+    "cache_config" : {
+      "cacheSizeMB" : 2248,
+      "cacheDir": "/tmp/mem-tier5",
+      "memoryTiers" : [
+        {
+          "ratio": 1,
+          "memBindNodes": 0
+        }, {
+          "ratio": 1,
+          "memBindNodes": 0
+        }
+      ],
+      "poolRebalanceIntervalSec" : 1,
+      "moveOnSlabRelease" : true,
+      "rebalanceMinSlabs" : 2,
+      "evictorThreads": 2,
+      "promoterThreads": 2
+    },
+    "test_config" : 
+      {
+        "preallocateCache" : true,
+        "numOps" : 40000000,
+        "numThreads" : 32,
+        "numKeys" : 250000,
+        "generator": "online",
+  
+        "keySizeRange" : [1, 8, 32, 64, 128, 256, 512],
+        "keySizeRangeProbability" : [0.1, 0.1, 0.2, 0.2, 0.3, 0.1],
+  
+        "valSizeRange" : [1, 128, 512, 1024, 4096, 10240, 20480, 40960, 60000],
+        "valSizeRangeProbability" : [0.1, 0.1, 0.1, 0.2, 0.2, 0.1, 0.1, 0.1],
+  
+        "getRatio" : 0.70,
+        "setRatio" : 0.30
+      }
+   
+  }
+  

cachelib/cachebench/test_configs/small_moving_bg.json

@@ -341,6 +341,7 @@ class RWBucketLocks : public BaseBucketLocks<LockType, LockAlignmentType> {
   using Lock = LockType;
   using ReadLockHolder = ReadLockHolderType;
   using WriteLockHolder = WriteLockHolderType;
+  using LockHolder = std::unique_lock<Lock>;
 
   RWBucketLocks(uint32_t locksPower, std::shared_ptr<Hash> hasher)
       : Base::BaseBucketLocks(locksPower, std::move(hasher)) {}
@@ -357,6 +358,11 @@ class RWBucketLocks : public BaseBucketLocks<LockType, LockAlignmentType> {
     return WriteLockHolder{Base::getLock(args...)};
   }
 
+  template <typename... Args>
+  LockHolder tryLockExclusive(Args... args) noexcept {
+    return LockHolder(Base::getLock(args...), std::try_to_lock);
+  }
+
   // try to grab the reader lock for a limit _timeout_ duration
   template <typename... Args>
   ReadLockHolder lockShared(const std::chrono::microseconds& timeout,

cachelib/common/Mutex.h

@@ -13,3 +13,4 @@ fi
 
 ../bin/cachebench --json_test_config ../test_configs/consistency/navy.json
 ../bin/cachebench --json_test_config ../test_configs/consistency/navy-multi-tier.json
+../opt/bin/cachebench --json_test_config /opt/test_configs/small_moving_bg.json