Skip to content

Commit 80af379

Browse files
igchorfacebook-github-bot
igchor
authored and
facebook-github-bot
committed
Shorten critical section in findEviction (#217)
Summary: This is the next part of the 'critical section' patch after #183. Original PR (some of the changes already upstreamed): #172 This PR contains changes to findEviction only. The remaining part (changes in SlabRelease code) will be provided in the next (final) PR. Pull Request resolved: #217 Reviewed By: therealgymmy Differential Revision: D45071460 Pulled By: haowu14 fbshipit-source-id: 4d44d75182537369a50e3c1ebb10a7c844449875
1 parent b20b514 commit 80af379

File tree

9 files changed

+149
-232
lines changed

9 files changed

+149
-232
lines changed

‎cachelib/allocator/CacheAllocator-inl.h‎

Lines changed: 111 additions & 185 deletions
Original file line numberDiff line numberDiff line change
@@ -1240,6 +1240,19 @@ bool CacheAllocator<CacheTrait>::moveChainedItem(ChainedItem& oldItem,
12401240
return true;
12411241
}
12421242

1243+
template <typename CacheTrait>
1244+
void CacheAllocator<CacheTrait>::unlinkItemForEviction(Item& it) {
1245+
XDCHECK(it.isMarkedForEviction());
1246+
XDCHECK_EQ(0u, it.getRefCount());
1247+
accessContainer_->remove(it);
1248+
removeFromMMContainer(it);
1249+
1250+
// Since we managed to mark the item for eviction we must be the only
1251+
// owner of the item.
1252+
const auto ref = it.unmarkForEviction();
1253+
XDCHECK_EQ(0u, ref);
1254+
}
1255+
12431256
template <typename CacheTrait>
12441257
typename CacheAllocator<CacheTrait>::Item*
12451258
CacheAllocator<CacheTrait>::findEviction(PoolId pid, ClassId cid) {
@@ -1248,76 +1261,109 @@ CacheAllocator<CacheTrait>::findEviction(PoolId pid, ClassId cid) {
12481261
// Keep searching for a candidate until we were able to evict it
12491262
// or until the search limit has been exhausted
12501263
unsigned int searchTries = 0;
1251-
auto itr = mmContainer.getEvictionIterator();
1252-
while ((config_.evictionSearchTries == 0 ||
1253-
config_.evictionSearchTries > searchTries) &&
1254-
itr) {
1255-
++searchTries;
1256-
(*stats_.evictionAttempts)[pid][cid].inc();
1257-
1258-
Item* toRecycle = itr.get();
1259-
1260-
Item* candidate =
1261-
toRecycle->isChainedItem()
1262-
? &toRecycle->asChainedItem().getParentItem(compressor_)
1263-
: toRecycle;
1264-
1265-
// make sure no other thead is evicting the item
1266-
if (candidate->getRefCount() != 0 || !candidate->markMoving()) {
1267-
++itr;
1264+
while (config_.evictionSearchTries == 0 ||
1265+
config_.evictionSearchTries > searchTries) {
1266+
Item* toRecycle = nullptr;
1267+
Item* candidate = nullptr;
1268+
typename NvmCacheT::PutToken token;
1269+
1270+
mmContainer.withEvictionIterator([this, pid, cid, &candidate, &toRecycle,
1271+
&searchTries, &mmContainer,
1272+
&token](auto&& itr) {
1273+
if (!itr) {
1274+
++searchTries;
1275+
(*stats_.evictionAttempts)[pid][cid].inc();
1276+
return;
1277+
}
1278+
1279+
while ((config_.evictionSearchTries == 0 ||
1280+
config_.evictionSearchTries > searchTries) &&
1281+
itr) {
1282+
++searchTries;
1283+
(*stats_.evictionAttempts)[pid][cid].inc();
1284+
1285+
auto* toRecycle_ = itr.get();
1286+
auto* candidate_ =
1287+
toRecycle_->isChainedItem()
1288+
? &toRecycle_->asChainedItem().getParentItem(compressor_)
1289+
: toRecycle_;
1290+
1291+
const bool evictToNvmCache = shouldWriteToNvmCache(*candidate_);
1292+
auto putToken = evictToNvmCache
1293+
? nvmCache_->createPutToken(candidate_->getKey())
1294+
: typename NvmCacheT::PutToken{};
1295+
1296+
if (evictToNvmCache && !putToken.isValid()) {
1297+
stats_.evictFailConcurrentFill.inc();
1298+
++itr;
1299+
continue;
1300+
}
1301+
1302+
auto markedForEviction = candidate_->markForEviction();
1303+
if (!markedForEviction) {
1304+
if (candidate_->hasChainedItem()) {
1305+
stats_.evictFailParentAC.inc();
1306+
} else {
1307+
stats_.evictFailAC.inc();
1308+
}
1309+
++itr;
1310+
continue;
1311+
}
1312+
1313+
XDCHECK(candidate_->isMarkedForEviction());
1314+
// markForEviction to make sure no other thead is evicting the item
1315+
// nor holding a handle to that item
1316+
toRecycle = toRecycle_;
1317+
candidate = candidate_;
1318+
token = std::move(putToken);
1319+
1320+
// Check if parent changed for chained items - if yes, we cannot
1321+
// remove the child from the mmContainer as we will not be evicting
1322+
// it. We could abort right here, but we need to cleanup in case
1323+
// unmarkForEviction() returns 0 - so just go through normal path.
1324+
if (!toRecycle_->isChainedItem() ||
1325+
&toRecycle->asChainedItem().getParentItem(compressor_) ==
1326+
candidate) {
1327+
mmContainer.remove(itr);
1328+
}
1329+
return;
1330+
}
1331+
});
1332+
1333+
if (!toRecycle) {
12681334
continue;
12691335
}
12701336

1271-
// for chained items, the ownership of the parent can change. We try to
1272-
// evict what we think as parent and see if the eviction of parent
1273-
// recycles the child we intend to.
1274-
bool evictionSuccessful = false;
1275-
{
1276-
auto toReleaseHandle =
1277-
itr->isChainedItem()
1278-
? advanceIteratorAndTryEvictChainedItem(itr)
1279-
: advanceIteratorAndTryEvictRegularItem(mmContainer, itr);
1280-
evictionSuccessful = toReleaseHandle != nullptr;
1281-
// destroy toReleaseHandle. The item won't be released to allocator
1282-
// since we marked for eviction.
1283-
}
1284-
1285-
const auto ref = candidate->unmarkMoving();
1286-
if (ref == 0u) {
1287-
// Invalidate iterator since later on we may use this mmContainer
1288-
// again, which cannot be done unless we drop this iterator
1289-
itr.destroy();
1290-
1291-
// recycle the item. it's safe to do so, even if toReleaseHandle was
1292-
// NULL. If `ref` == 0 then it means that we are the last holder of
1293-
// that item.
1294-
if (candidate->hasChainedItem()) {
1295-
(*stats_.chainedItemEvictions)[pid][cid].inc();
1296-
} else {
1297-
(*stats_.regularItemEvictions)[pid][cid].inc();
1298-
}
1337+
XDCHECK(toRecycle);
1338+
XDCHECK(candidate);
12991339

1300-
if (auto eventTracker = getEventTracker()) {
1301-
eventTracker->record(AllocatorApiEvent::DRAM_EVICT, candidate->getKey(),
1302-
AllocatorApiResult::EVICTED, candidate->getSize(),
1303-
candidate->getConfiguredTTL().count());
1304-
}
1340+
unlinkItemForEviction(*candidate);
13051341

1306-
// check if by releasing the item we intend to, we actually
1307-
// recycle the candidate.
1308-
if (ReleaseRes::kRecycled ==
1309-
releaseBackToAllocator(*candidate, RemoveContext::kEviction,
1310-
/* isNascent */ false, toRecycle)) {
1311-
return toRecycle;
1312-
}
1342+
if (token.isValid() && shouldWriteToNvmCacheExclusive(*candidate)) {
1343+
nvmCache_->put(*candidate, std::move(token));
1344+
}
1345+
1346+
// recycle the item. it's safe to do so, even if toReleaseHandle was
1347+
// NULL. If `ref` == 0 then it means that we are the last holder of
1348+
// that item.
1349+
if (candidate->hasChainedItem()) {
1350+
(*stats_.chainedItemEvictions)[pid][cid].inc();
13131351
} else {
1314-
XDCHECK(!evictionSuccessful);
1352+
(*stats_.regularItemEvictions)[pid][cid].inc();
13151353
}
13161354

1317-
// If we destroyed the itr to possibly evict and failed, we restart
1318-
// from the beginning again
1319-
if (!itr) {
1320-
itr.resetToBegin();
1355+
if (auto eventTracker = getEventTracker()) {
1356+
eventTracker->record(AllocatorApiEvent::DRAM_EVICT, candidate->getKey(),
1357+
AllocatorApiResult::EVICTED, candidate->getSize(),
1358+
candidate->getConfiguredTTL().count());
1359+
}
1360+
1361+
// check if by releasing the item we intend to, we actually
1362+
// recycle the candidate.
1363+
auto ret = releaseBackToAllocator(*candidate, RemoveContext::kEviction,
1364+
/* isNascent */ false, toRecycle);
1365+
if (ret == ReleaseRes::kRecycled) {
1366+
return toRecycle;
13211367
}
13221368
}
13231369
return nullptr;
@@ -1461,7 +1507,7 @@ bool CacheAllocator<CacheTrait>::pushToNvmCacheFromRamForTesting(
14611507

14621508
if (handle && nvmCache_ && shouldWriteToNvmCache(*handle) &&
14631509
shouldWriteToNvmCacheExclusive(*handle)) {
1464-
nvmCache_->put(handle, nvmCache_->createPutToken(handle->getKey()));
1510+
nvmCache_->put(*handle, nvmCache_->createPutToken(handle->getKey()));
14651511
return true;
14661512
}
14671513
return false;
@@ -2660,126 +2706,6 @@ void CacheAllocator<CacheTrait>::evictForSlabRelease(
26602706
}
26612707
}
26622708

2663-
template <typename CacheTrait>
2664-
typename CacheAllocator<CacheTrait>::WriteHandle
2665-
CacheAllocator<CacheTrait>::advanceIteratorAndTryEvictRegularItem(
2666-
MMContainer& mmContainer, EvictionIterator& itr) {
2667-
// we should flush this to nvmcache if it is not already present in nvmcache
2668-
// and the item is not expired.
2669-
Item& item = *itr;
2670-
const bool evictToNvmCache = shouldWriteToNvmCache(item);
2671-
2672-
auto token = evictToNvmCache ? nvmCache_->createPutToken(item.getKey())
2673-
: typename NvmCacheT::PutToken{};
2674-
2675-
// record the in-flight eviciton. If not, we move on to next item to avoid
2676-
// stalling eviction.
2677-
if (evictToNvmCache && !token.isValid()) {
2678-
++itr;
2679-
stats_.evictFailConcurrentFill.inc();
2680-
return WriteHandle{};
2681-
}
2682-
2683-
// If there are other accessors, we should abort. Acquire a handle here since
2684-
// if we remove the item from both access containers and mm containers
2685-
// below, we will need a handle to ensure proper cleanup in case we end up
2686-
// not evicting this item
2687-
auto evictHandle = accessContainer_->removeIf(item, &itemExclusivePredicate);
2688-
if (!evictHandle) {
2689-
++itr;
2690-
stats_.evictFailAC.inc();
2691-
return evictHandle;
2692-
}
2693-
2694-
mmContainer.remove(itr);
2695-
XDCHECK_EQ(reinterpret_cast<uintptr_t>(evictHandle.get()),
2696-
reinterpret_cast<uintptr_t>(&item));
2697-
XDCHECK(!evictHandle->isInMMContainer());
2698-
XDCHECK(!evictHandle->isAccessible());
2699-
2700-
// Invalidate iterator since later on if we are not evicting this
2701-
// item, we may need to rely on the handle we created above to ensure
2702-
// proper cleanup if the item's raw refcount has dropped to 0.
2703-
// And since this item may be a parent item that has some child items
2704-
// in this very same mmContainer, we need to make sure we drop this
2705-
// exclusive iterator so we can gain access to it when we're cleaning
2706-
// up the child items
2707-
itr.destroy();
2708-
2709-
// Ensure that there are no accessors after removing from the access
2710-
// container
2711-
XDCHECK(evictHandle->getRefCount() == 1);
2712-
2713-
if (evictToNvmCache && shouldWriteToNvmCacheExclusive(item)) {
2714-
XDCHECK(token.isValid());
2715-
nvmCache_->put(evictHandle, std::move(token));
2716-
}
2717-
return evictHandle;
2718-
}
2719-
2720-
template <typename CacheTrait>
2721-
typename CacheAllocator<CacheTrait>::WriteHandle
2722-
CacheAllocator<CacheTrait>::advanceIteratorAndTryEvictChainedItem(
2723-
EvictionIterator& itr) {
2724-
XDCHECK(itr->isChainedItem());
2725-
2726-
ChainedItem* candidate = &itr->asChainedItem();
2727-
++itr;
2728-
2729-
// The parent could change at any point through transferChain. However, if
2730-
// that happens, we would realize that the releaseBackToAllocator return
2731-
// kNotRecycled and we would try another chained item, leading to transient
2732-
// failure.
2733-
auto& parent = candidate->getParentItem(compressor_);
2734-
2735-
const bool evictToNvmCache = shouldWriteToNvmCache(parent);
2736-
2737-
auto token = evictToNvmCache ? nvmCache_->createPutToken(parent.getKey())
2738-
: typename NvmCacheT::PutToken{};
2739-
2740-
// if token is invalid, return. iterator is already advanced.
2741-
if (evictToNvmCache && !token.isValid()) {
2742-
stats_.evictFailConcurrentFill.inc();
2743-
return WriteHandle{};
2744-
}
2745-
2746-
// check if the parent exists in the hashtable and refcount is drained.
2747-
auto parentHandle =
2748-
accessContainer_->removeIf(parent, &itemExclusivePredicate);
2749-
if (!parentHandle) {
2750-
stats_.evictFailParentAC.inc();
2751-
return parentHandle;
2752-
}
2753-
2754-
// Invalidate iterator since later on we may use the mmContainer
2755-
// associated with this iterator which cannot be done unless we
2756-
// drop this iterator
2757-
//
2758-
// This must be done once we know the parent is not nullptr.
2759-
// Since we can very well be the last holder of this parent item,
2760-
// which may have a chained item that is linked in this MM container.
2761-
itr.destroy();
2762-
2763-
// Ensure we have the correct parent and we're the only user of the
2764-
// parent, then free it from access container. Otherwise, we abort
2765-
XDCHECK_EQ(reinterpret_cast<uintptr_t>(&parent),
2766-
reinterpret_cast<uintptr_t>(parentHandle.get()));
2767-
XDCHECK_EQ(1u, parent.getRefCount());
2768-
2769-
removeFromMMContainer(*parentHandle);
2770-
2771-
XDCHECK(!parent.isInMMContainer());
2772-
XDCHECK(!parent.isAccessible());
2773-
2774-
if (evictToNvmCache && shouldWriteToNvmCacheExclusive(*parentHandle)) {
2775-
XDCHECK(token.isValid());
2776-
XDCHECK(parentHandle->hasChainedItem());
2777-
nvmCache_->put(parentHandle, std::move(token));
2778-
}
2779-
2780-
return parentHandle;
2781-
}
2782-
27832709
template <typename CacheTrait>
27842710
typename CacheAllocator<CacheTrait>::WriteHandle
27852711
CacheAllocator<CacheTrait>::evictNormalItemForSlabRelease(Item& item) {
@@ -2812,7 +2738,7 @@ CacheAllocator<CacheTrait>::evictNormalItemForSlabRelease(Item& item) {
28122738
// now that we are the only handle and we actually removed something from
28132739
// the RAM cache, we enqueue it to nvmcache.
28142740
if (evictToNvmCache && shouldWriteToNvmCacheExclusive(item)) {
2815-
nvmCache_->put(handle, std::move(token));
2741+
nvmCache_->put(*handle, std::move(token));
28162742
}
28172743

28182744
return handle;
@@ -2913,7 +2839,7 @@ CacheAllocator<CacheTrait>::evictChainedItemForSlabRelease(ChainedItem& child) {
29132839
// the RAM cache, we enqueue it to nvmcache.
29142840
if (evictToNvmCache && shouldWriteToNvmCacheExclusive(*parentHandle)) {
29152841
DCHECK(parentHandle->hasChainedItem());
2916-
nvmCache_->put(parentHandle, std::move(token));
2842+
nvmCache_->put(*parentHandle, std::move(token));
29172843
}
29182844

29192845
return parentHandle;

‎cachelib/allocator/CacheAllocator.h‎

Lines changed: 6 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1659,6 +1659,12 @@ class CacheAllocator : public CacheBase {
16591659
bool removeFromNvm = true,
16601660
bool recordApiEvent = true);
16611661

1662+
// Must be called by the thread which called markForEviction and
1663+
// succeeded. After this call, the item is unlinked from Access and
1664+
// MM Containers. The item is no longer marked as exclusive and it's
1665+
// ref count is 0 - it's available for recycling.
1666+
void unlinkItemForEviction(Item& it);
1667+
16621668
// Implementation to find a suitable eviction from the container. The
16631669
// two parameters together identify a single container.
16641670
//
@@ -1669,25 +1675,6 @@ class CacheAllocator : public CacheBase {
16691675

16701676
using EvictionIterator = typename MMContainer::LockedIterator;
16711677

1672-
// Advance the current iterator and try to evict a regular item
1673-
//
1674-
// @param mmContainer the container to look for evictions.
1675-
// @param itr iterator holding the item
1676-
//
1677-
// @return valid handle to regular item on success. This will be the last
1678-
// handle to the item. On failure an empty handle.
1679-
WriteHandle advanceIteratorAndTryEvictRegularItem(MMContainer& mmContainer,
1680-
EvictionIterator& itr);
1681-
1682-
// Advance the current iterator and try to evict a chained item
1683-
// Iterator may also be reset during the course of this function
1684-
//
1685-
// @param itr iterator holding the item
1686-
//
1687-
// @return valid handle to the parent item on success. This will be the last
1688-
// handle to the item
1689-
WriteHandle advanceIteratorAndTryEvictChainedItem(EvictionIterator& itr);
1690-
16911678
// Deserializer CacheAllocatorMetadata and verify the version
16921679
//
16931680
// @param deserializer Deserializer object

0 commit comments

Comments
 (0)