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 crossbeam-channels optional #425

Merged
merged 1 commit into from
Aug 9, 2022
Merged

make crossbeam-channels optional #425

merged 1 commit into from
Aug 9, 2022

Conversation

0xpr03
Copy link
Member

@0xpr03 0xpr03 commented Aug 8, 2022

Allows disabling crossbeam-channel by disabling default features.

This fixes #380

@0xpr03
Copy link
Member Author

0xpr03 commented Aug 8, 2022

I'll leave an updated documentation to #395

@0xpr03 0xpr03 requested a review from JohnTitor August 8, 2022 20:52
@0xpr03 0xpr03 marked this pull request as ready for review August 8, 2022 20:52
@0xpr03 0xpr03 force-pushed the optional_crossbeam branch 12 times, most recently from 74c6903 to fcd5643 Compare August 8, 2022 21:25
@0xpr03
Copy link
Member Author

0xpr03 commented Aug 8, 2022

One downside of this approach is that users will have to use

notify = { version = "5", default-features = false, feature=["macos_kqueue"] }

Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

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

Looks like a good approach!

One downside of this approach is that users will have to use

notify = { version = "5", default-features = false, feature=["macos_kqueue"] }

I think it's acceptable as only those who want to disable it have to touch the file, that is existing users that don't have the crossbeam issue don't have to tweak their Cargo.toml.

@nicoburns
Copy link

I wonder whether it would be worth switching wholesale to something like https://docs.rs/flume/latest/flume/ ? I believe it has very similar performance to crossbeam_channel. It might cause the same problem of course.

@0xpr03
Copy link
Member Author

0xpr03 commented Aug 9, 2022

Unbounded MPSC seems to be the only case crossbeam-channel doesn't win over flume according to their benchmarks. Replacing our mpsc stuff with spsc would probably make more sense.

As far as I know parts of crossbeam are considered for moving into std, which would give it more credibility.

One major reason for using corssbeam-channel is probably the thread parking when waiting for channel events. Mixed up parking_lot with crossbeam-channel. CC explicitly uses more spinning to get higher performance.

@0xpr03
Copy link
Member Author

0xpr03 commented Aug 9, 2022

If flume does prevent tokio errors, I'd be willing to accept a PR. For now my main concern is resolving the tokio scheduler issues.

This PR makes it very easy to switch it out, so if any one wants to test this, feel free. As far as I know, you can't instantly trigger this scheduler hang, so testing help is welcome for this.

Edit: Just to clairfy: Flume looks good (I've never heard of it previously), but for now I'd prefer to finish #249 once and for all. Then we can still introduce flume later on. Crossbeam seems also stuck in 0.5 limbo, but at least is has a very big userbase, so I don't fear breaking much leaving it in.

@0xpr03 0xpr03 merged commit cf37946 into main Aug 9, 2022
@0xpr03 0xpr03 deleted the optional_crossbeam branch August 9, 2022 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crossbeam breaks tokio::spawn
3 participants