diff --git a/cachelib/allocator/CacheAllocator-inl.h b/cachelib/allocator/CacheAllocator-inl.h index c8c11c77f5..2861d1bcbc 100644 --- a/cachelib/allocator/CacheAllocator-inl.h +++ b/cachelib/allocator/CacheAllocator-inl.h @@ -415,7 +415,7 @@ CacheAllocator::allocateInternalTier(TierId tid, } template -typename CacheAllocator::ItemHandle +typename CacheAllocator::WriteHandle CacheAllocator::allocateInternal(PoolId pid, typename Item::Key key, uint32_t size, @@ -1186,14 +1186,13 @@ bool CacheAllocator::addWaitContextForMovingItem( } template -template typename CacheAllocator::ItemHandle CacheAllocator::moveRegularItemOnEviction( - ItemPtr& oldItemPtr, ItemHandle& newItemHdl) { + Item& oldItem, ItemHandle& newItemHdl) { + XDCHECK(oldItem.isMoving()); // TODO: should we introduce new latency tracker. E.g. evictRegularLatency_ // ??? util::LatencyTracker tracker{stats_.evictRegularLatency_}; - Item& oldItem = *oldItemPtr; if (!oldItem.isAccessible() || oldItem.isExpired()) { return {}; } @@ -1249,7 +1248,7 @@ CacheAllocator::moveRegularItemOnEviction( // it is unsafe to replace the old item with a new one, so we should // also abort. if (!accessContainer_->replaceIf(oldItem, *newItemHdl, - itemEvictionPredicate)) { + itemMovingPredicate)) { return {}; } @@ -1269,7 +1268,7 @@ CacheAllocator::moveRegularItemOnEviction( // Inside the MM container's lock, this checks if the old item exists to // make sure that no other thread removed it, and only then replaces it. - if (!replaceInMMContainer(oldItemPtr, *newItemHdl)) { + if (!replaceInMMContainer(oldItem, *newItemHdl)) { accessContainer_->remove(*newItemHdl); return {}; } @@ -1453,42 +1452,64 @@ CacheAllocator::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; - Item* candidate = itr.get(); + Item* toRecycle = nullptr; + Item* candidate = nullptr; + + mmContainer.withEvictionIterator([this, &candidate, &toRecycle, &searchTries](auto &&itr){ + while ((config_.evictionSearchTries == 0 || + config_.evictionSearchTries > searchTries) && itr) { + ++searchTries; + + 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; + + 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 // recycles the child we intend to. - - ItemHandle toReleaseHandle = tryEvictToNextMemoryTier(tid, pid, itr); - bool movedToNextTier = false; - if(toReleaseHandle) { - movedToNextTier = true; - } else { - toReleaseHandle = - itr->isChainedItem() - ? advanceIteratorAndTryEvictChainedItem(tid, pid, itr) - : advanceIteratorAndTryEvictRegularItem(tid, pid, mmContainer, itr); - } + auto toReleaseHandle = + evictNormalItem(*candidate, true /* skipIfTokenInvalid */); + auto ref = candidate->unmarkMoving(); - if (toReleaseHandle) { - if (toReleaseHandle->hasChainedItem()) { + if (toReleaseHandle || ref == 0u) { + if (candidate->hasChainedItem()) { (*stats_.chainedItemEvictions)[pid][cid].inc(); } else { (*stats_.regularItemEvictions)[pid][cid].inc(); } + } else { + if (candidate->hasChainedItem()) { + stats_.evictFailParentAC.inc(); + } else { + stats_.evictFailAC.inc(); + } + } - // Invalidate iterator since later on we may use this mmContainer - // again, which cannot be done unless we drop this iterator - itr.destroy(); - - // we must be the last handle and for chained items, this will be - // the parent. - XDCHECK(toReleaseHandle.get() == candidate || candidate->isChainedItem()); + if (toReleaseHandle) { + XDCHECK(toReleaseHandle.get() == candidate); + XDCHECK(toRecycle == candidate || toRecycle->isChainedItem()); XDCHECK_EQ(1u, toReleaseHandle->getRefCount()); // We manually release the item here because we don't want to @@ -1504,15 +1525,18 @@ CacheAllocator::findEviction(TierId tid, PoolId pid, ClassId cid) { // recycle the candidate. if (ReleaseRes::kRecycled == releaseBackToAllocator(itemToRelease, RemoveContext::kEviction, - /* isNascent */ movedToNextTier, candidate)) { - return candidate; + /* isNascent */ false, toRecycle)) { + return toRecycle; + } + } else if (ref == 0u) { + // it's safe to recycle the item here as there are no more + // references and the item could not been marked as moving + // by other thread since it's detached from MMContainer. + if (ReleaseRes::kRecycled == + releaseBackToAllocator(*candidate, RemoveContext::kEviction, + /* isNascent */ false, toRecycle)) { + return toRecycle; } - } - - // If we destroyed the itr to possibly evict and failed, we restart - // from the beginning again - if (!itr) { - itr.resetToBegin(); } } return nullptr; @@ -1567,24 +1591,23 @@ bool CacheAllocator::shouldWriteToNvmCacheExclusive( } template -template -typename CacheAllocator::ItemHandle +typename CacheAllocator::WriteHandle CacheAllocator::tryEvictToNextMemoryTier( - TierId tid, PoolId pid, ItemPtr& item) { - if(item->isChainedItem()) return {}; // TODO: We do not support ChainedItem yet - if(item->isExpired()) return acquire(item); + TierId tid, PoolId pid, Item& item) { + if(item.isChainedItem()) return {}; // TODO: We do not support ChainedItem yet + if(item.isExpired()) return acquire(&item); TierId nextTier = tid; // TODO - calculate this based on some admission policy while (++nextTier < numTiers_) { // try to evict down to the next memory tiers // allocateInternal might trigger another eviction auto newItemHdl = allocateInternalTier(nextTier, pid, - item->getKey(), - item->getSize(), - item->getCreationTime(), - item->getExpiryTime()); + item.getKey(), + item.getSize(), + item.getCreationTime(), + item.getExpiryTime()); if (newItemHdl) { - XDCHECK_EQ(newItemHdl->getSize(), item->getSize()); + XDCHECK_EQ(newItemHdl->getSize(), item.getSize()); return moveRegularItemOnEviction(item, newItemHdl); } @@ -1594,149 +1617,11 @@ CacheAllocator::tryEvictToNextMemoryTier( } template -typename CacheAllocator::ItemHandle -CacheAllocator::tryEvictToNextMemoryTier(Item* item) { - auto tid = getTierId(*item); - auto pid = allocator_[tid]->getAllocInfo(item->getMemory()).poolId; - return tryEvictToNextMemoryTier(tid, pid, item); -} - -template -typename CacheAllocator::ItemHandle -CacheAllocator::advanceIteratorAndTryEvictRegularItem( - TierId tid, PoolId pid, MMContainer& mmContainer, EvictionIterator& itr) { - Item& item = *itr; - - const bool evictToNvmCache = shouldWriteToNvmCache(item); - - auto token = evictToNvmCache ? nvmCache_->createPutToken(item.getKey()) - : typename NvmCacheT::PutToken{}; - // record the in-flight eviciton. If not, we move on to next item to avoid - // stalling eviction. - if (evictToNvmCache && !token.isValid()) { - ++itr; - stats_.evictFailConcurrentFill.inc(); - return ItemHandle{}; - } - - // If there are other accessors, we should abort. Acquire a handle here since - // if we remove the item from both access containers and mm containers - // below, we will need a handle to ensure proper cleanup in case we end up - // not evicting this item - auto evictHandle = accessContainer_->removeIf(item, &itemEvictionPredicate); - - if (!evictHandle) { - ++itr; - stats_.evictFailAC.inc(); - return evictHandle; - } - - mmContainer.remove(itr); - XDCHECK_EQ(reinterpret_cast(evictHandle.get()), - reinterpret_cast(&item)); - XDCHECK(!evictHandle->isInMMContainer()); - XDCHECK(!evictHandle->isAccessible()); - - // If the item is now marked as moving, that means its corresponding slab is - // being released right now. So, we look for the next item that is eligible - // for eviction. It is safe to destroy the handle here since the moving bit - // is set. Iterator was already advance by the remove call above. - if (evictHandle->isMoving()) { - stats_.evictFailMove.inc(); - return ItemHandle{}; - } - - // Invalidate iterator since later on if we are not evicting this - // item, we may need to rely on the handle we created above to ensure - // proper cleanup if the item's raw refcount has dropped to 0. - // And since this item may be a parent item that has some child items - // in this very same mmContainer, we need to make sure we drop this - // exclusive iterator so we can gain access to it when we're cleaning - // up the child items - itr.destroy(); - - // Ensure that there are no accessors after removing from the access - // container - XDCHECK(evictHandle->getRefCount() == 1); - - if (evictToNvmCache && shouldWriteToNvmCacheExclusive(item)) { - XDCHECK(token.isValid()); - nvmCache_->put(evictHandle, std::move(token)); - } - return evictHandle; -} - -template -typename CacheAllocator::ItemHandle -CacheAllocator::advanceIteratorAndTryEvictChainedItem( - TierId tid, PoolId pid, EvictionIterator& itr) { - XDCHECK(itr->isChainedItem()); - - ChainedItem* candidate = &itr->asChainedItem(); - ++itr; - - // The parent could change at any point through transferChain. However, if - // that happens, we would realize that the releaseBackToAllocator return - // kNotRecycled and we would try another chained item, leading to transient - // failure. - auto& parent = candidate->getParentItem(compressor_); - - const bool evictToNvmCache = shouldWriteToNvmCache(parent); - - auto token = evictToNvmCache ? nvmCache_->createPutToken(parent.getKey()) - : typename NvmCacheT::PutToken{}; - - // if token is invalid, return. iterator is already advanced. - if (evictToNvmCache && !token.isValid()) { - stats_.evictFailConcurrentFill.inc(); - return ItemHandle{}; - } - - // check if the parent exists in the hashtable and refcount is drained. - auto parentHandle = - accessContainer_->removeIf(parent, &itemEvictionPredicate); - if (!parentHandle) { - stats_.evictFailParentAC.inc(); - return parentHandle; - } - - // Invalidate iterator since later on we may use the mmContainer - // associated with this iterator which cannot be done unless we - // drop this iterator - // - // This must be done once we know the parent is not nullptr. - // Since we can very well be the last holder of this parent item, - // which may have a chained item that is linked in this MM container. - itr.destroy(); - - // Ensure we have the correct parent and we're the only user of the - // parent, then free it from access container. Otherwise, we abort - XDCHECK_EQ(reinterpret_cast(&parent), - reinterpret_cast(parentHandle.get())); - XDCHECK_EQ(1u, parent.getRefCount()); - - removeFromMMContainer(*parentHandle); - - XDCHECK(!parent.isInMMContainer()); - XDCHECK(!parent.isAccessible()); - - // TODO: add multi-tier support (similar as for unchained items) - - // We need to make sure the parent is not marked as moving - // and we're the only holder of the parent item. Safe to destroy the handle - // here since moving bit is set. - if (parentHandle->isMoving()) { - stats_.evictFailParentMove.inc(); - return ItemHandle{}; - } - - if (evictToNvmCache && shouldWriteToNvmCacheExclusive(*parentHandle)) { - XDCHECK(token.isValid()); - XDCHECK(parentHandle->hasChainedItem()); - nvmCache_->put(parentHandle, std::move(token)); - } - - return parentHandle; +typename CacheAllocator::WriteHandle +CacheAllocator::tryEvictToNextMemoryTier(Item& item) { + auto tid = getTierId(item); + auto pid = allocator_[tid]->getAllocInfo(item.getMemory()).poolId; + return tryEvictToNextMemoryTier(tid, pid, item); } template @@ -2936,7 +2821,7 @@ void CacheAllocator::evictForSlabRelease( auto owningHandle = item.isChainedItem() ? evictChainedItemForSlabRelease(item.asChainedItem()) - : evictNormalItemForSlabRelease(item); + : evictNormalItem(item); // we managed to evict the corresponding owner of the item and have the // last handle for the owner. @@ -2993,14 +2878,15 @@ void CacheAllocator::evictForSlabRelease( template typename CacheAllocator::ItemHandle -CacheAllocator::evictNormalItemForSlabRelease(Item& item) { +CacheAllocator::evictNormalItem(Item& item, + bool skipIfTokenInvalid) { XDCHECK(item.isMoving()); if (item.isOnlyMoving()) { return ItemHandle{}; } - auto evictHandle = tryEvictToNextMemoryTier(&item); + auto evictHandle = tryEvictToNextMemoryTier(item); if(evictHandle) return evictHandle; auto predicate = [](const Item& it) { return it.getRefCount() == 0; }; @@ -3009,6 +2895,11 @@ CacheAllocator::evictNormalItemForSlabRelease(Item& item) { auto token = evictToNvmCache ? nvmCache_->createPutToken(item.getKey()) : typename NvmCacheT::PutToken{}; + if (skipIfTokenInvalid && evictToNvmCache && !token.isValid()) { + stats_.evictFailConcurrentFill.inc(); + return ItemHandle{}; + } + // We remove the item from both access and mm containers. It doesn't matter // if someone else calls remove on the item at this moment, the item cannot // be freed as long as we have the moving bit set. diff --git a/cachelib/allocator/CacheAllocator.h b/cachelib/allocator/CacheAllocator.h index 319e66a626..fb342a6b71 100644 --- a/cachelib/allocator/CacheAllocator.h +++ b/cachelib/allocator/CacheAllocator.h @@ -1401,8 +1401,7 @@ class CacheAllocator : public CacheBase { // // @return true If the move was completed, and the containers were updated // successfully. - template - ItemHandle moveRegularItemOnEviction(ItemPtr& oldItem, ItemHandle& newItemHdl); + ItemHandle moveRegularItemOnEviction(Item& oldItem, ItemHandle& newItemHdl); // 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 @@ -1561,25 +1560,6 @@ class CacheAllocator : public CacheBase { // @return An evicted item or nullptr if there is no suitable candidate. Item* findEviction(TierId tid, PoolId pid, ClassId cid); - // 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. - ItemHandle advanceIteratorAndTryEvictRegularItem(TierId tid, PoolId pid, 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 - ItemHandle advanceIteratorAndTryEvictChainedItem(TierId tid, PoolId pid, EvictionIterator& itr); - // Try to move the item down to the next memory tier // // @param tid current tier ID of the item @@ -1588,8 +1568,7 @@ class CacheAllocator : public CacheBase { // // @return valid handle to the item. This will be the last // handle to the item. On failure an empty handle. - template - ItemHandle tryEvictToNextMemoryTier(TierId tid, PoolId pid, ItemPtr& item); + WriteHandle tryEvictToNextMemoryTier(TierId tid, PoolId pid, Item& item); // Try to move the item down to the next memory tier // @@ -1597,7 +1576,7 @@ class CacheAllocator : public CacheBase { // // @return valid handle to the item. This will be the last // handle to the item. On failure an empty handle. - ItemHandle tryEvictToNextMemoryTier(Item* item); + WriteHandle tryEvictToNextMemoryTier(Item& item); // Deserializer CacheAllocatorMetadata and verify the version // @@ -1724,7 +1703,7 @@ 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); + ItemHandle evictNormalItem(Item& item, bool skipIfTokenInvalid = false); // Helper function to evict a child item for slab release // As a side effect, the parent item is also evicted @@ -1845,10 +1824,6 @@ class CacheAllocator : public CacheBase { 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(); } diff --git a/cachelib/allocator/CacheItem-inl.h b/cachelib/allocator/CacheItem-inl.h index dcdaf4444d..d26d2ac303 100644 --- a/cachelib/allocator/CacheItem-inl.h +++ b/cachelib/allocator/CacheItem-inl.h @@ -229,8 +229,8 @@ bool CacheItem::markMoving() noexcept { } template -void CacheItem::unmarkMoving() noexcept { - ref_.unmarkMoving(); +RefcountWithFlags::Value CacheItem::unmarkMoving() noexcept { + return ref_.unmarkMoving(); } template diff --git a/cachelib/allocator/CacheItem.h b/cachelib/allocator/CacheItem.h index aa660b401b..9bf3da5fcc 100644 --- a/cachelib/allocator/CacheItem.h +++ b/cachelib/allocator/CacheItem.h @@ -378,7 +378,7 @@ class CACHELIB_PACKED_ATTR CacheItem { * Unmarking moving does not depend on `isInMMContainer` */ bool markMoving() noexcept; - void unmarkMoving() noexcept; + RefcountWithFlags::Value unmarkMoving() noexcept; bool isMoving() const noexcept; bool isOnlyMoving() const noexcept; diff --git a/cachelib/allocator/MM2Q-inl.h b/cachelib/allocator/MM2Q-inl.h index c112f0b442..e791d6c6c3 100644 --- a/cachelib/allocator/MM2Q-inl.h +++ b/cachelib/allocator/MM2Q-inl.h @@ -238,22 +238,21 @@ MM2Q::Container::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 T::*HookPtr> +template +void +MM2Q::Container::withEvictionIterator(F&& fun) { + lruMutex_->lock_combine([this, &fun]() { + fun(Iterator{LockHolder{}, lru_.rbegin()}); + }); +} + + template T::*HookPtr> void MM2Q::Container::removeLocked(T& node, bool doRebalance) noexcept { diff --git a/cachelib/allocator/MM2Q.h b/cachelib/allocator/MM2Q.h index f669192251..5138a78421 100644 --- a/cachelib/allocator/MM2Q.h +++ b/cachelib/allocator/MM2Q.h @@ -438,6 +438,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 + void withEvictionIterator(F&& f); + // get the current config as a copy Config getConfig() const; diff --git a/cachelib/allocator/MMLru-inl.h b/cachelib/allocator/MMLru-inl.h index 32972f06a5..a1b8bc6961 100644 --- a/cachelib/allocator/MMLru-inl.h +++ b/cachelib/allocator/MMLru-inl.h @@ -225,6 +225,15 @@ MMLru::Container::getEvictionIterator() const noexcept { return Iterator{std::move(l), lru_.rbegin()}; } +template T::*HookPtr> +template +void +MMLru::Container::withEvictionIterator(F&& fun) { + lruMutex_->lock_combine([this, &fun]() { + fun(Iterator{LockHolder{}, lru_.rbegin()}); + }); +} + template T::*HookPtr> void MMLru::Container::ensureNotInsertionPoint(T& node) noexcept { // If we are removing the insertion point node, grow tail before we remove diff --git a/cachelib/allocator/MMLru.h b/cachelib/allocator/MMLru.h index 8c0710f9b6..d4240c8d52 100644 --- a/cachelib/allocator/MMLru.h +++ b/cachelib/allocator/MMLru.h @@ -333,6 +333,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 + void withEvictionIterator(F&& f); + // get copy of current config Config getConfig() const; diff --git a/cachelib/allocator/MMTinyLFU-inl.h b/cachelib/allocator/MMTinyLFU-inl.h index 9d92c7a16b..53b081062e 100644 --- a/cachelib/allocator/MMTinyLFU-inl.h +++ b/cachelib/allocator/MMTinyLFU-inl.h @@ -220,6 +220,15 @@ MMTinyLFU::Container::getEvictionIterator() const noexcept { return Iterator{std::move(l), *this}; } +template T::*HookPtr> +template +void +MMTinyLFU::Container::withEvictionIterator(F&& fun) { + LockHolder l(lruMutex_); + fun(Iterator{LockHolder{}, *this}); +} + + template T::*HookPtr> void MMTinyLFU::Container::removeLocked(T& node) noexcept { if (isTiny(node)) { diff --git a/cachelib/allocator/MMTinyLFU.h b/cachelib/allocator/MMTinyLFU.h index 863b05bf8e..c8425edf11 100644 --- a/cachelib/allocator/MMTinyLFU.h +++ b/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 + void withEvictionIterator(F&& f); + // for saving the state of the lru // // precondition: serialization must happen without any reader or writer diff --git a/cachelib/allocator/Refcount.h b/cachelib/allocator/Refcount.h index 0bd604700a..cb93fb838c 100644 --- a/cachelib/allocator/Refcount.h +++ b/cachelib/allocator/Refcount.h @@ -251,10 +251,10 @@ class FOLLY_PACK_ATTR RefcountWithFlags { /** * The following four functions are used to track whether or not * an item is currently in the process of being moved. This happens during a - * slab rebalance or resize operation. + * slab rebalance or resize operation or during eviction. * - * An item can only be marked moving when `isInMMContainer` returns true. - * This operation is atomic. + * An item can only be marked moving when `isInMMContainer` returns true and + * the item is not yet marked as moving. 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. @@ -271,7 +271,8 @@ class FOLLY_PACK_ATTR RefcountWithFlags { Value curValue = __atomic_load_n(refPtr, __ATOMIC_RELAXED); while (true) { const bool flagSet = curValue & conditionBitMask; - if (!flagSet) { + const bool alreadyMoving = curValue & bitMask; + if (!flagSet || alreadyMoving) { return false; } @@ -290,9 +291,9 @@ class FOLLY_PACK_ATTR RefcountWithFlags { } } } - void unmarkMoving() noexcept { + Value unmarkMoving() noexcept { Value bitMask = ~getAdminRef(); - __atomic_and_fetch(&refCount_, bitMask, __ATOMIC_ACQ_REL); + return __atomic_and_fetch(&refCount_, bitMask, __ATOMIC_ACQ_REL) & kRefMask; } bool isMoving() const noexcept { return getRaw() & getAdminRef(); } bool isOnlyMoving() const noexcept {