Remove crossbeam-channel as a runtime dependency #13048
Conversation
james7132
added
A-Assets
james7132
force-pushed
the
remove-crossbeam-channel
branch
from
ba75a26 to
3542d1f
Compare
|
Performance-wise how do these compare? Are there benchmarks of crossbeam vs concurrent-queue anywhere under different load conditions? Some usages of these (such as the ID lifecycle stuff) might be affected by perf regressions. I'm in favor of removing dependencies, but we're notably taking on new code with the wrappers, and increasing the number of queue variants (ConcurrentQueue, Arc, EventQueue, EventReader, EventSender, instead of just Sender/Receiver), making this feel a bit more like a lateral move (I do acknowledge the benefits stated above though). But combine that with the "perf" question and I start to question why we're rocking the boat here. |
|
I don't have time to review this right now, but I think this PR is probably worth doing due to the fact that |
| /// Queue resource used to receive time from the render world. | ||
| #[derive(Resource, Clone)] | ||
| pub struct TimeEventQueue(pub Arc<ConcurrentQueue<Instant>>); |
There was a problem hiding this comment.
Seeing the transition from "channel" -> Arc<ConcurrentQueue> does incline me to echo cart's concerns about rocking the boat. It's probably still worth it just to remove a spinny dependency though.
Should probably have a different name though since this doesn't wrap the EventQueue type
| pub fn create_time_event_queue() -> TimeEventQueue { | ||
| // bound the channel to 2 since when pipelined the render phase can finish before | ||
| // the time system runs. | ||
| let (s, r) = crossbeam_channel::bounded::<Instant>(2); | ||
| (TimeSender(s), TimeReceiver(r)) | ||
| TimeEventQueue(Arc::new(ConcurrentQueue::bounded(2))) | ||
| } |
There was a problem hiding this comment.
I think this can just be a FromWorld impl
|
|
||
| /// A strictly non-blocking sender for a multi-producer multi-consumer channel. | ||
| #[derive(Debug)] | ||
| pub struct EventSender<T>(Arc<ConcurrentQueue<T>>); |
There was a problem hiding this comment.
Can we make this private? I'd prefer not to expose a reimplementation of a channel
| pub(crate) struct AssetIndexAllocator { | ||
| /// A monotonically increasing index. | ||
| next_index: AtomicU32, | ||
| recycled_queue_sender: Sender<AssetIndex>, | ||
| recycled_queue: ConcurrentQueue<AssetIndex>, | ||
| /// This receives every recycled [`AssetIndex`]. It serves as a buffer/queue to store indices ready for reuse. | ||
| recycled_queue_receiver: Receiver<AssetIndex>, | ||
| recycled_sender: Sender<AssetIndex>, | ||
| recycled_receiver: Receiver<AssetIndex>, | ||
| recycled: ConcurrentQueue<AssetIndex>, |
There was a problem hiding this comment.
I think this refactor is 100% worth doing, regardless of whether we end up removing the other channels. Right here the queues have clear a clear owner so it only makes sense to remove the indirection incurred by the channels
| use concurrent_queue::ConcurrentQueue; | ||
| use std::sync::Arc; | ||
|
|
||
| pub(crate) struct EventQueue<T>(Arc<ConcurrentQueue<T>>); |
There was a problem hiding this comment.
If we make this module private maybe this could just be a type alias?
| pub(crate) struct EventQueue<T>(Arc<ConcurrentQueue<T>>); | |
| pub(crate) type EventQueue<T> = Arc<ConcurrentQueue<T>>; |
janhohenheim
added
the
S-Needs-Review
BenjaminBrienen
added
S-Waiting-on-Author
Objective
Fixes #7153.
crossbeam-channelis a great crate, but it may not be the right fit for a lot of Bevy's use cases. It inherently allocates an inner representation, and uses an Arc-like indirection to access it via the sender and receiver. This might be useful when we want to separate the two and move them to their own tasks on different threads, but this may represent additional overhead when working with the channel. We also tend to only usetry_sendandtry_recvto avoid blocking at all costs. This makes sense given that we (almost) never want to block the current thread and wait on the other side to be populated.Solution
Remove all runtime dependency on
crossbeam-channel, and replace it with theconcurrent-queue, which is already in use by bevy_ecs, bevy_tasks, and by a good chunk of the async-* dependencies Bevy is already using. Where absolutely necessary, the queue will be wrapped in an Arc to provide a similar interface as before.The queue provides access to both ends of the queue from a single interface, does not require internal indirection, pushes and pops are strictly non-blocking, provides a similar internal implementation to that of crossbeam-channel, and allows us to remove one more dependency when compiling Bevy.
One potential improvement is to explicitly wrap the case of unclosable and unbounded queues in an Arc to create an infallible event queue.
Changelog
TODO
Migration Guide
TODO