fix: improve compute new state root when producing block #6195
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
nflaig
reviewed
Dec 15, 2023
| @@ -104,6 +104,12 @@ export class PrepareNextSlotScheduler { | |||
| RegenCaller.precomputeEpoch | |||
| ); | |||
|
|
|||
| // cache HashObjects for faster hashTreeRoot() later, especially for computeNewStateRoot() if we need to produce a block at slot 0 of epoch | |||
| // see https://github.com/ChainSafe/lodestar/issues/6194 | |||
| const hashTreeRootTimer = this.metrics?.stateHashTreeRootTime.startTimer({source: "prepare_next_slot"}); | |||
There was a problem hiding this comment.
What should be the naming convention we use for source?
e.g. here we use camelCase (function names)
and recently added (#6143) step metrics also use camelCase
I kinda like the function names and might be better for consistency, but maybe not always applicable (i.e. if there is no function call specific to the metric)
There was a problem hiding this comment.
What should be the naming convention we use for source?
agree to have some naming convention, we used snake case as suggested by @dapplion and that's my preference. Agree there are consistencies so need some agreement to work on
I don't want to use function names, just which whatever name that makes sense in the context because:
- some function names are so long so it's not nice to render in grafana, for example
verifyBlocksStateTransitionOnly - one function could call a metric multiple times
so this one should be more dynamic to me, as long as a PR gets through review process we're fine I think
There was a problem hiding this comment.
I don't want to use function names, just which whatever name that makes sense in the context because:
Makes sense to not be to strict here then, also I guess those labels are mostly used to visualize and separate metric values in a panel. As long as it is visually readable and makes sense in the context of the metrics the naming convention (camelCase vs snake_case) should not matter that much.
There was a problem hiding this comment.
snake case just reads better in grafana :)
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6195 +/- ##
=========================================
Coverage 90.35% 90.35%
=========================================
Files 78 78
Lines 8087 8087
Branches 490 490
=========================================
Hits 7307 7307
Misses 772 772
Partials 8 8 |
Performance Report✔️ no performance regression detected Full benchmark results
|
|
nflaig
approved these changes
Dec 15, 2023
|
🎉 This PR is included in v1.13.0 🎉 |
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
Improve "computeNewStateRoot" function
Description
computeNewStateRootwas so slow because we didn't compute hash tree root to cache in "prepareNextSlot". The main fix for this is to do it in "prepareNextSlot"state.hashTreeRoot()call in 3 places:verifyBlocksStateTransitionOnly(),prepareNextSlot()andcomputeNewStateRoot()Closes #6194