Nesting task pool's Scopes
#4301
Comments
MiniaczQ
added
C-Feature
|
I don't know if its related but thread::scope(|s| {
s.spawn(|| {
println!("hello from the first scoped thread");
s.spawn(|| println!("nested"));
});
});https://doc.rust-lang.org/nightly/std/thread/fn.scope.html |
|
That isn't identical to nested scopes, as the nested spawns can still only borrow locals defined outside the outermost scope. |
Either way would be fine for what I personally need (spawning system tasks from executor task). I should look into what |
|
I hit an unusual error (in the context of nesting scopes), because of the So decision has to be made:
Do you think there are other options? |
|
As @maniwani suggested I'll proceed with 3rd solution: pool.scope(|scope, local_scope| {
let local_task = |scope, local_scope| async {
let nested_local_task = |scope, local_scope| async {
/*...*/
};
local_scope.spawn(nested_local_task(scope, local_scope));
let nested_task = |scope| async {
/*...*/
};
scope.spawn(nested_task(scope));
};
local_scope.spawn(local_task(scope, local_scope));
let task = |scope| async {
let nested_task = |scope| async {
/*...*/
};
scope.spawn(nested_task(scope));
}
scope.spawn(task(scope))
}); |
|
As a note you can do nested spawns this way with the current API. fn test_nested_scopes() {
let pool = Arc::new(TaskPool::new());
pool.clone().scope(|scope| {
scope.spawn_local(async move {
pool.scope(|scope| {
scope.spawn(async move {
});
});
});
});
}This isn't ideal as the parallel executor would have to run on a locally spawned thread and non send systems would end up blocking the spawning of parallel tasks. It would be preferred if we could have nested spawning instead. I don't think the mutex approach taken in #4343 can work. To drive the tasks forward we need to take a lock on the tasks which will prevent the currently running tasks from spawning any new tasks. The work from @TheRawMeatball https://github.com/hymm/bevy/blob/taskpool-scope-shenanigans/crates/bevy_tasks/src/task_pool.rs is promising. I cleaned up a couple things on my branch, but I don't think we can use channels to pass the results as it doesn't have a stable ordering and there are some tests that expect it to be stable. (i.e. the order that tasks are queued in is the order the results are returned). I'll probably keep poking at this a bit more and see if I can come up with something that has a stable order. |
|
Alternatively, we could simply remove the stable order guarantee? Furthermore, it might actually be necessary to remove this guarantee as when tasks can be spawned from multiple threads at once ordering is bound to be lost anyways. |
|
The place where stable ordering would be important is with Internally bevy doesn't depend on scope returning a stable ordering. Most uses don't expect any data to be returned. The one usage where it does return data is in the gltf loader and that promptly uses for_each. I do agree that as soon as you start using nested spawns any stable ordering is thrown out the window. We would only be able to guarantee a stable ordering in cases where you only spawn things directly from the closure. |
|
Then personally I'd prefer simply removing the ParallelSlice iterator etc. as they can't really be used anyways and move forward. We can think about a stable-ordered alternative later on when we need it. |
Progress UpdateI have a branch with https://github.com/hymm/bevy/blob/taskpool-scope-shenanigans/ has an issue where all the tasks aren't completing in time when I use nested spawns in one of my tests. Both approaches have some UB too. Turns out that sticking // this test should fail to compile
// TODO: once it does fail move it to a compile fail doc test.
#[test]
fn compile_fail() {
let pool = TaskPool::new();
let foo = Box::new(42);
pool.scope(|scope| {
std::thread::spawn(move || {
// UB. This could spawn on the scope after `.scope` returns and the internal Scope is dropped.
scope.spawn(async move {
assert_eq!(*foo, 42);
});
});
});
} |
MiniaczQ
changed the title
Scopes through interior mutabilityScopes
aevyrie
added
A-Tasks
|
Mistook this for a PR... |
# Objective - Add ability to create nested spawns. This is needed for stageless. The current executor spawns tasks for each system early and runs the system by communicating through a channel. In stageless we want to spawn the task late, so that archetypes can be updated right before the task is run. The executor is run on a separate task, so this enables the scope to be passed to the spawned executor. - Fixes bevyengine#4301 ## Solution - Instantiate a single threaded executor on the scope and use that instead of the LocalExecutor. This allows the scope to be Send, but still able to spawn tasks onto the main thread the scope is run on. This works because while systems can access nonsend data. The systems themselves are Send. Because of this change we lose the ability to spawn nonsend tasks on the scope, but I don't think this is being used anywhere. Users would still be able to use spawn_local on TaskPools. - Steals the lifetime tricks the `std::thread::scope` uses to allow nested spawns, but disallow scope to be passed to tasks or threads not associated with the scope. - Change the storage for the tasks to a `ConcurrentQueue`. This is to allow a &Scope to be passed for spawning instead of a &mut Scope. `ConcurrentQueue` was chosen because it was already in our dependency tree because `async_executor` depends on it. - removed the optimizations for 0 and 1 spawned tasks. It did improve those cases, but made the cases of more than 1 task slower. --- ## Changelog Add ability to nest spawns ```rust fn main() { let pool = TaskPool::new(); pool.scope(|scope| { scope.spawn(async move { // calling scope.spawn from an spawn task was not possible before scope.spawn(async move { // do something }); }); }) } ``` ## Migration Guide If you were using explicit lifetimes and Passing Scope you'll need to specify two lifetimes now. ```rust fn scoped_function<'scope>(scope: &mut Scope<'scope, ()>) {} // should become fn scoped_function<'scope>(scope: &Scope<'_, 'scope, ()>) {} ``` `scope.spawn_local` changed to `scope.spawn_on_scope` this should cover cases where you needed to run tasks on the local thread, but does not cover spawning Nonsend Futures. ## TODO * [x] think real hard about all the lifetimes * [x] add doc about what 'env and 'scope mean. * [x] manually check that the single threaded task pool still works * [x] Get updated perf numbers * [x] check and make sure all the transmutes are necessary * [x] move commented out test into a compile fail test * [x] look through the tests for scope on std and see if I should add any more tests Co-authored-by: Michael Hsu <[email protected]> Co-authored-by: Carter Anderson <[email protected]>
# Objective - Add ability to create nested spawns. This is needed for stageless. The current executor spawns tasks for each system early and runs the system by communicating through a channel. In stageless we want to spawn the task late, so that archetypes can be updated right before the task is run. The executor is run on a separate task, so this enables the scope to be passed to the spawned executor. - Fixes bevyengine#4301 ## Solution - Instantiate a single threaded executor on the scope and use that instead of the LocalExecutor. This allows the scope to be Send, but still able to spawn tasks onto the main thread the scope is run on. This works because while systems can access nonsend data. The systems themselves are Send. Because of this change we lose the ability to spawn nonsend tasks on the scope, but I don't think this is being used anywhere. Users would still be able to use spawn_local on TaskPools. - Steals the lifetime tricks the `std::thread::scope` uses to allow nested spawns, but disallow scope to be passed to tasks or threads not associated with the scope. - Change the storage for the tasks to a `ConcurrentQueue`. This is to allow a &Scope to be passed for spawning instead of a &mut Scope. `ConcurrentQueue` was chosen because it was already in our dependency tree because `async_executor` depends on it. - removed the optimizations for 0 and 1 spawned tasks. It did improve those cases, but made the cases of more than 1 task slower. --- ## Changelog Add ability to nest spawns ```rust fn main() { let pool = TaskPool::new(); pool.scope(|scope| { scope.spawn(async move { // calling scope.spawn from an spawn task was not possible before scope.spawn(async move { // do something }); }); }) } ``` ## Migration Guide If you were using explicit lifetimes and Passing Scope you'll need to specify two lifetimes now. ```rust fn scoped_function<'scope>(scope: &mut Scope<'scope, ()>) {} // should become fn scoped_function<'scope>(scope: &Scope<'_, 'scope, ()>) {} ``` `scope.spawn_local` changed to `scope.spawn_on_scope` this should cover cases where you needed to run tasks on the local thread, but does not cover spawning Nonsend Futures. ## TODO * [x] think real hard about all the lifetimes * [x] add doc about what 'env and 'scope mean. * [x] manually check that the single threaded task pool still works * [x] Get updated perf numbers * [x] check and make sure all the transmutes are necessary * [x] move commented out test into a compile fail test * [x] look through the tests for scope on std and see if I should add any more tests Co-authored-by: Michael Hsu <[email protected]> Co-authored-by: Carter Anderson <[email protected]>
# Objective - Add ability to create nested spawns. This is needed for stageless. The current executor spawns tasks for each system early and runs the system by communicating through a channel. In stageless we want to spawn the task late, so that archetypes can be updated right before the task is run. The executor is run on a separate task, so this enables the scope to be passed to the spawned executor. - Fixes bevyengine#4301 ## Solution - Instantiate a single threaded executor on the scope and use that instead of the LocalExecutor. This allows the scope to be Send, but still able to spawn tasks onto the main thread the scope is run on. This works because while systems can access nonsend data. The systems themselves are Send. Because of this change we lose the ability to spawn nonsend tasks on the scope, but I don't think this is being used anywhere. Users would still be able to use spawn_local on TaskPools. - Steals the lifetime tricks the `std::thread::scope` uses to allow nested spawns, but disallow scope to be passed to tasks or threads not associated with the scope. - Change the storage for the tasks to a `ConcurrentQueue`. This is to allow a &Scope to be passed for spawning instead of a &mut Scope. `ConcurrentQueue` was chosen because it was already in our dependency tree because `async_executor` depends on it. - removed the optimizations for 0 and 1 spawned tasks. It did improve those cases, but made the cases of more than 1 task slower. --- ## Changelog Add ability to nest spawns ```rust fn main() { let pool = TaskPool::new(); pool.scope(|scope| { scope.spawn(async move { // calling scope.spawn from an spawn task was not possible before scope.spawn(async move { // do something }); }); }) } ``` ## Migration Guide If you were using explicit lifetimes and Passing Scope you'll need to specify two lifetimes now. ```rust fn scoped_function<'scope>(scope: &mut Scope<'scope, ()>) {} // should become fn scoped_function<'scope>(scope: &Scope<'_, 'scope, ()>) {} ``` `scope.spawn_local` changed to `scope.spawn_on_scope` this should cover cases where you needed to run tasks on the local thread, but does not cover spawning Nonsend Futures. ## TODO * [x] think real hard about all the lifetimes * [x] add doc about what 'env and 'scope mean. * [x] manually check that the single threaded task pool still works * [x] Get updated perf numbers * [x] check and make sure all the transmutes are necessary * [x] move commented out test into a compile fail test * [x] look through the tests for scope on std and see if I should add any more tests Co-authored-by: Michael Hsu <[email protected]> Co-authored-by: Carter Anderson <[email protected]>
What problem does this solve or what need does it fill?
Currently scopes cannot be nested, which means tasks spawned in a scope cannot spawn more tasks.
What solution would you like?
Modify
Scopedefinition (forwasmand non-wasm) to use interior mutability.That way many spawn-able scopes can be passed deeper.
This in both current cases means wrapping the
resultsvector with a thread-safe interior mutability type, likeMutexorRwLock.What alternative(s) have you considered?
A alternative to
scopesmentioned in #4287 could cover some of the nesting use cases.Additional context
This solution was used in other crates such as
crossbeamThe text was updated successfully, but these errors were encountered: