Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

igchor
Copy link
Contributor

@igchor igchor commented Nov 7, 2022

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:

  1. Implement item movement between tiers as a separate code path in findEviction
  2. 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

@facebook-github-bot 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
if (evictToNvmCache && shouldWriteToNvmCacheExclusive(item)) {
nvmCache_->put(handle, std::move(token));
}
// search if the child is present in the chain
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@haowu14 haowu14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


return markInternal(predicate, newValue);
}
Value unmarkMoving() noexcept {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: empty line between functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

* 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

auto ref = getRefWithAccessAndAdmin();
bool anyOtherBitSet = ref & ~getAdminRef<kExclusive>();
if (anyOtherBitSet) {
Value valueWithoutMovingBit = ref & ~getAdminRef<kExclusive>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// @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() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you refactor this with markInternal?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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 ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

@haowu14 haowu14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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).
  2. 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!

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

// 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() ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
});

if (!toRecycle)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// 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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

igchor commented Dec 13, 2022

@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:

  1. 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.

  1. 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
@igchor igchor force-pushed the cs_exclusive_and_moving_rebased2 branch from e60eeed to 6aa4078 Compare January 9, 2023 18:59
facebook-github-bot pushed a commit that referenced this pull request Feb 3, 2023
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
facebook-github-bot pushed a commit that referenced this pull request Jun 5, 2023
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
Copy link
Contributor

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 [email protected]. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants