feat: make aggregate with randomness async again #7761
Merged
Conversation
This file contains hidden or 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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #7761 +/- ##
=========================================
Coverage 56.08% 56.08%
=========================================
Files 823 823
Lines 58031 58033 +2
Branches 4461 4467 +6
=========================================
+ Hits 32545 32550 +5
+ Misses 25419 25416 -3
Partials 67 67 🚀 New features to boost your workflow:
|
Contributor
Performance Report✔️ no performance regression detected Full benchmark results
|
Contributor
|
Hey Matthew and Lodestar team, thanks for giving this another try so quickly. I noticed a big change in event loop lag after upgrading our Gnosis Chain nodes from Lodestar 1.27.0-rc.0 (which included this change) to 1.29.0 (which didn't anymore):
Even in 1.27.0-rc.0, event loop lag was kind of high but 1.29 made it worse, leading to very erratic API response times. I am now running this branch on one of our Gnosis nodes and so far it is looking very good. On the main thread, event loop lag is much better than on 1.29.0:
In terms of the issues seen in #7218 . Event loop lag is worse on the network thread than on 1.29.0:
GC on the network thread is only slightly worse than on 1.29.0: 
And peers are looking just fine on this node:
I'll keep an eye on this over the coming days but those API response times are much more stable now: |
Contributor
|
need to make sure metrics on network thread is not as worse as monitored here |
nflaig
reviewed
Apr 30, 2025
Member
|
shared some metrics on discord |
Contributor
|
the bad peer count metrics I have last time does not happen, it just look like #7359 (comment) I do see this PR improved the main thread a lot that caused performance issue on network thread, I track the issue in #7801 so looks good to me overal, that's something for us to improve in the future |
twoeths
previously approved these changes
May 7, 2025
wemeetagain
approved these changes
May 8, 2025
Member
wemeetagain
left a comment
wemeetagain
left a comment
There was a problem hiding this comment.
metrics look good on this, excited to finally get it back
nflaig
added a commit
that referenced
this pull request
May 8, 2025
Member
|
🎉 This PR is included in v1.30.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
Gnosis analysis shows that there is a lot of time spent on main thread for running aggregateWithRandomness on main thread. Now that libp2p@v2 is more stable and so are some of the other conditions that triggered our revert last time, looking to check this again to see what the metrics look like for running not on main thread.