Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Shorten critical section in findEviction #217

Closed
wants to merge 3 commits into from
Closed

Shorten critical section in findEviction #217

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c44e7f9
Accept reference to the item instead of handle in NVMCache::put()
igchor Mar 23, 2023
0253683
Refactor findEviction function using markForEviction bit
igchor Mar 23, 2023
5d5c3f0
Fix issue with token creation
igchor Apr 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
291 changes: 105 additions & 186 deletions cachelib/allocator/CacheAllocator-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1240,6 +1240,19 @@ bool CacheAllocator<CacheTrait>::moveChainedItem(ChainedItem& oldItem,
return true;
}

template <typename CacheTrait>
void CacheAllocator<CacheTrait>::unlinkItemForEviction(Item& it) {
XDCHECK(it.isMarkedForEviction());
XDCHECK(it.getRefCount() == 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: XDCHECK_EQ(0, it.getRefcount());

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

accessContainer_->remove(it);
removeFromMMContainer(it);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didn't we already remove this item from mm-container in findEviction at L1313? (if it were a parent or just a regular item?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we might have removed the item but removeFromMMContainer checks if the item is still in the MMContainer and only then calls mmcontainer.remove(). We need to keep this call here because If the parent of the item changed, the item will still be in the mmcontainer.


// Since we managed to mark the item for eviction we must be the only
// owner of the item.
const auto ref = it.unmarkForEviction();
XDCHECK(ref == 0u);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same nitpick: XDCHECK_EQ

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}

template <typename CacheTrait>
typename CacheAllocator<CacheTrait>::Item*
CacheAllocator<CacheTrait>::findEviction(PoolId pid, ClassId cid) {
Expand All @@ -1248,76 +1261,102 @@ CacheAllocator<CacheTrait>::findEviction(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) {
++searchTries;
(*stats_.evictionAttempts)[pid][cid].inc();

Item* toRecycle = itr.get();

Item* candidate =
toRecycle->isChainedItem()
? &toRecycle->asChainedItem().getParentItem(compressor_)
: toRecycle;

// make sure no other thead is evicting the item
if (candidate->getRefCount() != 0 || !candidate->markMoving()) {
++itr;
continue;
}

// 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.
bool evictionSuccessful = false;
{
auto toReleaseHandle =
itr->isChainedItem()
? advanceIteratorAndTryEvictChainedItem(itr)
: advanceIteratorAndTryEvictRegularItem(mmContainer, itr);
evictionSuccessful = toReleaseHandle != nullptr;
// destroy toReleaseHandle. The item won't be released to allocator
// since we marked for eviction.
}

const auto ref = candidate->unmarkMoving();
if (ref == 0u) {
// Invalidate iterator since later on we may use this mmContainer
// again, which cannot be done unless we drop this iterator
itr.destroy();

// recycle the item. it's safe to do so, even if toReleaseHandle was
// NULL. If `ref` == 0 then it means that we are the last holder of
// that item.
if (candidate->hasChainedItem()) {
(*stats_.chainedItemEvictions)[pid][cid].inc();
} else {
(*stats_.regularItemEvictions)[pid][cid].inc();
while (config_.evictionSearchTries == 0 ||
config_.evictionSearchTries > searchTries) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need this when we're doing a while loop already inside withEvictionIterator?

Can we just reset the iterator internally and restart the search under lock?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potentially yes, but it might have an impact on performance. The current implementation behaves pretty much the same as the original implementation. If we got rid of this loop, the MMContainer lock could be held for a longer duration.

Resetting the iterator under the lock might actually be a better solution but I just didn't want to change the behavior too much in this PR - perhaps it would be better to do it in a separate PR and evaluate it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I think the outer loop is also need for cases when releaseBackToAllocator ends up not releasing the item we intend to - this is already done outside the lock.

Item* toRecycle = nullptr;
Item* candidate = nullptr;
typename NvmCacheT::PutToken token;

mmContainer.withEvictionIterator([this, pid, cid, &candidate, &toRecycle,
&searchTries, &mmContainer,
&token](auto&& itr) {
if (!itr) {
++searchTries;
(*stats_.evictionAttempts)[pid][cid].inc();
return;
}

if (auto eventTracker = getEventTracker()) {
eventTracker->record(AllocatorApiEvent::DRAM_EVICT, candidate->getKey(),
AllocatorApiResult::EVICTED, candidate->getSize(),
candidate->getConfiguredTTL().count());
}
while ((config_.evictionSearchTries == 0 ||
config_.evictionSearchTries > searchTries) &&
itr) {
++searchTries;
(*stats_.evictionAttempts)[pid][cid].inc();

auto* toRecycle_ = itr.get();
auto* candidate_ =
toRecycle_->isChainedItem()
? &toRecycle_->asChainedItem().getParentItem(compressor_)
: toRecycle_;

const bool evictToNvmCache = shouldWriteToNvmCache(*candidate_);
if (evictToNvmCache)
token = nvmCache_->createPutToken(candidate_->getKey());

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (evictToNvmCache)
token = nvmCache_->createPutToken(candidate_->getKey());
if (evictToNvmCache) {
token = nvmCache_->createPutToken(candidate_->getKey());
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this might have been lost in one of the patches you add after you ran test but here is a bug.
The token can be created on candidate A, then candidate A failed to markForEviction and we move to the next item.
For candidate B, evictToNvmCache is false but we successfully markForEviction.

We'll exit this loop with candidate B but the token created from candidate A.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw a crash in production test with exception "Item is not nvm evicted and nvm clean". And I suspect this is the reason. I'm putting together a hacky fix on my end to resume the test.

if (evictToNvmCache && !token.isValid()) {
stats_.evictFailConcurrentFill.inc();
} else if (candidate_->markForEviction()) {
XDCHECK(candidate_->isMarkedForEviction());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we refactor this a bit to have fewer nested if-stmt?

e.g.

if (evictToNvmCache && !token_.isValid()) {
  stats_.evictFailConcurrentFill.inc();
  ++itr;
  continue;
}

auto markedForEviction = candidate_->markForEviction();
if (!markedForEviction) {
  ++itr;
  continue;
}

// ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, done.

// markForEviction to make sure no other thead is evicting the item
// nor holding a handle to that item
toRecycle = toRecycle_;
candidate = candidate_;

// Check if parent changed for chained items - if yes, we cannot
// remove the child from the mmContainer as we will not be evicting
// it. We could abort right here, but we need to cleanup in case
// unmarkForEviction() returns 0 - so just go through normal path.
if (!toRecycle_->isChainedItem() ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate more?
In the old way of eviction, if we are trying to recycle a chained item (toRecycle.isChainedItem()) but the parent has changed, we don't do any specific treatment until in releaseBackToAllocator, we'll go through the chain with parent being candidate, and found out that toRecycle was never reached by walking the chain. So releaseBackToAllocator fails and we continue iterating the eviction queue.

How does the new way work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new way is pretty much the same. This comment states that we don't do any specific treatment as well. We'll just try to evict the item as usual and leverage the existing logic in releaseBackToAllocator.

It might be possible to optimize this case but for this PR I felt like having similar behavior is better.

The only reason for this if here is to optimize the happy case: if the item is not a chained item or the parent hasn't changed, we can safely remove the item from MMContainer (we already hold the MMContainer lock so doing it here has low overhead). If the parent has changed we cannot call the mmContainer.remove() here as it could lead to deadlocks. Instead, we'll just call mmContainer.remove() inside unlinkItemForEviction which is slower (need to acquire the lock again) but safe.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the optimization now.
I think as long as the chained item can't change parent after this point then this is safe. And since we marked the parent as forEviction, it can't be moving nor accessed, so we don't have to do the chainedItemLock_ here.

How much does this optimization give us?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using default hit_ratio/graph_cache_leader_fbobj/config.json this optimization improves throughput by ~40% on my machine. This is mostly due to almost 100% eviction Success rate (without the optimization I get ~50% Success rate).

&toRecycle->asChainedItem().getParentItem(compressor_) ==
candidate)
mmContainer.remove(itr);
return;
} else {
if (candidate_->hasChainedItem()) {
stats_.evictFailParentAC.inc();
} else {
stats_.evictFailAC.inc();
}
}

// check if by releasing the item we intend to, we actually
// recycle the candidate.
if (ReleaseRes::kRecycled ==
releaseBackToAllocator(*candidate, RemoveContext::kEviction,
/* isNascent */ false, toRecycle)) {
return toRecycle;
++itr;
XDCHECK(toRecycle == nullptr);
XDCHECK(candidate == nullptr);
}
});

if (!toRecycle)
continue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!toRecycle)
continue;
if (!toRecycle) {
continue;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This happens iff we exhausted the entire linked list and hasn't found a candidate while not exhausting searchTries right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's correct.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the format suggestion.


XDCHECK(toRecycle);
XDCHECK(candidate);

unlinkItemForEviction(*candidate);

if (token.isValid() && shouldWriteToNvmCacheExclusive(*candidate)) {
nvmCache_->put(*candidate, std::move(token));
}

// recycle the item. it's safe to do so, even if toReleaseHandle was
// NULL. If `ref` == 0 then it means that we are the last holder of
// that item.
if (candidate->hasChainedItem()) {
(*stats_.chainedItemEvictions)[pid][cid].inc();
} else {
XDCHECK(!evictionSuccessful);
(*stats_.regularItemEvictions)[pid][cid].inc();
}

// If we destroyed the itr to possibly evict and failed, we restart
// from the beginning again
if (!itr) {
itr.resetToBegin();
if (auto eventTracker = getEventTracker()) {
eventTracker->record(AllocatorApiEvent::DRAM_EVICT, candidate->getKey(),
AllocatorApiResult::EVICTED, candidate->getSize(),
candidate->getConfiguredTTL().count());
}

// check if by releasing the item we intend to, we actually
// recycle the candidate.
auto ret = releaseBackToAllocator(*candidate, RemoveContext::kEviction,
/* isNascent */ false, toRecycle);
if (ret == ReleaseRes::kRecycled) {
return toRecycle;
}
}
return nullptr;
Expand Down Expand Up @@ -1461,7 +1500,7 @@ bool CacheAllocator<CacheTrait>::pushToNvmCacheFromRamForTesting(

if (handle && nvmCache_ && shouldWriteToNvmCache(*handle) &&
shouldWriteToNvmCacheExclusive(*handle)) {
nvmCache_->put(handle, nvmCache_->createPutToken(handle->getKey()));
nvmCache_->put(*handle, nvmCache_->createPutToken(handle->getKey()));
return true;
}
return false;
Expand Down Expand Up @@ -2660,126 +2699,6 @@ void CacheAllocator<CacheTrait>::evictForSlabRelease(
}
}

template <typename CacheTrait>
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
// and the item is not expired.
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 WriteHandle{};
}

// 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, &itemExclusivePredicate);
if (!evictHandle) {
++itr;
stats_.evictFailAC.inc();
return evictHandle;
}

mmContainer.remove(itr);
XDCHECK_EQ(reinterpret_cast<uintptr_t>(evictHandle.get()),
reinterpret_cast<uintptr_t>(&item));
XDCHECK(!evictHandle->isInMMContainer());
XDCHECK(!evictHandle->isAccessible());

// 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 CacheTrait>
typename CacheAllocator<CacheTrait>::WriteHandle
CacheAllocator<CacheTrait>::advanceIteratorAndTryEvictChainedItem(
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 WriteHandle{};
}

// check if the parent exists in the hashtable and refcount is drained.
auto parentHandle =
accessContainer_->removeIf(parent, &itemExclusivePredicate);
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<uintptr_t>(&parent),
reinterpret_cast<uintptr_t>(parentHandle.get()));
XDCHECK_EQ(1u, parent.getRefCount());

removeFromMMContainer(*parentHandle);

XDCHECK(!parent.isInMMContainer());
XDCHECK(!parent.isAccessible());

if (evictToNvmCache && shouldWriteToNvmCacheExclusive(*parentHandle)) {
XDCHECK(token.isValid());
XDCHECK(parentHandle->hasChainedItem());
nvmCache_->put(parentHandle, std::move(token));
}

return parentHandle;
}

template <typename CacheTrait>
typename CacheAllocator<CacheTrait>::WriteHandle
CacheAllocator<CacheTrait>::evictNormalItemForSlabRelease(Item& item) {
Expand Down Expand Up @@ -2812,7 +2731,7 @@ CacheAllocator<CacheTrait>::evictNormalItemForSlabRelease(Item& item) {
// now that we are the only handle and we actually removed something from
// the RAM cache, we enqueue it to nvmcache.
if (evictToNvmCache && shouldWriteToNvmCacheExclusive(item)) {
nvmCache_->put(handle, std::move(token));
nvmCache_->put(*handle, std::move(token));
}

return handle;
Expand Down Expand Up @@ -2913,7 +2832,7 @@ CacheAllocator<CacheTrait>::evictChainedItemForSlabRelease(ChainedItem& child) {
// the RAM cache, we enqueue it to nvmcache.
if (evictToNvmCache && shouldWriteToNvmCacheExclusive(*parentHandle)) {
DCHECK(parentHandle->hasChainedItem());
nvmCache_->put(parentHandle, std::move(token));
nvmCache_->put(*parentHandle, std::move(token));
}

return parentHandle;
Expand Down
25 changes: 6 additions & 19 deletions cachelib/allocator/CacheAllocator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1652,6 +1652,12 @@ class CacheAllocator : public CacheBase {
bool removeFromNvm = true,
bool recordApiEvent = true);

// Must be called by the thread which called markForEviction and
// succeeded. After this call, the item is unlinked from Access and
// MM Containers. The item is no longer marked as exclusive and it's
// ref count is 0 - it's available for recycling.
void unlinkItemForEviction(Item& it);

// Implementation to find a suitable eviction from the container. The
// two parameters together identify a single container.
//
Expand All @@ -1662,25 +1668,6 @@ class CacheAllocator : public CacheBase {

using EvictionIterator = typename MMContainer::LockedIterator;

// 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
Expand Down
Loading