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

sync: Add async mpsc::Sender::shared_send method #2041

Closed
wants to merge 1 commit into from

Conversation

udoprog
Copy link
Contributor

@udoprog udoprog commented Dec 30, 2019

Add async mpsc::Sender::shared_send method which takes a shared reference instead of a mutable reference.

Motivation

I've frequently found a need to use a Sender through a shared reference, and I decided to try and grok the code to find out why that is the case.

So we require unique access because the Sender embeds the permit used. And with the current Semaphore API, we need to provide a mutable permit for it to work with.

Solution

This send variant creates a new permit on-the-fly to avoid taking a unique reference.

The alternative is to clone the entire sender, which would involve cloning the inner Arc<Char<T, S>> nedlessly. Using shared_send instead does the minimal amount of work possible with the current algorithm.

There's some unfortunate leakage in Chat that had to be introduced pub(crate). But I'm not sure how to do this in a cleaner fashion. Suggestions are welcome, and feel free to bikeshed the name.

This send variant creates a new permit on-the-fly to avoid taking a
unique reference. It's useful for instances where the Sender is
embedded into a larger struct which itself is shared across an
application.

The alternative is to clone the entire sender, which would involve
cloning the inner `Arc<Char<T, S>>` nedlessly. Using `shared_send`
instead does the minimal amount of work possible with the current
algorithm.
@cynecx
Copy link
Contributor

cynecx commented Dec 31, 2019

The overhead should be the same imo, the only difference is that the new permit for each call to shared_send is created and located on the stack. Is there is a reason why we can’t use the shared_send variant as the main one instead of having a struct local permit? Am I missing something?

@udoprog
Copy link
Contributor Author

udoprog commented Jan 2, 2020

The overhead should be the same imo, the only difference is that the new permit for each call to shared_send is created and located on the stack. Is there is a reason why we can’t use the shared_send variant as the main one instead of having a struct local permit? Am I missing something?

Creating a permit has low, but not zero cost. Struct local permit is the most efficient when we have mutable access to the sender.

Cloning the inner Arc is also low cost, but not zero. It fully depends on how constrained the scenario in which it's used is. But I'm mostly interested in the &self-taking send API, which simplifies use for a number of scenarios. If we come up with any future improvements to how it works internally, any user of it will also automatically benefit.

@cynecx
Copy link
Contributor

cynecx commented Jan 2, 2020

Creating a permit has low, but not zero cost.

IMO, in its current implementation, it's pretty much zero cost, and the overhead is negligible.

The compiled assembly code reflects this: https://godbolt.org/z/-X8vSP .

Struct local permit is the most efficient when we have mutable access to the sender.

How so? It has the same efficiency, with or without mutable access to the sender (In case of a struct method local permit).

But I'm mostly interested in the &self-taking send API, which simplifies use for a number of scenarios. If we come up with any future improvements to how it works internally, any user of it will also automatically benefit.

Yes, that's exactly what I am proposing, by actually using a struct method local permit, which in my in my opinion is pretty much zero-cost (except I am missing something crucial), we can have a send method which takes a non-mutable self reference, and this may replace the current one.

@udoprog
Copy link
Contributor Author

udoprog commented Jan 2, 2020

Yes, that's exactly what I am proposing, by actually using a struct local permit, which in my in my opinion is pretty much zero-cost (except I am missing something crucial), we can have a send method which takes a non-mutable self reference, and this may replace the current one.

I'm not following. The permit is currently struct local (as in the Sender struct) which is why the sender must be borrowed exclusively.

@cynecx
Copy link
Contributor

cynecx commented Jan 2, 2020

@udoprog Ups, sorry. I meant method local permit. (I've fixed my comment).

@Darksonn Darksonn added A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-sync Module: tokio/sync labels Jul 25, 2020
@Darksonn
Copy link
Contributor

Closing due to inactivity.

@Darksonn Darksonn closed this Aug 26, 2020
@udoprog udoprog deleted the shared-send branch August 26, 2020 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-sync Module: tokio/sync
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants