Critical section part 2: findEviction optimization and code unification #172

igchor · 2022-11-07T20:22:14Z

Previous PR (#166) refactored code in findEviction so it would be easier to add support for multiple memory tiers.

Now, we have two possible approaches to move forward with adding multi-tier support:

Implement item movement between tiers as a separate code path in findEviction
Unify eviction and item movement (for single-tier, multi-tier and SlabRelease)

This PR implements the second approach. It unifies logic for item movement and eviction, allowing us to later reuse SlabRelease item movement logic for multi-tier implementation. This PR also enables the use of combined locking for MMContainer. It allows us to achieve much better performance in multi-tier scenario.

We have also observed 2X throughput improvement for leader benchmarks (on the upstream, single-tier version) with this PR. We have also seen an improvement in allocation latency. However, I have to admit that this patch is pretty complex. Please let me know if you're open to merging this or if you'd prefer not to modify so much code.

It would be great if you could validate those changes internally. I encountered one inconsistency when running consistency/navy.json benchmark but it's pretty rare (one per 1200M ops) and I'm not able to debug this. We'd like to validate those changes by running navy tests but unfortunately they are failing even on main branch: #169

igchor · 2022-11-07T20:23:56Z

cachelib/allocator/CacheAllocator-inl.h

-  if (evictToNvmCache && shouldWriteToNvmCacheExclusive(item)) {
-    nvmCache_->put(handle, std::move(token));
-  }
+      // search if the child is present in the chain


I'm not really sure if this code for searching for a child is needed... I copied it from the original implementation of SlabRelease eviction logic, but it seems to me like checking if the item's pointer to the parent is valid should be enough.

haowu14

I have only reviewed Refcount.h , NvmCache-inl.h and some of the eviction iterator change and have some questions.

haowu14 · 2022-12-07T05:40:18Z

cachelib/allocator/Refcount.h

+
+    return markInternal(predicate, newValue);
+  }
+  Value unmarkMoving() noexcept {


nit: empty line between functions.

haowu14 · 2022-12-07T06:01:10Z

cachelib/allocator/Refcount.h

      const Value newCount = oldVal + static_cast<Value>(1);
      if (UNLIKELY((oldVal & kAccessRefMask) == (kAccessRefMask))) {
+        throw exception::RefcountOverflow("Refcount maxed out.");
+      }
+      if (alreadyExclusive && (oldVal & kAccessRefMask) == 0) {


Why adding (oldVal & kAccessRefMask) == 0? I thought we would only check alreadyExclusive here .

The idea is to distinguish between moving and forEviction. We only want to return false when the item is marked markedExclusive (marked for eviction).

I renamed the mark/unmark functions as you suggested so hopefully it makes more sense now.

haowu14 · 2022-12-07T06:02:20Z

cachelib/allocator/Refcount.h

@@ -192,8 +200,18 @@ class FOLLY_PACK_ATTR RefcountWithFlags {
    }
  }

-  // Return refcount excluding control bits and flags
-  Value getAccessRef() const noexcept { return getRaw() & kAccessRefMask; }
+  // Return refcount excluding control bits and flags.


Suggested change

// Return refcount excluding control bits and flags.

// Return refcount excluding moving refcount, control bits and flags.

haowu14 · 2022-12-07T06:12:40Z

cachelib/allocator/Refcount.h

-   * an item is currently in the process of being moved. This happens during a
-   * slab rebalance or resize operation or during eviction.
+   * The following two functions corresond to whether or not an item is
+   * currently in the process of being evicted. When item is marked exclsuive


What's your future plan for markExclusive()? Is it also used in a case other than eviction?
If not, I think we can rename this called markEviction() where markEviction means marking exclusive bit with access ref==0, isLinked and isAccessible.

Yes, I think that's a good idea. The markExlusive is indeed only used for the eviction case and we do not have any other plans for that.

haowu14 · 2022-12-07T06:14:21Z

cachelib/allocator/Refcount.h

+   * and item is not already exclusive nor moving.
+   *
+   * User can also query if an item "isOnlyMoving". This returns true only
+   * if the refcount is one and only the moving bit is set.


Can you remove the phrase "moving bit"? We don't really have a bit that's indicating moving. It's really isExclusive + access ref == 1.

Right, done. I also fixed this in other places.

haowu14 · 2022-12-07T06:21:15Z

cachelib/allocator/Refcount.h

    auto ref = getRefWithAccessAndAdmin();
-    bool anyOtherBitSet = ref & ~getAdminRef<kExclusive>();
-    if (anyOtherBitSet) {
+    Value valueWithoutMovingBit = ref & ~getAdminRef<kExclusive>();


Suggested change

Value valueWithoutMovingBit = ref & ~getAdminRef<kExclusive>();

Value valueWithoutExclusiveBit = ref & ~getAdminRef<kExclusive>();

haowu14 · 2022-12-07T06:21:40Z

cachelib/allocator/Refcount.h

+  }
+
+  bool isOnlyMoving() const noexcept {
+    // An item is only moving when its refcount is one and only the moving bit


Suggested change

// An item is only moving when its refcount is one and only the moving bit

// An item is only moving when its refcount is one and only the exclusive bit

haowu14 · 2022-12-07T06:22:44Z

cachelib/allocator/Refcount.h

+  // @return true if refcount is bumped. false otherwise (if item is exclusive)
+  // @throw  exception::RefcountOverflow if new count would be greater than
+  // maxCount
+  FOLLY_ALWAYS_INLINE bool incRef() {


Can you refactor this with markInternal?

haowu14 · 2022-12-07T06:23:04Z

cachelib/allocator/Refcount.h

@@ -370,6 +454,32 @@ class FOLLY_PACK_ATTR RefcountWithFlags {
  }

 private:
+  template <typename P, typename F>
+  bool markInternal(P&& predicate, F&& newValueF) {


Please add documentation.

Done. I also renamed the function since we now use it inside incRef/decRef

haowu14 · 2022-12-07T06:49:09Z

cachelib/allocator/CacheAllocator-inl.h

@@ -1231,75 +1262,106 @@ 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 ||


I'm a bit confused here. Are we trying to advance the iterator inside or outside the lambda function passed into withEvictionIterator?
My guess would be inside, because the second time you call withEvictionIterator, the iterator is a newly created one starting from the end of the queue. But this code seems to attempt to advance iterator both inside and outside that lambda.

Right, this is because in case of chained items, we might end up evicting the old parent, and the releaseBackToAllocator on line 1358 will return kNotRecycled. In that case, we'll just loop again and start looking for a new candidate (calling withEvictionIterator again). I did it this way to keep the behavior as close as possible to the original implementation.

haowu14

I made a pass over the entire PR. I think I understand the overall changes and I'd like to suggest breaking it down into the following components:

A. Introducing mmContainer.withEvictionIterator that uses combined locking.
B. Introduce the moving concept (exclusive and refcount>0) and evicting concept (exclusive and refcount==0) into refcount and item. Also, now if an item isExclusive and accessRef == 0, we can't increment the refcount and can't create item handle for it. This is new.
C. Use the B. concepts for findEviction.
D. Use the B. concepts for slabRelease.

Inside each component, I suggest you separate the refactors from the changes. For example:
A.1: Introduce a new iterator class with no lock and the m.withEvictionIterator function.
A.2: Switch the code to use m.withEvictionIterator.
A.3 (optional): Remove the old iterator with LockHolder.
This way we can land the PRs progressively and if we have to revert, only one of the PR is reverted and the fix can be easy.

I'm less confident about if BCD can be separated. I think maybe C and D have to go together but B can ship separately. In B, you created an markInternal interface and I suggest separating that out . In C and D, you consollidated the regular item and chained item version of evict/move. I suggest you separate the refactor and code change (maybe this one is hard).

A few other questions I encountered when reviewing:

While moving, we call acquire(item) and we make assertions on the refcount. Those assertions need to be updated to reflect the fact that we now have refcount 1 under moving. (And maybe we don't need to acquire at all).
When unmakrMoving, do we always have refcount==1 and turning it into refcount==0 afterwards? For a successful move, we'd always have the above condition right?

Coming back to the purpose of the PR on unifying movement and eviction. Can you provide more detail about what you mean on "unify"? I can see we refactored a few place such as moving creation of NVM put token, etc. When I see a change I'd like to associate it with how it may help in the future, but it's a bit hard. I can only verify the correctness but not the intention right now.

Overall I think this diff is the right direction. And if you can break it down into multiple PRs it, we should be able to ship it gradually. I wonder what the other option looks like. Is it going to be less code change?

Thanks for working on this!

haowu14 · 2022-12-12T18:17:53Z

cachelib/allocator/CacheAllocator-inl.h

+                                      &searchTries, &mmContainer,
+                                      &token](auto&& itr) {
+      if (!itr) {
+        ++searchTries;


I think we don't need ++searchTries here. Also move this block to after the end of while loop.

haowu14 · 2022-12-12T18:58:34Z

cachelib/allocator/CacheAllocator-inl.h

-        toRecycle->isChainedItem()
-            ? &toRecycle->asChainedItem().getParentItem(compressor_)
-            : toRecycle;
+        if (candidate_->hasChainedItem()) {


Move this into the block under else if (candidate_->markForEviction()) because we don't want to double increment failure reason.

Actually this belongs to the else of if (candidate_->markForEviction()).

haowu14 · 2022-12-12T18:59:32Z

cachelib/allocator/CacheAllocator-inl.h

+
+        if (shouldWriteToNvmCache(*candidate_) && !token.isValid()) {
+          stats_.evictFailConcurrentFill.inc();
+        } else if (candidate_->markForEviction()) {


In the past, we mark eviction before creating put token. Any reason to change the order?

Yes, currently marking Item for eviction makes the item inaccessible for all reader (all find() calls will return NULL handle for item that is marked for eviction, as if it was already removed from Access Container). This means that we have to create the token earlier to guarantee consistency.

I might have got something wrong here, however. There is still (quite rare failure on consistency/navy.json benchmark).

haowu14 · 2022-12-12T19:16:52Z

cachelib/allocator/CacheAllocator-inl.h

+          // 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() ||


This condition reads to me as if the item is not chained item, or if is chained item and the ownership didn't change, then remove from mmContainer.

But we are removing from mmContainer again in line 1337 unlinkItemForEviction.

Yes, that's correct. This is just an optimization for the common case.
If we succeed here, we don't have to take MMContainer lock for the second time in line 1337.

If the condition fails (the parent has changed), then - we could theoretically just move on to the next candidate (freeing the parent will no longer recycle the memory we want), but we need to do cleanup anyway.

haowu14 · 2022-12-12T19:18:27Z

cachelib/allocator/CacheAllocator-inl.h

+      }
+    });
+
+    if (!toRecycle)


It seems this would only happen if we have used up all the searchTries?

haowu14 · 2022-12-12T19:45:02Z

cachelib/allocator/CacheAllocator-inl.h

+      // we know the item must still be valid. Item cannot be marked as
+      // exclusive. Only parent can be marked as such and even parent needs
+      // to be unmark prior to calling releaseBackToAllocator.
+      const bool wasMoving = head->isMoving();


Why this change?

For chained items, we cannot mark the individual children as exclusive anymore - only moving. Since exclusive (markedForEviction) means that particular item is going to be evicted it can only be applied to the parent (it does not make sense to evict a single child).

igchor · 2022-12-13T21:32:04Z

@haowu14 Thanks for the review!

Yes, splitting this PR is a good idea. I wanted to get your feedback on the overall approach first but if you belive this approach is correct I'm happy to work on splitting it.

I've already created an initial PR which adds combined locking support for MMContainer: #182

As for your comments:

While moving, we call acquire(item) and we make assertions on the refcount. Those assertions need to be updated to reflect the fact that we now have refcount 1 under moving. (And maybe we don't need to acquire at all).

I've actually made it so that item.refCount() will NOT return the extra ref which is used to signal that item is marked as moving. So, in case the item is marked, it's ref count can still be 0.

When unmakrMoving, do we always have refcount==1 and turning it into refcount==0 afterwards? For a successful move, we'd always have the above condition right?

Yes, the internal refcount will be 1 and when we call unmarkMoving this will atomically clear the kExclusive flag and decrease refcount by 1. In case of successful move, the return value from unmarkMoving will be 0. In case of failure it can be != 0 (since item might not have been removed from AC or some reader increased the ref count).

Coming back to the purpose of the PR on unifying movement and eviction. Can you provide more detail about what you mean on "unify"? I can see we refactored a few place such as moving creation of NVM put token, etc. When I see a change I'd like to associate it with how it may help in the future, but it's a bit hard. I can only verify the correctness but not the intention right now. Overall I think this diff is the right direction. And if you can break it down into multiple PRs it, we should be able to ship it gradually. I wonder what the other option looks like. Is it going to be less code change?

In terms of benefits for the single-tier version, this PR introduces the possibility to use combined locking without impacting the eviction success rate which positively impacts performance in our benchmarks. (Using combined locking brings even more performance benefits for multi-tier implementation.)

The main intention under "unification" is to reuse as much code as possible for multi-tier implementation. Without the changes in this PR, we would have to implement two entirely separate code paths for findEviction (in the case of single-tier and multiple memory tiers). We would also need to duplicate or extend the Eviction and Movement logic for SlabRelease to support multiple tiers. Moreover, we recently added support for chained items to our fork that relies on this PR and I'm not sure if we would be able to implement that support as easily without this PR.

That said, some subset of changes from this PR is needed for multi-tier support in both options (e.g. distinguishing between markedForEviction and makedMoving) so once we split the PR we can always just agree on upstreaming some of the changes. In a few days, we plan to clean up our git history and create a draft PR with multi-tier support so you can see what that looks like.

It is similar to 'moving' but requires ref count to be 0. An item which is marked for eviction causes all incRef() calls to that item to fail. This will be used to ensure that once item is selected for eviction, no one can interfere and prevent the eviction from suceeding. 'markedForEviction' relies on the same 'exlusive' bit as the 'moving' state. To distinguish between those two states, 'moving' add 1 to the refCount. This is hidden from the user, so getRefCount() will not return that extra ref.

through withEvictionIterator function. Also, expose config option to enable and disable combined locking. withEvictionIterator is implemented as en extra function, getEvictionIterator() is still there and it's behavior hasn't changed.

markForEviction is used only in findEviction and evictForSlabRelease but not for item movement. moveForSlabRelease relies on markMoving(). Only allow to mark item for eviction 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 for eviction. 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

Summary: through withEvictionIterator function. Also, expose the config option to enable and disable combined locking. withEvictionIterator is implemented as an extra function, getEvictionIterator() is still there and it's behavior hasn't changed. This is a subset of changes from: #172 Pull Request resolved: #182 Reviewed By: therealgymmy Differential Revision: D42038532 Pulled By: haowu14 fbshipit-source-id: 4c4b0671778c3c59f015bd9d68d6068d24d01f8a

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

facebook-github-bot · 2024-02-13T05:58:49Z

Hi @igchor!

Thank you for your pull request.

We require contributors to sign our Contributor License Agreement, and yours needs attention.

You currently have a record in our system, but the CLA is no longer valid, and will need to be resubmitted.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 7, 2022

igchor commented Nov 7, 2022

View reviewed changes

haowu14 requested changes Dec 7, 2022

View reviewed changes

igchor force-pushed the cs_exclusive_and_moving_rebased2 branch from ccd35ab to e60eeed Compare December 9, 2022 02:01

haowu14 requested changes Dec 12, 2022

View reviewed changes

igchor mentioned this pull request Dec 13, 2022

Add combined locking support for MMContainer #182

Closed

igchor added 3 commits January 9, 2023 14:32

Add combined locking support for MMContainer

9fe02ed

through withEvictionIterator function. Also, expose config option to enable and disable combined locking. withEvictionIterator is implemented as en extra function, getEvictionIterator() is still there and it's behavior hasn't changed.

igchor force-pushed the cs_exclusive_and_moving_rebased2 branch from e60eeed to 6aa4078 Compare January 9, 2023 18:59

igchor mentioned this pull request Feb 8, 2023

Introduce 'markedForEviction' state for the Item. #183

Closed

igchor mentioned this pull request Mar 24, 2023

Shorten critical section in findEviction #217

Closed

	// Return refcount excluding control bits and flags.
	// Return refcount excluding moving refcount, control bits and flags.

	Value valueWithoutMovingBit = ref & ~getAdminRef<kExclusive>();
	Value valueWithoutExclusiveBit = ref & ~getAdminRef<kExclusive>();

	// An item is only moving when its refcount is one and only the moving bit
	// An item is only moving when its refcount is one and only the exclusive bit

Critical section part 2: findEviction optimization and code unification #172

Are you sure you want to change the base?

Critical section part 2: findEviction optimization and code unification #172

Conversation

igchor commented Nov 7, 2022

Choose a reason for hiding this comment

haowu14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haowu14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igchor commented Dec 13, 2022 • edited

facebook-github-bot commented Feb 13, 2024

Process

igchor commented Dec 13, 2022 •

edited