Pull gossip queues for better throughput #5195
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 Full benchmark results
|
|
Deployed to Queues are never full, message drop rate is 0, and job wait time is almost 0
But mesh peers is now at minimum and don't have peers on most subnets
We receive way less messages in the beacon_attestation topic
Peer quality is much different, we have mostly nodes with no validators connected
EDIT: From Tuyen
|
|
peers are not pruned because:
this is the same issue to #5198 The reason it happens more to this branch maybe because gossip score it so great (point 3 above - I compared gossipsub score in feat3 and other groups) |
dapplion
force-pushed
the
dapplion/gossip-queue-pull
branch
from
9a5b31b
to
99cb497
Compare
1 task
twoeths
reviewed
Mar 1, 2023
packages/api/package.json
Outdated
| @@ -11,7 +11,7 @@ | |||
| "bugs": { | |||
| "url": "https://github.com/ChainSafe/lodestar/issues" | |||
| }, | |||
| "version": "1.4.3", | |||
| "version": "1.5.0", | |||
There was a problem hiding this comment.
I wonder why it shows the diff here while unstable really have version as 1.5.0, should not be an issue anyway
There was a problem hiding this comment.
Rebased on unstable and fixed the bad diff
|
This is awesome work, I can see good metrics like "Job Wait Time" or "Dropped jobs %". However I also notice this push some pressure to I/O lag issue on #4002:
|
|
@dapplion is it possible to introduce some flags to mitigate the issue? |
dapplion
force-pushed
the
dapplion/gossip-queue-pull
branch
from
0e803d0
to
d04463a
Compare
If any this PR is allowing nodes to process all network objects they are required to process as per the spec. "The issue" is actually correct behavior, where current stable drops too many messages. To mitigate "the issue" we should force the node into processing less messages than they can, sort of re-introducing a forced limiting throughput. I'm softly against introducing data, and instead spend our energy understanding what's taking CPU time. If we were to add flags we could limit the max concurrency per topic. |
|
0301_gossip-queue-pull_1k.cpuprofile.zip I'm attaching the profile, there is no special thing except for In the current implementation we yield to the macro queue every 50ms through |
That should not be necessary in theory, I added metrics and each gossip packet tends to trigger work on itself. From looking at the profile I don't see issues related with this code specifically. Since everything is run sync now it makes sense to show up on the call stack of handleReceivedMessages and executeWork but their self time is small |
wemeetagain
reviewed
Mar 1, 2023
| gossipHandlers?: GossipHandlers; | ||
| }; | ||
|
|
||
| export class NetworkWorker { |
There was a problem hiding this comment.
might be worth adding a tsdoc comment here describing the purpose or mentioning that this isn't related to thread workers
There was a problem hiding this comment.
Yeah naming sucks, I think this specific class should be merged into the NetworkProcessor
wemeetagain
reviewed
Mar 1, 2023
| maxGossipTopicConcurrency?: number; | ||
| }; | ||
|
|
||
| const executeGossipWorkOrderObj: Record<GossipType, true> = { |
There was a problem hiding this comment.
might be worth comment here mentioning the order here defines the order that gossip topics will be processed
wemeetagain
reviewed
Mar 1, 2023
| }; | ||
|
|
||
| export type NetworkProcessorOpts = GossipHandlerOpts & { | ||
| maxGossipTopicConcurrency?: number; |
wemeetagain
approved these changes
Mar 1, 2023
nazarhussain
added a commit
that referenced
this pull request
Mar 14, 2023
This reverts commit f03f911.
nazarhussain
added a commit
that referenced
this pull request
Mar 14, 2023
|
🎉 This PR is included in v1.6.0 🎉 |
twoeths
added a commit
that referenced
this pull request
Mar 27, 2023
twoeths
added a commit
that referenced
this pull request
Mar 27, 2023
|
🎉 This PR is included in v1.7.0 🎉 |
twoeths
added a commit
that referenced
this pull request
Apr 7, 2023
wemeetagain
pushed a commit
that referenced
this pull request
Apr 7, 2023
|
🎉 This PR is included in v1.8.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
Lodestar's gossip queues are too restrictive to handle big volumes of objects such when subscribed to all subnets
Our existing queues throttle by limiting concurrency, irrespective of downstream worker's capacity. They have the authority to push jobs, and do so independently from each other
Description
This PR updates the queue design to:
Marking as draft until testing shows all good