Clone states only when necessary #4279
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dapplion
commented
Jul 11, 2022
| @@ -27,6 +27,7 @@ export function stateTransition( | |||
| const block = signedBlock.message; | |||
| const blockSlot = block.slot; | |||
|
|
|||
| // .clone() before mutating state in state transition | |||
| let postState = state.clone(); | |||
There was a problem hiding this comment.
Here and line 85 are the only two instances where state is cloned
Performance Report✔️ no performance regression detected Full benchmark results
|
|
In all contabo-5,contabo-18,contabo-19 there's a noticeable improvement in block processing times (deployed branch at the blue line)
|
dapplion
commented
Jul 12, 2022
| export function initiateValidatorExit(state: CachedBeaconStateAllForks, validator: phase0.Validator): void { | ||
| export function initiateValidatorExit( | ||
| state: CachedBeaconStateAllForks, | ||
| validator: CompositeViewDU<typeof ssz.phase0.Validator> |
There was a problem hiding this comment.
Using CompositeViewDU to express the need for mutability here
twoeths
reviewed
Jul 12, 2022
| postState = processSlots(postState, nextEpochSlot, metrics); | ||
|
|
||
| // Cache state to preserve epoch transition work | ||
| const checkpointState = postState.clone(); | ||
| const checkpointState = postState; |
There was a problem hiding this comment.
@dapplion I think it's safer to do postState.clone(false)?
from your implementation
clone(dontTransferCache?: boolean): this {
if (dontTransferCache) {
return this.type.getViewDU(this.node) as this;
} else {
const cache = this.cache;
this.clearCache();
return this.type.getViewDU(this.node, cache) as this;
}
}There was a problem hiding this comment.
Sorry I don't follow, what would it be safer to do a clone here?
There was a problem hiding this comment.
@tuyennhv The point of this PR is to think critically about when our state can actually be mutated. My opinion is that we clone is too many places that a state can't be mutated so cloning is an unnecessary expense that slows than Lodestar for no gain
There was a problem hiding this comment.
@dapplion in the past we used to have an issue of missing a clone() call, it really took time to debug in that case, @wemeetagain may experience it
so what I mean is instead of dropping the clone() call, can we do clone(false) with less performance impact?
if both clone() and clone(false) cause performance issue then we need to benchmark to improve performance?
There was a problem hiding this comment.
My confusion: I thought the cache is merely used for the mutation, it also contains the index of nodes so that we only have to traverse to the node once.
if we use clone(false), the consumer does not have a chance to use the cached nodes of the tree so it's all bad going with either clone() or clone(false)
with ssz v2, we should not ever have a state mutation unless in state transition, thanks @dapplion for your explanation 👍
twoeths
reviewed
Jul 18, 2022
|
|
||
| // Clone first to account for metrics below | ||
| const itemCloned = item.clone(); | ||
|
|
||
| this.metrics?.stateClonedCount.observe(item.clonedCount); |
There was a problem hiding this comment.
since we don't call clone() anymore, should metric observe clonedCount here?
twoeths
approved these changes
Jul 18, 2022
wemeetagain
approved these changes
Jul 18, 2022
|
Oh, just noticed the metrics above |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
SSZ v2 introduced a new caching strategy:
Description
To prevent losing the cache to only-read clones, clone the state only when it's mutated via a state transition run
@wemeetagain @tuyennhv This PR can have serious implications, please review in depth and think of corner cases. This should be tested on nodes too