fix: improve forkchoice updateHead() time #5867
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
Performance Report✔️ no performance regression detected 🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
|
I'm not sure if using common variables before the for loop help in the case lodestar uses so much memory, the performance is the same when I test in goerli. At least it does no harm, may want to consult Ben if it's a necessary change or not 
I'm not sure why the perf test is way faster even though I added all votes with different current root / next root. At least I see the benchmark is better, from more than 3ms for 250k validators 
to this new value 
|
dapplion
reviewed
Aug 9, 2023
dapplion
reviewed
Aug 9, 2023
| @@ -21,33 +21,38 @@ export function computeDeltas( | |||
| ): number[] { | |||
| const deltas = Array.from({length: indices.size}, () => 0); | |||
There was a problem hiding this comment.
This can also be improved, it's faster to do a regular for loop than array.from
There was a problem hiding this comment.
What about Array.fill performance?
There was a problem hiding this comment.
Last time i checked regular for loop in the fastest way to populate an array. There should be benchmarks in the state transition package
There was a problem hiding this comment.
array creation
Array.from(() => 0): 9.6100125 ms
✔ Array.from(() => 0)
Array.from().fill(0): 8.389225 ms
✔ Array.from().fill(0)
Array.from(): 15.170741699999999 ms
✔ Array.from()
new Array(): 0.8613583 ms
✔ new Array()
new Array(); for loop: 1.2474166 ms
✔ new Array(); for loop
There was a problem hiding this comment.
Have you benchmark just pushing to an empty array?
There was a problem hiding this comment.
not yet, how important is it? in this case there are always more than 0 protonodes in forkchoice
|
with 0aa5224 the |
twoeths
changed the title
wemeetagain
approved these changes
Aug 11, 2023
|
The root cause of this slow down is we call The complexity of a function to check if all nodes are ancestor of finalized node (naive way, not the improvement in this PR) is We need to review:
|
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
forkChoice updateHead vc 600000 bc 7200 eq 0testDescription
part of #5852