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

Make the number of events buffered to/from tasks configurable #1574

Merged
merged 6 commits into from
May 15, 2020

Conversation

tomaka
Copy link
Member

@tomaka tomaka commented May 14, 2020

cc paritytech/substrate#6009

Hints seem to indicate that we have some CPU issues in Substrate from the low number of events that can be buffered. This PR makes this limit configurable.

Additionally, this PR increases these values: the channel size from tasks to the swarm is now 32 (instead of 1!), and the channel size from the swarm to each task is now 7 (so 8 in practice).

When reviewing, keep in mind that channels reserve an extra element for each active sender, as documented: https://docs.rs/futures/0.3.5/futures/channel/mpsc/fn.channel.html

(I also slightly fixed the CHANGELOG to be alphabetically-ordered)

@tomaka tomaka requested a review from romanb May 14, 2020 09:28
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.

I left only naming and documentation suggestions that are motivated as follows:

  • Internally, I previously established the terminology of "task events" vs "task commands", rather than speaking of "events from" or "events to" tasks, respectively, so that naming in the pool and manager settings would be more consistent.
  • On the public API, I don't think it is beneficial to talk about (background) tasks. Instead, a user of libp2p-core knows that notify_handler is used to send events to the connection handlers and that he receives connection events by polling the network. Hence buffer sizes that relate to notify_handler and "connection events" are directly meaningful, especially with the added documentation about the effect of a full buffer.

core/src/connection/manager.rs Outdated Show resolved Hide resolved
core/src/connection/manager.rs Outdated Show resolved Hide resolved
core/src/connection/manager.rs Outdated Show resolved Hide resolved
core/src/network.rs Outdated Show resolved Hide resolved
core/src/connection/manager.rs Outdated Show resolved Hide resolved
core/src/network.rs Outdated Show resolved Hide resolved
core/src/network.rs Outdated Show resolved Hide resolved
swarm/src/lib.rs Outdated Show resolved Hide resolved
swarm/src/lib.rs Outdated Show resolved Hide resolved
core/src/network.rs Outdated Show resolved Hide resolved
tomaka and others added 2 commits May 14, 2020 17:25
Co-authored-by: Roman Borschel <romanb@users.noreply.github.com>
core/src/connection/manager.rs Outdated Show resolved Hide resolved
core/src/network.rs Outdated Show resolved Hide resolved
Co-authored-by: Roman Borschel <romanb@users.noreply.github.com>
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

For what my review is worth, this looks good to me.

@tomaka tomaka merged commit c271f6f into libp2p:master May 15, 2020
@tomaka tomaka deleted the configurable-channel-sizes branch May 15, 2020 12:40
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