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

Implement Sync for SyncSender #42397

Merged
merged 1 commit into from Jun 21, 2017

Conversation

@sfackler
Copy link
Member

commented Jun 3, 2017

@sfackler sfackler force-pushed the sfackler:syncsender-sync branch from 1784d76 to 0f6c01d Jun 3, 2017

@alexcrichton alexcrichton added the T-libs label Jun 3, 2017

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jun 3, 2017

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

commented Jun 3, 2017

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@BurntSushi

This comment has been minimized.

Copy link
Member

commented Jun 3, 2017

Is there context on this? Was there something preventing this from happening sooner?

Related RFC? https://github.com/seanmonstar/rfcs/blob/shared-sender/text/0000-shared-sender.md

@sfackler

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2017

The Sender type is genuinely non-Sync, but that's not the case for SyncSender - it's just a wrapper around an Arc. (SyncSender is used with fixed-capacity channels while Sender is used with infinite-capacity channels). @alexcrichton mentioned on IRC that he's pretty sure the !Sync bound was just there for consistency with Sender.

@carols10cents

This comment has been minimized.

Copy link
Member

commented Jun 12, 2017

ping @brson looks like you're the only remaining checkbox that's unchecked!

@rfcbot

This comment has been minimized.

Copy link

commented Jun 20, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jun 20, 2017

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2017

📌 Commit 0f6c01d has been approved by alexcrichton

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Jun 20, 2017

@bors rollup

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Jun 21, 2017
bors added a commit that referenced this pull request Jun 21, 2017
Auto merge of #42802 - frewsxcv:rollup, r=frewsxcv
Rollup of 4 pull requests

- Successful merges: #42397, #42620, #42762, #42766
- Failed merges:

@bors bors merged commit 0f6c01d into rust-lang:master Jun 21, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sfackler sfackler deleted the sfackler:syncsender-sync branch Jun 26, 2017

@stjepang

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2017

Sorry for being late to the party. I feel like the full implications of this change were not thorougly discussed, so I'd like to clear up the confusion.

We have two kinds of channels:

  1. Infinite: (Sender<T>, Receiver<T>)
  2. Bounded: (SyncSender<T>, Receiver<T>)

Both kinds of channels are MPSC - in other words, they allow multiple senders, but only one receiver.
For that reason we have Sender<T>: Clone, SyncSender<T>: Clone, but Receiver<T> is not cloneable.

Should these types implement Sync? Well, Receiver<T> surely shouldn't, since only one thread may have the receiver. But what about Sender<T> and SyncSender<T>?

Before answering this question, let's take a look at the how the channels are implemented:

  1. Infinite channels are smart. They keep track of number of calls to Sender::clone, so if the first sender was never cloned, a faster implementation with fewer atomic operations is used. However, if Sender<T>: Sync were true, then we couldn't use this optimization because any sender could be shared among multiple threads right from the start!

  2. Bounded channels are not so smart. The implementation is pretty simple - all send/recv operations are performed behind one big mutex. This is because nobody had the time to write a smarter implementation using atomics. We don't use the single-sender optimization like in the case of inifinite channels, so it's no problem to impl Sync for SyncSender<T>. But now we won't be able to do that optimization in the future, and that is something we should take into consideration.

An alternative solution was proposed in this RFC, introducing a new type SharedSender<T> that implements Sync. I like this suggestion (in the "Alternatives" section):

impl<T: Send> Sender<T> {
    pub fn shared(self) -> SharedSender<T> {
        // upgrade to Flavor::Shared, take shared::Packet, create SharedSender
    }
}

Likewise, we could introduce a new type SharedSyncSender<T>. This solution doesn't prevent single-sender optimizations, but the drawback is increased API surface.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jul 17, 2017

@stjepang thanks for the comment! I think you're spot on with your conclusions. That being said I've often somewhat regretted the specialization of mpsc channels here in libstd, they're probably out of place at this point in time... In any case the implementation of SyncSender has always been much less optimized than Sender itself, so I'm not too too worried about blocking future implementations here.

@rust-lang/libs this is still possible to revert though I think as it's just going to beta soon (not stable). I personally would love to have a SharedSender in libstd (I think the extra API surface area is worth it), but thoughts?

@stjepang

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2017

@alexcrichton Personally I'm not too opposed to having SyncSender: Sync. It's just that the rationale for it is a bit weak. It'd make sense to have both Sender and SyncSender impl Sync, or none of them, but not just one of them. :)

That being said I've often somewhat regretted the specialization of mpsc channels here in libstd, they're probably out of place at this point in time...

I'd love to hear more - what in particular you're regretting and why? At the moment I'm working on a crate that provides MPMC channels (it aims to fix the usual pain points with std::sync::mpsc) so I'm facing the same API design issues.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jul 17, 2017

I'd love to hear more - what in particular you're regretting and why?

Heh sure!

  • I think the implementation is way too "big" for the standard library. It's probably one of the most "clever" things in the standard library and feels somewhat out of place. A simpler and/or easier to understand implementation would likely be more welcome today. When this was added it was the way to communicate between threads in Rust, so it had to be mega-optimized for everyone's use case. Nowadays we've got a whole host of methods to communicate, so there's no need for such a large and crazy implementation to be in libstd.
  • A downside of being "big" is that usage of the channels involves monomorphizing a lot of code. On the playground an empty hello world is 0.6s to compile whereas creating a channel, sending a message, and receiving takes 1.7s to compile. That's a full second for a huge suite of optimizations you probably don't need.
  • I wish Receiver::recv took &mut self to make Receiver be Sync, but explicitly denote that you need unique access.
  • The whole business with selection over multiple channels never really worked out unfortunately.
  • It's a real bummer that Sender isn't Sync. It should be basically!

So basically no real strong reason or regrets, just that if we started from scratch today I'd probably do things differently!

@sfackler

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2017

The select infrastructure also makes the internals way more complex than they'd otherwise be. Sync channels can otherwise be nothing more than a Mutex<VecDeque<T>> + Condvar.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Aug 1, 2017

The libs team got a chance to discuss this during triage today, and the conclusion was that the likelihood of us taking advantage of SyncSender not being Sync (e.g. what Sender does) is so low that we'll let this slide today. We'd be very interested, though, to add a SharedSender type which is Sync for the unbounded case though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.