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

Use crossbeam-channel instead of stdlib #160

Closed
matklad opened this issue Aug 13, 2018 · 8 comments · Fixed by #214
Closed

Use crossbeam-channel instead of stdlib #160

matklad opened this issue Aug 13, 2018 · 8 comments · Fixed by #214
Milestone

Comments

@matklad
Copy link

matklad commented Aug 13, 2018

Hi! My understanding is that currently notify hard-codes the usage of std::mpsc, and this is a bit unfortunate: there are many channel implementations in Rust (crossbeam-channel most notably), and the std version is not really great: it lacks select, which severally restricts its applicability.

It would be cool if notify had a trait EventSink { fn send(&mut self, Event); } and used that instead of any particular channel implementation.

Though, for practical purposes, perhaps just betting on crossbeam-channel would be even better?

@passcod
Copy link
Member

passcod commented Aug 13, 2018

v4 is in maintenance only, v5 (aiming for end of this year) uses a significantly different architecture based on Tokio and Futures. Internally, v5 uses multiqueue channels for some plumbing which I believe use crossbeam. Externally, in v5 you'll just subscribe to a Stream which will plug into the kernel eventing as appropriate.

@passcod passcod closed this as completed Aug 13, 2018
@matklad
Copy link
Author

matklad commented Aug 13, 2018

Excellent, that should be even better! Thanks!

@netvl
Copy link

netvl commented Feb 8, 2019

If v5 is still some time away, are there any chances for this to be implemented in v4?

@passcod passcod reopened this Feb 8, 2019
@passcod
Copy link
Member

passcod commented Feb 8, 2019

Hmm, I'll take an implementation if someone wants to.

The remaining question I think would be whether to be generic over only the channel exposed through the public API (which would be simpler), or to be generic over every channel use inside this library (which would likely help the performance characteristic further than just the last hop, varying on internals).

@passcod passcod added the Z-needs implementation Needs an implementation, will accept PRs label Feb 9, 2019
@passcod passcod added this to the 5.0.0 milestone Mar 29, 2019
@passcod passcod changed the title Be generic over channel implementation Use crossbeam-channel instead of stdlib Mar 29, 2019
@passcod
Copy link
Member

passcod commented Mar 29, 2019

Shifting this to be purely crossbeam-channel as originally alternately proposed. If there's interest in making it generic I'm happy to take an implementation later.

passcod added a commit that referenced this issue Mar 30, 2019
Only a straight find-replace was done. There are many places where std 
channels were placed in mutexes and arcs; this is not necessary anymore 
but needs more careful refactoring.

See #160
passcod added a commit that referenced this issue Mar 30, 2019
Only a straight find-replace was done. There are many places where std 
channels were placed in mutexes and arcs; this is not necessary anymore 
but needs more careful refactoring.

See #160
passcod added a commit that referenced this issue Mar 30, 2019
Only a straight find-replace was done. There are many places where std 
channels were placed in mutexes and arcs; this is not necessary anymore 
but needs more careful refactoring.

See #160
passcod added a commit that referenced this issue Mar 30, 2019
Only a straight find-replace was done. There are many places where std 
channels were placed in mutexes and arcs; this is not necessary anymore 
but needs more careful refactoring.

See #160
@passcod
Copy link
Member

passcod commented Apr 1, 2019

Done in 0eb4f2e and parents

@passcod passcod closed this as completed Apr 1, 2019
@matklad
Copy link
Author

matklad commented Aug 24, 2019

FWIW, I slowly coming to conclusion that accepting an Box<dyn FnMut(T)> is almost always better than returning an Receiver<T>. The former is more general, as Box::new(|t| chan.send(t)) works, and you can put any channel and any buffering strategy you want, or use something like futures::oneshot instead, or do additional transforms like |t| chan.send(Event::Notify(t))

@ctrlcctrlv
Copy link

Sorry, stupid and confused person here, if I'm using 4.x branch, what is the best way to start using Crossbeam channels? Is such even possible?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants