Skip to content
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: message prioritization with immediate peer-published dispatch and queuing for other msgs #1015

Merged
merged 14 commits into from
Feb 16, 2024

Conversation

diegomrsantos
Copy link
Contributor

@diegomrsantos diegomrsantos commented Jan 29, 2024

Motivation: We are assuming the local peer might be spending too much time/bandwidth relaying messages rather than publishing its own messages, which could increase latency for the latter.

Objective: To enhance efficiency in message processing by prioritizing immediate dispatch of peer-published messages, and managing other messages through a queuing system.

Changes:

Immediate Dispatch for Peer-Published Messages: Ensures top priority by immediately sending messages published by the peer.
Queued Handling for IWANT Replies and Relay Messages: Introduces a unified queuing system for both replies to "IWANT" messages and messages from other peers that need relaying. These are processed after peer-published messages.
Asynchronous Task Queue Tracking: Implements an additional queue to monitor asynchronous tasks associated with each published message. The queue for IWANT replies and relay messages is processed once this is cleared.

@diegomrsantos diegomrsantos force-pushed the forward-msgs-non-priority-v2 branch 2 times, most recently from 8a2f8db to 22d533d Compare January 30, 2024 16:05
Copy link

codecov bot commented Jan 30, 2024

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (e0f70b7) 83.17% compared to head (ace2c68) 83.28%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##           unstable    #1015      +/-   ##
============================================
+ Coverage     83.17%   83.28%   +0.11%     
============================================
  Files            91       91              
  Lines         15351    15442      +91     
============================================
+ Hits          12768    12861      +93     
+ Misses         2583     2581       -2     
Files Coverage Δ
libp2p/protocols/pubsub/floodsub.nim 90.00% <100.00%> (ø)
libp2p/protocols/pubsub/gossipsub.nim 88.22% <100.00%> (+0.13%) ⬆️
libp2p/protocols/pubsub/pubsub.nim 85.44% <100.00%> (+1.53%) ⬆️
libp2p/protocols/pubsub/gossipsub/behavior.nim 88.39% <75.00%> (ø)
libp2p/protocols/pubsub/pubsubpeer.nim 93.49% <93.82%> (+0.99%) ⬆️

... and 4 files with indirect coverage changes

Copy link
Collaborator

@kaiserd kaiserd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you :). Looks good to me.
Did you already test this in a test network / simulation, to make sure the behaviour is as expected.

Why did you open a new PR? Wouldn't it be easier to track changes if we'd push new commits to the existing ones?

@diegomrsantos
Copy link
Contributor Author

diegomrsantos commented Feb 1, 2024

@kaiserd I tested it running a local Nimbus node on Holenski, the previous metrics seem fine. But I see huge spikes in the non-priority queue size (can be hundreds of thousands for some nodes. I'm subscribing to all subnets to have enough traffic. It'd be good to test with a validator node as well.

@diegomrsantos
Copy link
Contributor Author

diegomrsantos commented Feb 1, 2024

My idea with this PR was to make the difference between the implementations more explicit and potentially testing each one easier. I'm gonna squash the commits here and close the original PR later.

@kaiserd
Copy link
Collaborator

kaiserd commented Feb 2, 2024

Thank for the clarification.

For information of contributors not in the Vac P2P unit: We discussed in the meeting this is experimental and not meant for merging yet. We'll also do more experiments on #1009
We will perform more testing on this, including larger scale tests.

@diegomrsantos diegomrsantos changed the title feat: make relayed messages non priority (don't use an explicit queue for priority msgs) feat: message prioritization with immediate peer-published dispatch and queuing for other msgs Feb 6, 2024
@diegomrsantos diegomrsantos marked this pull request as ready for review February 6, 2024 14:02
@diegomrsantos diegomrsantos self-assigned this Feb 8, 2024
Copy link
Collaborator

@kaiserd kaiserd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you :).

@diegomrsantos diegomrsantos merged commit fe4ff79 into unstable Feb 16, 2024
11 checks passed
@diegomrsantos diegomrsantos deleted the forward-msgs-non-priority-v2 branch February 16, 2024 09:54
diegomrsantos added a commit that referenced this pull request Feb 19, 2024
…spatch and queuing for other msgs (#1015)"

This reverts commit fe4ff79.
arnetheduck added a commit that referenced this pull request Mar 4, 2024
…ispatch and queuing for other msgs (#1015)"

This reverts commit c5e4f8e.
This was referenced Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: done
Development

Successfully merging this pull request may close these issues.

3 participants