feat: track prepare next epoch time #6256
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
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6256 +/- ##
============================================
+ Coverage 80.31% 80.38% +0.07%
============================================
Files 202 202
Lines 19543 19622 +79
Branches 1169 1176 +7
============================================
+ Hits 15695 15773 +78
- Misses 3820 3821 +1
Partials 28 28 |
Performance Report✔️ no performance regression detected Full benchmark results
|
nflaig
reviewed
Jan 8, 2024
| @@ -1298,6 +1298,11 @@ export function createLodestarMetrics( | |||
| name: "lodestar_precompute_next_epoch_transition_waste_total", | |||
| help: "Total number of precomputing next epoch transition wasted", | |||
| }), | |||
| duration: register.histogram({ | |||
| name: "lodestar_precompute_next_epoch_transition_duration_seconds", | |||
| help: "Duration of precomputeNextEpochTransition at lodestar side", | |||
There was a problem hiding this comment.
The "at lodestar side" might be confusing, are we precomputing the next epoch transition somewhere else (not in lodestar)?
Looking at the code the two major tasks we do are
- precompute next epoch transition
- and prepare execution payload
Might be missing something
There was a problem hiding this comment.
There's also the prepareExecutionPayload call that's not included in this timer.
There was a problem hiding this comment.
But isn't that a different task or maybe I misunderstand what is meant by precomputeEpochTransition, my assumption is this includes epoch transition + hashing the state.
From my understanding the "at lodestar side" is redundant and can be removed as the task precomputeNextEpochTransition is only executed by Lodestar.
prepareExecutionPayload is part of preparing for next slot but not part of precomputing the epoch transition.
wemeetagain
reviewed
Jan 8, 2024
wemeetagain
previously approved these changes
Jan 8, 2024
nflaig
reviewed
Jan 8, 2024
nflaig
approved these changes
Jan 8, 2024
wemeetagain
approved these changes
Jan 8, 2024
|
🎉 This PR is included in v1.14.0 🎉 |
ensi321
pushed a commit
to ensi321/lodestar
that referenced
this pull request
Jan 22, 2024
* feat: track prepare next epoch time * Update packages/beacon-node/src/metrics/metrics/lodestar.ts * Align variable name of timer with histogram naming --------- Co-authored-by: Cayman <caymannava@gmail.com> Co-authored-by: Nico Flaig <nflaig@protonmail.com>
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
Track prepare next epoch time at lodestar side, the log shows that a lot of time it take >4s on holesky
Description
hashTreeRoot()