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

swarm/src/protocols_handler: Use FuturesUnordered in NodeHandlerWrapper #1775

Merged
merged 5 commits into from
Oct 9, 2020

Conversation

mxinden
Copy link
Member

@mxinden mxinden commented Sep 29, 2020

Futures managed by FuturesUnordered will only be polled when they
generate wake-up notifications. This reduces the required amount of work
needed to poll large numbers of futures.

https://docs.rs/futures/0.3.5/futures/stream/struct.FuturesUnordered.html

Instead of iterating each inbound and outbound upgrade looking for one
to make progress, use a FuturesUnordered for both pending inbound and
pending outbound upgrades. As a result only those upgrades are polled
that are ready to progress.

> Futures managed by FuturesUnordered will only be polled when they
generate wake-up notifications. This reduces the required amount of work
needed to poll large numbers of futures.

https://docs.rs/futures/0.3.5/futures/stream/struct.FuturesUnordered.html

Instead of iterating each inbound and outbound upgrade looking for one
to make progress, use a `FuturesUnordered` for both pending inbound and
pending outbound upgrades. As a result only those upgrades are polled
that are ready to progress.
Copy link
Contributor

@romanb romanb left a comment

Choose a reason for hiding this comment

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

Nice improvement 👍

Copy link
Member

@tomaka tomaka left a comment

Choose a reason for hiding this comment

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

While the change in isolation is good, it might reveal buggy poll implementations in ProtocolsHandlers that wouldn't wake up the task when they should.
For Substrate in particular, we should thoroughly test it when updating libp2p.

@mxinden
Copy link
Member Author

mxinden commented Oct 1, 2020

While the change in isolation is good, it might reveal buggy poll implementations in ProtocolsHandlers that wouldn't wake up the task when they should.

Good point @tomaka . I will add a note in the changelog so we don't forget.

@@ -2,7 +2,7 @@
name = "libp2p-swarm"
edition = "2018"
description = "The libp2p swarm"
version = "0.22.0"
version = "0.22.1"
Copy link
Member Author

Choose a reason for hiding this comment

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

I am declaring this as a non-breaking change, thus bumping the patch version only. Let me know in case you prefer a minor version bump.

@mxinden mxinden merged commit 0c02a8a into libp2p:master Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants