-
-
Notifications
You must be signed in to change notification settings - Fork 311
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
feat: improve before process epoch #6979
Conversation
Performance Report✔️ no performance regression detected Full benchmark results
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #6979 +/- ##
============================================
- Coverage 62.51% 62.48% -0.03%
============================================
Files 576 576
Lines 61170 61217 +47
Branches 2140 2135 -5
============================================
+ Hits 38239 38253 +14
- Misses 22892 22925 +33
Partials 39 39 |
indicesToSlash, | ||
indicesEligibleForActivationQueue, | ||
indicesEligibleForActivation, | ||
indicesToEject, | ||
nextEpochShufflingActiveValidatorIndices, | ||
totalNextEpochShufflingActiveIndices, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you call this nextEpochShufflingActiveIndicesLength
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be interesting to see how https://github.com/tc39/proposal-resizablearraybuffer performs here
eg:
// in epochTransitionCache.ts
// top-level declaration
const nextEpochShufflingActiveValidatorIndices = new Uint32Array(new ArrayBuffer(0, {maxByteLength: PRACTICAL_MAX_ACTIVE_INDICES_LENGTH}));
// in beforeProcessEpoch
nextEpochShufflingActiveValidatorIndices.buffer.resize(validatorCount * 4);
/////
// in epochShuffling.ts
export function computeEpochShuffling(
...,
activeIndices: Uint32Array,
activeIndicesLength: number
) {
...
const _activeIndices = activeIndices.slice(0, activeIndicesLength);
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes right now the resize is only available for Array not typed array, once it's available we can apply for other places too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it appears that resizable ArrayBuffer is in node 20 already so we may begin experimentation
* fix: do not populate proposerIndices and inclusionDelays from altair * feat: remove eligibleValidatorIndices * fix: avoid array.slice in processRegistryUpdates() * fix: reuse nextEpochShufflingActiveValidatorIndices * fix: state-transition check-types * chore: rename nextEpochShufflingActiveIndicesLength
🎉 This PR is included in v1.21.0 🎉 |
* fix: do not populate proposerIndices and inclusionDelays from altair * feat: remove eligibleValidatorIndices * fix: avoid array.slice in processRegistryUpdates() * fix: reuse nextEpochShufflingActiveValidatorIndices * fix: state-transition check-types * chore: rename nextEpochShufflingActiveIndicesLength
Motivation
beforeProcessEpoch
by not allocating big arrays unnecessarily (that could cause spikes due to gc)Description
eligibleValidatorIndices
: we can just base onflags
nextEpochShufflingActiveValidatorIndices
by allocatingvalidatorCount
items, usetotalNextEpochShufflingActiveIndices
to control its lengthproposerIndices
andinclusionDelays
if it's not phase0Testing
on holesky
beforeProcessEpoch
could be less than 200ms. Note that it could still spike due to allocating validators array