-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 rayon as the bevy_task thread pool #11995
base: main
Are you sure you want to change the base?
Conversation
We should investigate this comment for running async on rayon rayon-rs/rayon#679 (comment) as an alternative to the block_on. |
Given the slightly cursed things we're doing to support scopes with external executors, I'm not confident that would be 100% sufficient, but it definitely is a cleaner approach overall. |
This can all go away, once we remove non send data from the world. |
I got this working, and the results are a bit mixed right now. The Good
You can immediately see the results in the trace, as the threads are spinning up faster than with just async_executor. You can see that the app as a whole is using almost all of the available threads, even for schedules with lots of systems with little work to do in each. The BadEvery schedule takes longer to run as a whole. Here's the comparison for the Render schedule: This is either stemming from the higher contention from spinning up and turning down so many threads, the alterations to |
I did some of my own investigation. One thing I did notice is it seems to take longer for the time system to start running in First after the first run of the multithreaded executor. It wasn't by a ton, maybe 5us. Seems likely this is due to contention. One other note is that it seems very aggressive at starting more threads. Enough so that I'm wondering if spawn_async is misbehaving and starting too many threads. Will investigate more later. |
AFAIK no, @james7132 when you tested with rayon-rs/rayon#679 (comment) 's suggestion were you still running that |
I was using the fork of block_on that is in this PR. It still uses parking. As far as I can tell, there's no way to yield to rayon that allows it to go to sleep, so our use of block_on may need to use something else. |
@@ -611,6 +532,10 @@ pub struct Scope<'scope, 'env: 'scope, T> { | |||
} | |||
|
|||
impl<'scope, 'env, T: Send + 'scope> Scope<'scope, 'env, T> { | |||
pub fn spawn(&self, f: impl FnOnce() + Send + 'scope) { | |||
self.thread_pool.spawn(|_| f()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't quite right as the scope async executors can end while there are still tasks on the rayon scope. We need to push something into self.spawned. An easy fix here is to use spawn_async.
where | ||
T: Send + 'static, | ||
{ | ||
Task::new(self.executor.spawn(future)) | ||
let (runnable, task) = async_task::spawn(future, |runnable: Runnable| { | ||
rayon_core::spawn(move || { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that rayon_core::spawn
spawns the task on whatever rayon context is the current one in the thread it is called in. Most notably, the async_task
closure may be called on a different thread than the one the task was spawned in (e.g. if the Future
's Waker
was moved into a different thread), in which case this would likely initialize a different global rayon's context, with a different thread pool. It should instead spawn on the current thread pool (with self.thread_pool.spawn(...)
)
Objective
This is a draft. There is zero intent to merge this until there is some form of consensus on general direction
This is one potential option for dealing with #6941 and #10064, as well as a general alternative to #11492.
This PR aims to lay the foundations for using rayon as Bevy's core task executor, as well as support the very powerful APIs built on top of it (i.e.
ParallelIterator
and friends).Background
Bevy's tasks used to be based on rayon. However, in #1525, we shifted to use
async_executor
and our own hand-rolled thread pool. The are several reasons for that change:async
/await
is non-existent, and still effectively is not supported in any first party way.SharedArrayBuffer
on web platforms.However, losing rayon has actually caused us to lose a significant amount of performance, and the powerful APIs associated with rayon. With rayon being one of the best examples of high data parallelism and ergonomics in the Rust ecosystem, this is potentially leaving a lot on the table.
Rayon has since added support for WASM, and in this PR, I am going to attempt to bridge the gap to allow us to run async tasks in the same thread pool.
Solution
This PR replaces our thread pool with
rayon::ThreadPool
, and hand create our own async task that schedules using rayon. We then forkfutures_lite::block_on
to avoid parking the thread, usingrayon_core::yield_now
to run rayon's tasks whenever the local task yields. This should enable us to get the performance benefits and API support from Rayon, without losing our capability to schedule async tasks onto the same threads.Not implemented in this PR is to move all non-async tasks (i.e. blocking CPU-bound operations on the ComputeTaskPool) to queue onto rayon instead of async_executor.
Current Unknowns
block_on
, will Rayon be able to resume a thread that has been parked by the current implementation ofblock_on
?Do we still see the benefits of rayon's thread management and contention mitigation?It's definitely there, but we may have contention issues in the actual tasks being run.Can we get into the internals of rayon to try to merge these two work stealing queues together?We don't have access to the internals, but we don't need that to use only one top level work stealing queue.