Skip to content

Replace crossbeam with std::sync::mpsc #7346

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

Closed
wants to merge 5 commits into from

Conversation

OneFourth
Copy link
Contributor

@OneFourth OneFourth commented Jan 23, 2023

Objective

Fixes #7153.

Solution

Remove crossbeam_channel as a dependency and replace it with std::sync::mpsc instead.

A one-to-one conversion was not possible because crossbeam_channel supports Sync while the Receiver in std::sync::mpsc does not, so I tried to make use of bevy_utils::sync_cell::SyncCell.

bevy_asset had many many issues with this though, I tried to sort through them but I couldn't figure out what to do now or I'm taking the wrong approach.

Current problems

  • I tried to get around Arc<RefChangeChannel> needing to be mut now with SyncCell by wrapping the inner RefChangeChannel in a Mutex, but this results in future cannot be sent between threads safetly in AssetServer::load_untracked.
  • impl Clone for Handle is also a problem, since we don't have a mut borrow of self
  • A lot more places where we need mut now

Changelog

Removed crossbeam_channel dependency.
Replaced crossbeam_channel usage with std::sync::mpsc.

Migration Guide

Unsure

@james7132 james7132 added the C-Dependencies A change to the crates that Bevy depends on label Jan 23, 2023
@alice-i-cecile alice-i-cecile added A-Assets Load files from disk to use for things like images, models, and sounds A-Tasks Tools for parallel and async work D-Complex Quite challenging from either a design or technical perspective. Ask for help! labels Jan 23, 2023
@james7132 james7132 added the S-Blocked This cannot move forward until something else changes label Jan 24, 2023
@joseph-gio joseph-gio removed the S-Blocked This cannot move forward until something else changes label Jan 26, 2023
@joseph-gio
Copy link
Member

Rust 1.67 is out, so this is now unblocked.

@james7132 james7132 added the S-Blocked This cannot move forward until something else changes label Jan 26, 2023
@james7132
Copy link
Member

Rust 1.67 is out, so this is now unblocked.

Going to block this until we bump the MSRV to 1.67, otherwise we actually have a period where we are allowing the use of the older std::sync::mpsc implementation.

@james7132
Copy link
Member

@OneFourth do you think you can drive this to completion? Seemed like you weren't when you mentioned it in the issue. If not, I can mark this up for adoption.

@OneFourth
Copy link
Contributor Author

@james7132 I can't, definitely out of my depth with trying to get bevy_asset to compile, feel free to mark it up for adoption! I hope the other commits are still useful at least

@james7132 james7132 added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Jan 26, 2023
bors bot pushed a commit that referenced this pull request Jan 27, 2023
# Objective
Bump the MSRV to 1.67. Enable cleanup PRs like #7346 to work.

## Solution
Bump it to 1.67

---

## Changelog
Changed: The MSRV of the engine is now 1.67.
@MinerSebas
Copy link
Contributor

I am wary that this change should be done at this Point in Time.

  1. This doesn't remove crossbeam from the dependency graph, as notify and tracing-chrome still depend on it. (But it should be also mentioned that both are only optional dependencies themselves).
  2. std::sync::mpsc not being Sync seems to complicate the required Implementation. (Introduction of Mutex and SyncCell which require locking, switching Res to `ResMut potentially hurting parralelism)

For these Reasons, I would not merge this PR yet, and even revert #7379, as the only practical reason to require Rust 1.67.0 is this yet unmerged PR.
If #7379 were reverted, but the consensus is to merge this PR anyway, then the MSRV bump can still happen in this PR.

@ryrony123
Copy link

ryrony123 commented Jan 27, 2023 via email

ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective
Bump the MSRV to 1.67. Enable cleanup PRs like bevyengine#7346 to work.

## Solution
Bump it to 1.67

---

## Changelog
Changed: The MSRV of the engine is now 1.67.
@stephenmartindale
Copy link
Contributor

I tend to agree with @MinerSebas

Does anyone know why std::sync::mpsc::Sender (et al.) wasn't made Sync after 1.67?

2. `std::sync::mpsc` not being Sync seems to complicate the required Implementation. (Introduction of Mutex and SyncCell which require locking, switching `Res` to `ResMut potentially hurting parralelism)

For these Reasons...

The claim in the Rust 1.67 change-log that boasts that mpsc was "switched out" with crossbeam is a tad far-fetched.

crossbeam_channel::Sender is both Send and Sync and std::sync::mpsc::Sender is !Sync and that makes ALL the difference in the world.

Receiver is no matter because there should be only one owner of that end and that owner typically wants it mut, anyway, but requiring a Mutex or some other lock to clone the Sender voids the entire purpose of the channel in the first place!

I can think of some hacks of the Really Nasty variety that could work around this but they would not only be nasty: there is no point replacing crossbeam_channel if user-code just adds it back out of necessity.

@james7132
Copy link
Member

After trying to adopt this PR myself, I am inclined to agree. It's unfortunate that the stdlib cannot be updated to reflect this. We may need to wait for a std::sync:channel alternative. In the meantime, I think it's best that we close this PR and the original issue, as it's unclear if this will be possible even in the indefinite future.

With that said, @OneFourth, thanks for exploring this space for us. Really appreciate the effort done here, even if it didn't lead to concrete changes, we've learned quite a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds A-Tasks Tools for parallel and async work C-Dependencies A change to the crates that Bevy depends on D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! S-Blocked This cannot move forward until something else changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace use of crossbeam_channel with std::sync::mpsc
7 participants