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

Prioritisation network manager + transactions manager + eth request handler #6590

Merged
merged 78 commits into from
Mar 6, 2024

Conversation

emhane
Copy link
Member

@emhane emhane commented Feb 13, 2024

Closes #6589. Closes #6336.

Generalises preemption pattern from #6334 (originally from NetworkManager when polling Swarm). Incorporates this pattern into NetworkManager as well as TransactionsManager.

Decomposed into #6656 and #6651 and this pr.

@emhane emhane changed the title Prioritisation network manager + transactions manager Prioritisation network manager + transactions manager + eth request handler Mar 2, 2024
@emhane
Copy link
Member Author

emhane commented Mar 2, 2024

dug up some helpful resources

tasks can starve in tokio, just "not for too long":
https://github.com/tokio-rs/tokio/blob/5658d7c5032ade1bd0b18e3ec5b2b788eeac630e/tokio/src/runtime/coop.rs#L45-L57

pre-emption in a loop can cause busy-wait:
https://doc.rust-lang.org/nightly/std/thread/fn.yield_now.html

(Tokio's yield_now is analogous to that of std lib https://docs.rs/tokio/latest/tokio/task/index.html#yield_now)

@emhane emhane requested a review from mattsse March 2, 2024 19:30
@emhane
Copy link
Member Author

emhane commented Mar 4, 2024

Screenshot 2024-03-04 at 01 22 34 Screenshot 2024-03-04 at 01 22 41 Screenshot 2024-03-04 at 01 23 01 Screenshot 2024-03-04 at 01 22 49 Screenshot 2024-03-04 at 01 29 26

merged into main at commit 3e54d94. looking a lot better than on main.

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

the results look great.

I think I can live with the macro, if we rename it to
poll_nested_stream_with_budget! because I find it yield point misleading because the macro doesn't trigger a yield point (return Pending)

Comment on lines 675 to 682
let maybe_more_handle_messages = poll_nested_stream_with_yield_points!(
"net",
"Network message channel",
DEFAULT_BUDGET_TRY_DRAIN_NETWORK_HANDLE_CHANNEL,
this.from_handle_rx.poll_next_unpin(cx),
|msg| this.on_handle_message(msg),
error!("Network channel closed");
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, good point, so better be safe than sorry

crates/net/network/src/manager.rs Outdated Show resolved Hide resolved
crates/net/network/src/budget.rs Show resolved Hide resolved
crates/net/network/src/transactions/mod.rs Outdated Show resolved Hide resolved
@emhane
Copy link
Member Author

emhane commented Mar 4, 2024

the results look great.

yay! are you keen on getting this into beta or no?

if so, it would be nice if time can be allocated to extended the prio-poll pattern to ActiveSession. each active session is spawned as a non-blocking task. this could possibly reduce spikes in FetchEvents.
Screenshot 2024-03-04 at 14 05 22

I think I can live with the macro, if we rename it to poll_nested_stream_with_budget! because I find it yield point misleading because the macro doesn't trigger a yield point (return Pending)

true, good point. naming that is as coherent with the code as possible is key to readability and adoptability.

@mattsse mattsse mentioned this pull request Mar 4, 2024
@mattsse
Copy link
Collaborator

mattsse commented Mar 4, 2024

yay! are you keen on getting this into beta or no?

definitely

@emhane emhane requested a review from mattsse March 6, 2024 04:57
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

cool, with the extracted eventhandler functions this is a lot easier to read now.

nice work!

@emhane emhane added this pull request to the merge queue Mar 6, 2024
Merged via the queue into main with commit 422b8f8 Mar 6, 2024
30 checks passed
@emhane emhane deleted the emhane/prioritisation-network-manager branch March 6, 2024 13:54
@emhane emhane mentioned this pull request Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-networking Related to networking in general C-perf A change motivated by improving speed, memory usage or disk footprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prioritisation abstraction in NetworkManager Prioritisation abstraction in TransactionsManager
2 participants