Skip to content

Commit d16fb0e

Browse files
igchorbyrnedj
igchor
authored and
byrnedj
committed
Chained item movement between tiers - currently sync on the parent
item for moving. - updated tests accordingly, note that we can no longer swap parent item if chained item is being moved for slab release.
1 parent 964c759 commit d16fb0e

File tree

16 files changed

+560
-517
lines changed

16 files changed

+560
-517
lines changed

‎cachelib/allocator/CacheAllocator-inl.h‎

Lines changed: 415 additions & 423 deletions
Large diffs are not rendered by default.

‎cachelib/allocator/CacheAllocator.h‎

Lines changed: 46 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1387,7 +1387,7 @@ class CacheAllocator : public CacheBase {
13871387

13881388
private:
13891389
// wrapper around Item's refcount and active handle tracking
1390-
FOLLY_ALWAYS_INLINE RefcountWithFlags::incResult incRef(Item& it, bool failIfMoving);
1390+
FOLLY_ALWAYS_INLINE RefcountWithFlags::incResult incRef(Item& it);
13911391
FOLLY_ALWAYS_INLINE RefcountWithFlags::Value decRef(Item& it);
13921392

13931393
// drops the refcount and if needed, frees the allocation back to the memory
@@ -1545,6 +1545,26 @@ class CacheAllocator : public CacheBase {
15451545
WriteHandle allocateChainedItemInternal(const ReadHandle& parent,
15461546
uint32_t size);
15471547

1548+
// Allocate a chained item to a specific tier
1549+
//
1550+
// The resulting chained item does not have a parent item yet
1551+
// and if we fail to link to the chain for any reasoin
1552+
// the chained item will be freed once the handle is dropped.
1553+
//
1554+
// The parent item parameter here is mainly used to find the
1555+
// correct pool to allocate memory for this chained item
1556+
//
1557+
// @param parent parent item
1558+
// @param size the size for the chained allocation
1559+
// @param tid the tier to allocate on
1560+
//
1561+
// @return handle to the chained allocation
1562+
// @throw std::invalid_argument if the size requested is invalid or
1563+
// if the item is invalid
1564+
WriteHandle allocateChainedItemInternalTier(const Item& parent,
1565+
uint32_t size,
1566+
TierId tid);
1567+
15481568
// Given an item and its parentKey, validate that the parentKey
15491569
// corresponds to an item that's the parent of the supplied item.
15501570
//
@@ -1620,19 +1640,17 @@ class CacheAllocator : public CacheBase {
16201640
//
16211641
// @return true If the move was completed, and the containers were updated
16221642
// successfully.
1623-
bool moveRegularItemWithSync(Item& oldItem, WriteHandle& newItemHdl);
1643+
bool moveRegularItem(Item& oldItem, WriteHandle& newItemHdl);
16241644

1625-
// Moves a regular item to a different slab. This should only be used during
1626-
// slab release after the item's exclusive bit has been set. The user supplied
1627-
// callback is responsible for copying the contents and fixing the semantics
1628-
// of chained item.
1645+
// Moves a chained item to a different memory tier.
16291646
//
1630-
// @param oldItem item being moved
1647+
// @param oldItem Reference to the item being moved
16311648
// @param newItemHdl Reference to the handle of the new item being moved into
1649+
// @param parentHandle Reference to the handle of the parent item
16321650
//
16331651
// @return true If the move was completed, and the containers were updated
16341652
// successfully.
1635-
bool moveRegularItem(Item& oldItem, WriteHandle& newItemHdl);
1653+
bool moveChainedItem(ChainedItem& oldItem, WriteHandle& newItemHdl, Item& parentItem);
16361654

16371655
// template class for viewAsChainedAllocs that takes either ReadHandle or
16381656
// WriteHandle
@@ -1645,29 +1663,12 @@ class CacheAllocator : public CacheBase {
16451663
template <typename Handle>
16461664
folly::IOBuf convertToIOBufT(Handle& handle);
16471665

1648-
// Moves a chained item to a different slab. This should only be used during
1649-
// slab release after the item's exclusive bit has been set. The user supplied
1650-
// callback is responsible for copying the contents and fixing the semantics
1651-
// of chained item.
1652-
//
1653-
// Note: If we have successfully moved the old item into the new, the
1654-
// newItemHdl is reset and no longer usable by the caller.
1655-
//
1656-
// @param oldItem Reference to the item being moved
1657-
// @param newItemHdl Reference to the handle of the new item being
1658-
// moved into
1659-
//
1660-
// @return true If the move was completed, and the containers were updated
1661-
// successfully.
1662-
bool moveChainedItem(ChainedItem& oldItem, WriteHandle& newItemHdl);
1663-
16641666
// Transfers the chain ownership from parent to newParent. Parent
16651667
// will be unmarked as having chained allocations. Parent will not be null
16661668
// after calling this API.
16671669
//
1668-
// Parent and NewParent must be valid handles to items with same key and
1669-
// parent must have chained items and parent handle must be the only
1670-
// outstanding handle for parent. New parent must be without any chained item
1670+
// NewParent must be valid handles to item with same key as Parent and
1671+
// Parent must have chained items. New parent must be without any chained item
16711672
// handles.
16721673
//
16731674
// Chained item lock for the parent's key needs to be held in exclusive mode.
@@ -1676,7 +1677,7 @@ class CacheAllocator : public CacheBase {
16761677
// @param newParent the new parent for the chain
16771678
//
16781679
// @throw if any of the conditions for parent or newParent are not met.
1679-
void transferChainLocked(WriteHandle& parent, WriteHandle& newParent);
1680+
void transferChainLocked(Item& parent, WriteHandle& newParent);
16801681

16811682
// replace a chained item in the existing chain. This needs to be called
16821683
// with the chained item lock held exclusive
@@ -1690,6 +1691,21 @@ class CacheAllocator : public CacheBase {
16901691
WriteHandle newItemHdl,
16911692
const Item& parent);
16921693

1694+
//
1695+
// Performs the actual inplace replace - it is called from
1696+
// moveChainedItem and replaceChainedItemLocked
1697+
// must hold chainedItemLock
1698+
//
1699+
// @param oldItem the item we are replacing in the chain
1700+
// @param newItem the item we are replacing it with
1701+
// @param parent the parent for the chain
1702+
//
1703+
// @return handle to the oldItem
1704+
void replaceInChainLocked(Item& oldItem,
1705+
WriteHandle& newItemHdl,
1706+
const Item& parent,
1707+
bool fromMove);
1708+
16931709
// Insert an item into MM container. The caller must hold a valid handle for
16941710
// the item.
16951711
//
@@ -1980,7 +1996,7 @@ auto& mmContainer = getMMContainer(tid, pid, cid);
19801996
throw std::runtime_error("Not supported for chained items");
19811997
}
19821998

1983-
if (candidate->markMoving(true)) {
1999+
if (candidate->markMoving()) {
19842000
mmContainer.remove(itr);
19852001
candidates.push_back(candidate);
19862002
} else {
@@ -2053,7 +2069,7 @@ auto& mmContainer = getMMContainer(tid, pid, cid);
20532069

20542070
// TODO: only allow it for read-only items?
20552071
// or implement mvcc
2056-
if (candidate->markMoving(true)) {
2072+
if (candidate->markMoving()) {
20572073
// promotions should rarely fail since we already marked moving
20582074
mmContainer.remove(itr);
20592075
candidates.push_back(candidate);

‎cachelib/allocator/CacheItem-inl.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -238,8 +238,8 @@ bool CacheItem<CacheTrait>::markForEvictionWhenMoving() {
238238
}
239239

240240
template <typename CacheTrait>
241-
bool CacheItem<CacheTrait>::markMoving(bool failIfRefNotZero) {
242-
return ref_.markMoving(failIfRefNotZero);
241+
bool CacheItem<CacheTrait>::markMoving() {
242+
return ref_.markMoving();
243243
}
244244

245245
template <typename CacheTrait>

‎cachelib/allocator/CacheItem.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -312,9 +312,9 @@ class CACHELIB_PACKED_ATTR CacheItem {
312312
//
313313
// @return true on success, failure if item is marked as exclusive
314314
// @throw exception::RefcountOverflow on ref count overflow
315-
FOLLY_ALWAYS_INLINE RefcountWithFlags::incResult incRef(bool failIfMoving) {
315+
FOLLY_ALWAYS_INLINE RefcountWithFlags::incResult incRef() {
316316
try {
317-
return ref_.incRef(failIfMoving);
317+
return ref_.incRef();
318318
} catch (exception::RefcountOverflow& e) {
319319
throw exception::RefcountOverflow(
320320
folly::sformat("{} item: {}", e.what(), toString()));
@@ -381,7 +381,7 @@ class CACHELIB_PACKED_ATTR CacheItem {
381381
* Unmarking moving will also return the refcount at the moment of
382382
* unmarking.
383383
*/
384-
bool markMoving(bool failIfRefNotZero);
384+
bool markMoving();
385385
RefcountWithFlags::Value unmarkMoving() noexcept;
386386
bool isMoving() const noexcept;
387387
bool isOnlyMoving() const noexcept;

‎cachelib/allocator/MM2Q.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -503,6 +503,9 @@ class MM2Q {
503503
template <typename F>
504504
void withEvictionIterator(F&& f);
505505

506+
template <typename F>
507+
void withContainerLock(F&& f) { return; };
508+
506509
// Execute provided function under container lock. Function gets
507510
// iterator passed as parameter.
508511
template <typename F>

‎cachelib/allocator/MMLru-inl.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -218,6 +218,12 @@ MMLru::Container<T, HookPtr>::getEvictionIterator() const noexcept {
218218
return LockedIterator{std::move(l), lru_.rbegin()};
219219
}
220220

221+
template <typename T, MMLru::Hook<T> T::*HookPtr>
222+
template <typename F>
223+
void MMLru::Container<T, HookPtr>::withContainerLock(F&& fun) {
224+
lruMutex_->lock_combine([this, &fun]() { fun(); });
225+
}
226+
221227
template <typename T, MMLru::Hook<T> T::*HookPtr>
222228
template <typename F>
223229
void MMLru::Container<T, HookPtr>::withEvictionIterator(F&& fun) {

‎cachelib/allocator/MMLru.h‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -377,6 +377,11 @@ class MMLru {
377377
template <typename F>
378378
void withEvictionIterator(F&& f);
379379

380+
// Execute provided function under container lock. Function gets
381+
// iterator passed as parameter.
382+
template <typename F>
383+
void withContainerLock(F&& f);
384+
380385
template <typename F>
381386
void withPromotionIterator(F&& f);
382387

‎cachelib/allocator/MMTinyLFU.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -497,6 +497,9 @@ class MMTinyLFU {
497497
template <typename F>
498498
void withEvictionIterator(F&& f);
499499

500+
template <typename F>
501+
void withContainerLock(F&& f) { return; };
502+
500503
template <typename F>
501504
void withPromotionIterator(F&& f);
502505

‎cachelib/allocator/Refcount.h‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -140,9 +140,9 @@ class FOLLY_PACK_ATTR RefcountWithFlags {
140140
// @return true if refcount is bumped. false otherwise (if item is exclusive)
141141
// @throw exception::RefcountOverflow if new count would be greater than
142142
// maxCount
143-
FOLLY_ALWAYS_INLINE incResult incRef(bool failIfMoving) {
143+
FOLLY_ALWAYS_INLINE incResult incRef() {
144144
incResult res = incOk;
145-
auto predicate = [failIfMoving, &res](const Value curValue) {
145+
auto predicate = [&res](const Value curValue) {
146146
Value bitMask = getAdminRef<kExclusive>();
147147

148148
const bool exlusiveBitIsSet = curValue & bitMask;
@@ -151,7 +151,7 @@ class FOLLY_PACK_ATTR RefcountWithFlags {
151151
} else if (exlusiveBitIsSet && (curValue & kAccessRefMask) == 0) {
152152
res = incFailedEviction;
153153
return false;
154-
} else if (exlusiveBitIsSet && failIfMoving) {
154+
} else if (exlusiveBitIsSet) {
155155
res = incFailedMoving;
156156
return false;
157157
}
@@ -320,12 +320,15 @@ class FOLLY_PACK_ATTR RefcountWithFlags {
320320
*
321321
* Unmarking moving does not depend on `isInMMContainer`
322322
*/
323-
bool markMoving(bool failIfRefNotZero) {
324-
auto predicate = [failIfRefNotZero](const Value curValue) {
323+
bool markMoving() {
324+
auto predicate = [](const Value curValue) {
325325
Value conditionBitMask = getAdminRef<kLinked>();
326326
const bool flagSet = curValue & conditionBitMask;
327327
const bool alreadyExclusive = curValue & getAdminRef<kExclusive>();
328-
if (failIfRefNotZero && (curValue & kAccessRefMask) != 0) {
328+
const bool isChained = curValue & getFlag<kIsChainedItem>();
329+
330+
// chained item can have ref count == 1, this just means it's linked in the chain
331+
if ((curValue & kAccessRefMask) > isChained ? 1 : 0) {
329332
return false;
330333
}
331334
if (!flagSet || alreadyExclusive) {

‎cachelib/allocator/tests/AllocatorTypeTest.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -288,8 +288,8 @@ TYPED_TEST(BaseAllocatorTest, AddChainedItemMultiThreadWithMovingAndSync) {
288288
this->testAddChainedItemMultithreadWithMovingAndSync();
289289
}
290290

291-
TYPED_TEST(BaseAllocatorTest, TransferChainWhileMoving) {
292-
this->testTransferChainWhileMoving();
291+
TYPED_TEST(BaseAllocatorTest, TransferChainAfterMoving) {
292+
this->testTransferChainAfterMoving();
293293
}
294294

295295
TYPED_TEST(BaseAllocatorTest, AddAndPopChainedItemMultithread) {

0 commit comments

Comments
 (0)