Async eviction #16

vinser52 · 2021-11-29T21:03:32Z

This change is

igchor

Reviewable status: 0 of 14 files reviewed, 6 unresolved discussions (waiting on @igchor and @vinser52)

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

                                                                   // waiters
  };
  using MoveMap = folly::F14ValueMap<folly::StringPiece, std::unique_ptr<MoveCtx>, folly::HeterogeneousAccessHash<folly::StringPiece>>;

Why not use some concurrent map from folly instead of sharding?

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

  static constexpr size_t kShards = 8192; // TODO: need to define right value

unnecessary whitespace

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

template <typename CacheTrait>
bool CacheAllocator<CacheTrait>::addWaitContextForMovingItem(

So, right now, on find if element is not ready we register a waiter and once item is moved between tiers and access container is updated the waiter is waken up? I think it should be described somewhere in comment (make next to a movesMap_ declaration?

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

template <typename CacheTrait>
bool CacheAllocator<CacheTrait>::addWaitContextForMovingItem(
  folly::StringPiece key,

please align function parameter similarly as in other functions (here and in rest of the methods)

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

    auto res = movesMap.try_emplace(key, std::make_unique<MoveCtx>());
    if(!res.second) {
      return false;

If this is false, there is already some other move in progress - this can happen only if there are several concurrent calls to tryEvictToNextMemoryTier, right? In that case, can't we just fail earlier by e.g. checking the notReady flag?

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

  auto guard = folly::makeGuard([key, this, ctx, shard, &resHdl]() {
      auto& movesMap = getMoveMapForShard(shard);
      resHdl->unmarkNotReady();

Is it correct? From Handle.h : It's a bug to set a hdl that's already ready and can // terminate the application. and if I understand correctly set is called when we do movesMap.erase(key); (so after marking resHndl as ready)

... or this notReady has nothing to do with isReady() method? if so, I would suggest renaming this

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

    config_.moveCb(oldItem, *newItemHdl, nullptr);
  } else {
    std::memcpy(newItemHdl->getWritableMemory(), oldItem.getMemory(), oldItem.getSize());

I think we should not have a default fallback - in my opinion, we should require having moveCb set by the user. It can still be a regular memcpy but it should be explicit. Otherwise, we will break current behavior which guarantees that items are not moved without any synchronization with the user.

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

        return acquire(&item);
      }
      // TODO: should we free the newItemHdl if moveRegularItemOnEviction returns false???

I think so

cachelib/allocator/Handle.h, line 471 at r3 (raw file):

      waitContext_ = std::make_shared<ItemWaitContext>(alloc);
      if(!alloc_->addWaitContextForMovingItem(it->getKey(), waitContext_)) {
        waitContext_.reset();

why do you need this reset?

vinser52

Reviewable status: 0 of 14 files reviewed, 6 unresolved discussions (waiting on @igchor and @vinser52)

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

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

Why not use some concurrent map from folly instead of sharding?

It was my initial idea. But folly::ConcurrentHashMap does not provide an interface that allows to modify mapped value in place. In our use case, we need to add waiters to the MoveCtx. But find method returns only a const iterator. There is assign method that allows to replace existing mapped value with the new one - it is not what we need as well.

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

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

So, right now, on find if element is not ready we register a waiter and once item is moved between tiers and access container is updated the waiter is waken up? I think it should be described somewhere in comment (make next to a movesMap_ declaration?

The logic is the following.
The thread which moves Item:

Create MoveCtx and put it to the movesMap.
Updates the access container with the new item from the tier we are moving to. This Item has kNotReady flag set.
Copy data from the old Item to the new one.
Unset thekNotReady flag and Notify MoveCtx

Concurrent threads which are getting handle to the same key:

When a handle is created it checks if the kNotReady flag is set
If so, Handle implementation creates waitContext and adds it to the MoveCtx by accessing movesMap.
Wait until the moving thread will complete its job.

vinser52

Reviewable status: 0 of 14 files reviewed, 6 unresolved discussions (waiting on @igchor and @vinser52)

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

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

unnecessary whitespace

Removed

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

Previously, vinser52 (Sergei Vinogradov) wrote…

The logic is the following.
The thread which moves Item:

Create MoveCtx and put it to the movesMap.

Updates the access container with the new item from the tier we are moving to. This Item has kNotReady flag set.

Copy data from the old Item to the new one.

Unset thekNotReady flag and Notify MoveCtx

Concurrent threads which are getting handle to the same key:

When a handle is created it checks if the kNotReady flag is set

If so, Handle implementation creates waitContext and adds it to the MoveCtx by accessing movesMap.

Wait until the moving thread will complete its job.

I have added comment.

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

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

please align function parameter similarly as in other functions (here and in rest of the methods)

Done

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

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

If this is false, there is already some other move in progress - this can happen only if there are several concurrent calls to tryEvictToNextMemoryTier, right? In that case, can't we just fail earlier by e.g. checking the notReady flag?

notReady flag is set for the new Item (we are moving to). At this point, we have not updated accessContainer yet, because before updating accessContainer we need to create MoveCtx and put it to movesMap. When we updating accessContainer the new Item become visible to other threads. Concurrent threads that access the new Item should create waitContext and add it to corresponding MoveCtx.

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

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

I think we should not have a default fallback - in my opinion, we should require having moveCb set by the user. It can still be a regular memcpy but it should be explicit. Otherwise, we will break current behavior which guarantees that items are not moved without any synchronization with the user.

Should we add some checks to config if multiple tiers are configured but the callback is not set?

cachelib/allocator/Handle.h, line 471 at r3 (raw file):

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

why do you need this reset?

The addWaitContextForMovingItem() returns false if the concurrent thread that do eviction already done and destroyed MoveCtx and cleaned up the movesMap. It means that we cannot register waitContext so we should just destroy it.

vinser52

Reviewable status: 0 of 14 files reviewed, 6 unresolved discussions (waiting on @igchor and @vinser52)

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

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

I think so

I just check the code. The destructor of HandleImplcalls reset() method which call void CacheAllocator<CacheTrait>::release(Item* it, bool isNascent) method. In case refcount to the Item is 0 the releaseBackToAllocator() method is called. So, I think we should not do anything in that case. Are you agree?

vinser52

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

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

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

Is it correct? From Handle.h : It's a bug to set a hdl that's already ready and can // terminate the application. and if I understand correctly set is called when we do movesMap.erase(key); (so after marking resHndl as ready)

... or this notReady has nothing to do with isReady() method? if so, I would suggest renaming this

Yeah, I think naming is confusing in this case. The method you are referring is related to waitContext of the Handle object while isNotRedy flag is set to the Item - it indicates that Item is not ready when new Handle by concurrent thread is creted.

victoria-mcgrath · 2021-12-02T01:54:06Z

cachelib/allocator/CacheAllocator-inl.h, line 1106 at r7 (raw file):

/* Next two methods are used to asynchronously move Item between memory tiers.
 *
 * The thread, which moves Item allocate, new Item in the tier we are moving to

Should comma go before "allocate"?

victoria-mcgrath · 2021-12-02T01:55:30Z

cachelib/allocator/CacheAllocator-inl.h, line 1109 at r7 (raw file):

 * and calls moveRegularItemOnEviction() method. This method does the following:
 *  1. Create MoveCtx and put it to the movesMap.
 *  2. Updates the access container with the new item from the tier we are

"Update"? For consistency with the rest of the bullets.

victoria-mcgrath · 2021-12-02T01:55:51Z

cachelib/allocator/CacheAllocator-inl.h, line 1113 at r7 (raw file):

 *  3. Copy data from the old Item to the new one.
 *  4. Unset thekNotReady flag and Notify MoveCtx
 *

Extra spaces.

victoria-mcgrath · 2021-12-02T02:06:51Z

cachelib/allocator/CacheAllocator-inl.h, line 1106 at r7 (raw file):

Previously, victoria-mcgrath wrote…

Should comma go before "allocate"?

"allocates"

victoria-mcgrath · 2021-12-02T02:08:02Z

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

Previously, vinser52 (Sergei Vinogradov) wrote…

It was my initial idea. But folly::ConcurrentHashMap does not provide an interface that allows to modify mapped value in place. In our use case, we need to add waiters to the MoveCtx. But find method returns only a const iterator. There is assign method that allows to replace existing mapped value with the new one - it is not what we need as well.

Is extending an existing class an option?

victoria-mcgrath

Reviewed 6 of 8 files at r1, 1 of 2 files at r5, 3 of 7 files at r6, 4 of 4 files at r7, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @igchor and @vinser52)

igchor

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @vinser52)

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

Previously, vinser52 (Sergei Vinogradov) wrote…

Yeah, I think naming is confusing in this case. The method you are referring is related to waitContext of the Handle object while isNotRedy flag is set to the Item - it indicates that Item is not ready when new Handle by concurrent thread is creted.

I see, so maybe something like markNotVisible or markIncomplete?

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

Previously, vinser52 (Sergei Vinogradov) wrote…

Should we add some checks to config if multiple tiers are configured but the callback is not set?

Yeah, I think so, we could just throw an error in that case

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

Previously, vinser52 (Sergei Vinogradov) wrote…

I just check the code. The destructor of HandleImplcalls reset() method which call void CacheAllocator<CacheTrait>::release(Item* it, bool isNascent) method. In case refcount to the Item is 0 the releaseBackToAllocator() method is called. So, I think we should not do anything in that case. Are you agree?

Ok, makes sense, what about the case where we managed to insert the item into accessContainer but failed updating MMContainer? WHat should happen with such element?

vinser52

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @igchor and @vinser52)

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

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

Yeah, I think so, we could just throw an error in that case

I am thinking that it is not an easy question. For example, eviction (when we campletly remove Item from the cache) provides callback option that user can set to be notified when an Item is evicted from the cache. But this callback is not mandatory. So now I do not see significant difference between moving Item between tiers and eviction from the cache.

vinser52

Reviewable status: 7 of 14 files reviewed, 5 unresolved discussions (waiting on @igchor and @victoria-mcgrath)

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

Previously, victoria-mcgrath wrote…

Is extending an existing class an option?

I believe it was designed in such a way to address possible concurrency issues. Adding a new interface will require changing the implementation.

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

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

I see, so maybe something like markNotVisible or markIncomplete?

possibly "Incomplete" is a better choice. Because Item is visible (and it is our intention) but data not copied from the old location yet.

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

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

Ok, makes sense, what about the case where we managed to insert the item into accessContainer but failed updating MMContainer? WHat should happen with such element?

In that case, Item is removed from the accessContainer by the moveRegularItemOnEviction method.
I have removed TODO comment

cachelib/allocator/CacheAllocator-inl.h, line 1106 at r7 (raw file):

Previously, victoria-mcgrath wrote…

"allocates"

Done.

cachelib/allocator/CacheAllocator-inl.h, line 1109 at r7 (raw file):

Previously, victoria-mcgrath wrote…

"Update"? For consistency with the rest of the bullets.

Done.

cachelib/allocator/CacheAllocator-inl.h, line 1113 at r7 (raw file):

Previously, victoria-mcgrath wrote…

Extra spaces.

Done.

Now it's size is 8 bytes intead of 4. Original CompressedPtr stored only some offset with a memory Allocator. For multi-tier implementation, this is not enough. We must also store tierId and when uncompressing, select a proper allocator. An alternative could be to just resign from CompressedPtr but they are leveraged to allow the cache to be mapped to different addresses on shared memory. Changing CompressedPtr impacted CacheItem size - it increased from 32 to 44 bytes.

igchor · 2021-12-31T09:19:29Z

Changes already merged into develop.

vinser52 requested a review from igchor November 29, 2021 21:03

igchor requested changes Nov 30, 2021

View reviewed changes

vinser52 force-pushed the async_eviction branch from 78eed16 to 6a1fc2a Compare November 30, 2021 17:57

vinser52 commented Nov 30, 2021

View reviewed changes

vinser52 force-pushed the async_eviction branch from 6a1fc2a to 0431837 Compare December 1, 2021 11:57

vinser52 commented Dec 1, 2021

View reviewed changes

victoria-mcgrath reviewed Dec 2, 2021

View reviewed changes

igchor requested changes Dec 3, 2021

View reviewed changes

vinser52 commented Dec 3, 2021

View reviewed changes

vinser52 force-pushed the async_eviction branch from 0431837 to f205398 Compare December 7, 2021 21:16

vinser52 commented Dec 7, 2021

View reviewed changes

vinser52 force-pushed the async_eviction branch 4 times, most recently from 46c6487 to 36ce841 Compare December 15, 2021 12:18

vinser52 force-pushed the async_eviction branch 5 times, most recently from 12b469c to 61bdc55 Compare December 17, 2021 15:59

igchor and others added 3 commits December 17, 2021 20:47

Initial multi-tier support implementation

ccbb618

Run CI on prebuild docker image

d231814

Run only centos build on CI

aa9a5c1

igchor mentioned this pull request Dec 20, 2021

Async eviction #21

Merged

vinser52 added 2 commits December 21, 2021 17:31

Implemented async Item movement between tiers

2afeaef

Adding example for multitiered cache

0840066

vinser52 force-pushed the async_eviction branch from 61bdc55 to 0840066 Compare December 21, 2021 14:31

igchor closed this Dec 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Async eviction #16

Async eviction #16

Uh oh!

vinser52 commented Nov 29, 2021 •

edited by igchor

Loading

Uh oh!

igchor left a comment

Uh oh!

vinser52 left a comment

Uh oh!

vinser52 left a comment

Uh oh!

vinser52 left a comment

Uh oh!

vinser52 left a comment

Uh oh!

victoria-mcgrath commented Dec 2, 2021

Uh oh!

victoria-mcgrath commented Dec 2, 2021

Uh oh!

victoria-mcgrath commented Dec 2, 2021

Uh oh!

victoria-mcgrath commented Dec 2, 2021

Uh oh!

victoria-mcgrath commented Dec 2, 2021

Uh oh!

victoria-mcgrath left a comment

Uh oh!

igchor left a comment

Uh oh!

vinser52 left a comment

Uh oh!

vinser52 left a comment

Uh oh!

igchor commented Dec 31, 2021

Uh oh!

Uh oh!

Async eviction #16

Async eviction #16

Uh oh!

Conversation

vinser52 commented Nov 29, 2021 • edited by igchor 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!

vinser52 left a comment

Choose a reason for hiding this comment

Uh oh!

vinser52 left a comment

Choose a reason for hiding this comment

Uh oh!

vinser52 left a comment

Choose a reason for hiding this comment

Uh oh!

vinser52 left a comment

Choose a reason for hiding this comment

Uh oh!

victoria-mcgrath commented Dec 2, 2021

Uh oh!

victoria-mcgrath commented Dec 2, 2021

Uh oh!

victoria-mcgrath commented Dec 2, 2021

Uh oh!

victoria-mcgrath commented Dec 2, 2021

Uh oh!

victoria-mcgrath commented Dec 2, 2021

Uh oh!

victoria-mcgrath 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!

vinser52 left a comment

Choose a reason for hiding this comment

Uh oh!

vinser52 left a comment

Choose a reason for hiding this comment

Uh oh!

igchor commented Dec 31, 2021

Uh oh!

Uh oh!

vinser52 commented Nov 29, 2021 •

edited by igchor

Loading