Combined locking with exclusive bit #38

byrnedj · 2022-11-21T12:54:04Z

For leader and follower - I see about 20% performance improvement in throughput, for follower it is about 10% improvement.

This change is

(findEviction and evictForSlabRelease) but not for item movement. moveForSlabRelease relies on markMoving(). Only allow to mark item as exclusive if ref count is 0. This ensures that after item is marked eviction cannot fail. This makes it possible to return NULL handle immediately from find if item is marked as exclusive. markMoving() does have those restrictions and still allows readers to obtain a handle to a moving item. Also, add option to use combined locking for MMContainer iteration. Pass item ref to NavyCache::put

and just return the actual ref count

This simplifies moving code a lot. We don't need to worry about any failure other than allocation failure since item is protected from any readers (and by extension, removes).

igchor

Reviewed 1 of 20 files at r2.
Reviewable status: 0 of 29 files reviewed, 2 unresolved discussions (waiting on @byrnedj)

cachelib/allocator/CacheAllocator.h line 1968 at r3 (raw file):

  // exposed for the background evictor to iterate through the memory and evict
  // in batch. This should improve insertion path for tiered memory config
  size_t traverseAndEvictItems(unsigned int tid, unsigned int pid, unsigned int cid, size_t batch) {

this (and promotion function) should be adjusted and use similar logic as in findEviction (mark moving first, then exclusive if needed)

cachelib/allocator/CacheAllocator-inl.h line 3260 at r3 (raw file):

      token = createPutToken(*evicted);
      if (evicted->markExclusiveWhenMoving()) {
        unlinkItemExclusive(*evicted);

I think we need to wakeUp any waiters here as well (similarly to findEviction).
We should probably do this only for mutli-tier case. And for single-tier we might allow readers for moving items (pass false to incRef() call in acquire for single tier)

byrnedj

Reviewable status: 0 of 29 files reviewed, 2 unresolved discussions (waiting on @igchor)

cachelib/allocator/CacheAllocator.h line 1968 at r3 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

this (and promotion function) should be adjusted and use similar logic as in findEviction (mark moving first, then exclusive if needed)

Done.

cachelib/allocator/CacheAllocator-inl.h line 3260 at r3 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

I think we need to wakeUp any waiters here as well (similarly to findEviction).
We should probably do this only for mutli-tier case. And for single-tier we might allow readers for moving items (pass false to incRef() call in acquire for single tier)

Done

igchor

Reviewed 2 of 2 files at r4.
Reviewable status: 2 of 29 files reviewed, 2 unresolved discussions (waiting on @byrnedj)

cachelib/allocator/CacheAllocator-inl.h line 3260 at r3 (raw file):

Previously, byrnedj (Daniel Byrne) wrote…

Done

How about chained items? Will you wakeUpWaiters for them then? Perhpas just add some TODO now?

cachelib/allocator/CacheAllocator-inl.h line 1040 at r4 (raw file):

  SCOPE_FAIL { stats_.numRefcountOverflow.inc(); };

  auto failIfMoving = getNumTiers() > 1 ? true : false;

nit: you can drop the ? true : false part ;d

cachelib/allocator/CacheAllocator-inl.h line 1362 at r4 (raw file):

  // Adding the item to mmContainer has to succeed since no one can remove the item
  auto& newContainer = getMMContainer(*newItemHdl);
  auto mmContainerAdded = newContainer.add(*newItemHdl);

Hm, I think this should still be replace after all. Then, you don't nee to call removeFromMMContainer in traverseAndPromoteItems

…during eviction: -if marked moving, then another thread can still acquire handle to the item (because in single tier we don't have wait context, an item can acquire handle) -then we will later fail to markExclusiveWhenMoving during the eviction process because there is an outstanding handle -> made it so that in single tier (or when evicting from last tier) we always just mark it as exclusive so that there will be no threads that can get a valid handle.

byrnedj

Reviewable status: 1 of 29 files reviewed, 2 unresolved discussions (waiting on @igchor)

cachelib/allocator/CacheAllocator-inl.h line 3260 at r3 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

How about chained items? Will you wakeUpWaiters for them then? Perhpas just add some TODO now?

Added for now - chained item PR will address any additional logic.

cachelib/allocator/CacheAllocator-inl.h line 1040 at r4 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

nit: you can drop the ? true : false part ;d

done

cachelib/allocator/CacheAllocator-inl.h line 1362 at r4 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

Hm, I think this should still be replace after all. Then, you don't nee to call removeFromMMContainer in traverseAndPromoteItems

I am not sure because replaceInMMContainer will only replace if oldItem exists in mmContainer due to the short circuit evaluation. But here we have already removed the oldItem from mmContainer in most cases (expect for promotion case).

igchor

Reviewable status: 1 of 29 files reviewed, 1 unresolved discussion (waiting on @byrnedj)

cachelib/allocator/CacheAllocator.h line 2083 at r6 (raw file):

      } else {
	 //we failed to allocate a new item, this item is no  longer moving
	candidate->unmarkMoving();

I think you need to handle possible ref==0 case here as well.

cachelib/allocator/CacheAllocator-inl.h line 1362 at r4 (raw file):

Previously, byrnedj (Daniel Byrne) wrote…

I am not sure because replaceInMMContainer will only replace if oldItem exists in mmContainer due to the short circuit evaluation. But here we have already removed the oldItem from mmContainer in most cases (expect for promotion case).

Ok, I see, but we still need to make sure the oldItem is removed from the MMContainer. Perhaps some assert in findEviction and BGEvciction then? And I'm not sure if ChainedItem support won't require any changes

byrnedj

Reviewable status: 0 of 30 files reviewed, 1 unresolved discussion (waiting on @igchor)

cachelib/allocator/CacheAllocator.h line 2083 at r6 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

I think you need to handle possible ref==0 case here as well.

Done.

cachelib/allocator/CacheAllocator-inl.h line 1362 at r4 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

Ok, I see, but we still need to make sure the oldItem is removed from the MMContainer. Perhaps some assert in findEviction and BGEvciction then? And I'm not sure if ChainedItem support won't require any changes

remove with the iterator returns void - removedLock in mmContainer does not fail so I think we can assume that it succeeded.

igchor

Reviewable status: 0 of 30 files reviewed, all discussions resolved

- transparent item movement - multi-tier combined locking with exclusive bit (#38) with refactored incRef to support returning the result of markMoving (fail if already moving or exclusvie bit is set) option. - add tests (updated for numa bindings - post combined locking) for transparent item movement

Return a sum of sizes of each tier instead of just 1st tier's size.

- transparent item movement - multi-tier combined locking with exclusive bit (#38) with refactored incRef to support returning the result of markMoving (fail if already moving or exclusvie bit is set) option. - add tests (updated for numa bindings - post combined locking) for transparent item movement

igchor and others added 4 commits November 7, 2022 11:12

Hide extra ref when item is marked moving

ccd35ab

and just return the actual ref count

enable combined locking with exclusive bit

ed069fa

headers + test fix

eaf1f35

byrnedj requested a review from igchor November 21, 2022 12:54

igchor force-pushed the cs_part_2 branch 2 times, most recently from 8e33d00 to d351b6d Compare November 22, 2022 00:36

Block readers when item is moving

a30f2ac

This simplifies moving code a lot. We don't need to worry about any failure other than allocation failure since item is protected from any readers (and by extension, removes).

igchor force-pushed the cs_part_2 branch 2 times, most recently from 3e6ec39 to a30f2ac Compare November 22, 2022 17:21

igchor requested changes Nov 22, 2022

View reviewed changes

updated per comments

3040d37

byrnedj commented Nov 22, 2022

View reviewed changes

igchor requested changes Nov 23, 2022

View reviewed changes

byrnedj added 2 commits November 30, 2022 14:53

updated per comment

f5e5a5d

byrnedj commented Nov 30, 2022

View reviewed changes

igchor requested changes Nov 30, 2022

View reviewed changes

byrnedj added 2 commits November 30, 2022 19:50

formatting

564b074

avoid item leak & fix refcount test & disable nvm cache tests for ci

369defe

byrnedj commented Dec 2, 2022

View reviewed changes

igchor approved these changes Dec 2, 2022

View reviewed changes

igchor merged commit 7aed493 into intel:develop Dec 2, 2022

byrnedj pushed a commit that referenced this pull request Jul 23, 2023

Fixed total cache size in CacheMemoryStats (#38)

0883a5a

Return a sum of sizes of each tier instead of just 1st tier's size.

byrnedj pushed a commit that referenced this pull request Jul 23, 2023

Fixed total cache size in CacheMemoryStats (#38)

172caf1

Return a sum of sizes of each tier instead of just 1st tier's size.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Combined locking with exclusive bit #38

Combined locking with exclusive bit #38

Uh oh!

byrnedj commented Nov 21, 2022 •

edited by rdower

Loading

Uh oh!

igchor left a comment

Uh oh!

byrnedj left a comment

Uh oh!

igchor left a comment

Uh oh!

byrnedj left a comment

Uh oh!

igchor left a comment

Uh oh!

byrnedj left a comment

Uh oh!

igchor left a comment

Uh oh!

Uh oh!

Combined locking with exclusive bit #38

Combined locking with exclusive bit #38

Uh oh!

Conversation

byrnedj commented Nov 21, 2022 • edited by rdower Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

igchor left a comment

Choose a reason for hiding this comment

Uh oh!

byrnedj left a comment

Choose a reason for hiding this comment

Uh oh!

igchor left a comment

Choose a reason for hiding this comment

Uh oh!

byrnedj left a comment

Choose a reason for hiding this comment

Uh oh!

igchor left a comment

Choose a reason for hiding this comment

Uh oh!

byrnedj left a comment

Choose a reason for hiding this comment

Uh oh!

igchor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

byrnedj commented Nov 21, 2022 •

edited by rdower

Loading