-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
[Merged by Bors] - Remove task_pool parameter from par_for_each(_mut) #4705
Conversation
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 is really nice, it was always annoying to need to add a parameter to a system if you wanted to convert it to a parallel query.
Could you add a short code snippet with a before and after in the migration guide section? It's not a big deal, but it makes writing the full migration guide much easier when examples are already taken care of.
Done! |
edit: nvm this was me missing that the task pools were cloned. |
This probably adds a small perf cost for cloning the task pool to each query even if it isn't being used. Could you run benches to see? |
The clone was removed. This only affects queries that use the |
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@james7132 can you:
|
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@@ -61,6 +62,9 @@ impl<Q: WorldQuery, F: WorldQuery> QueryState<Q, F> { | |||
|
|||
let mut state = Self { | |||
world_id: world.id(), | |||
task_pool: world |
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 clone is going to cause a subtle bug if anyone tries to replace the compute pool during running. The par_for_each is going ignore the pool being replaced and continue to use the original compute pool.
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.
The more I think about this, the less such an operation makes sense. Even more reason to make TaskPools global and initialized once instead of allowing arbitrary reconfiguration mid execution.
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.
agreed, but since this is one of the objections against global task pools, I'm not sure we can merge this without more clarity there.
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.
Thinking on this a bit more, this isn't too different from the status quo. You can already spawn a task, embed a clone of the TaskPool it's running on, and have it run forever. Replacing the resource would spawn a new TaskPool, but the former pool wouldn't be cleaned up.
Is this a problem? Yes. Is it one solvable within this PR? No. Does integrating this into ECS exacerbate the issue? Probably. Is it worth blocking this change on it? I don't think so. However, in turn, we should prioritize reviewing and merging #2250 or some variant of it to address this issue.
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.
bors r+
# Objective Fixes #3183. Requiring a `&TaskPool` parameter is sort of meaningless if the only correct one is to use the one provided by `Res<ComputeTaskPool>` all the time. ## Solution Have `QueryState` save a clone of the `ComputeTaskPool` which is used for all `par_for_each` functions. ~~Adds a small overhead of the internal `Arc` clone as a part of the startup, but the ergonomics win should be well worth this hardly-noticable overhead.~~ Updated the docs to note that it will panic the task pool is not present as a resource. # Future Work If bevyengine/rfcs#54 is approved, we can replace these resource lookups with a static function call instead to get the `ComputeTaskPool`. --- ## Changelog Removed: The `task_pool` parameter of `Query(State)::par_for_each(_mut)`. These calls will use the `World`'s `ComputeTaskPool` resource instead. ## Migration Guide The `task_pool` parameter for `Query(State)::par_for_each(_mut)` has been removed. Remove these parameters from all calls to these functions. Before: ```rust fn parallel_system( task_pool: Res<ComputeTaskPool>, query: Query<&MyComponent>, ) { query.par_for_each(&task_pool, 32, |comp| { ... }); } ``` After: ```rust fn parallel_system(query: Query<&MyComponent>) { query.par_for_each(32, |comp| { ... }); } ``` If using `Query(State)` outside of a system run by the scheduler, you may need to manually configure and initialize a `ComputeTaskPool` as a resource in the `World`.
# Objective Fixes bevyengine#3183. Requiring a `&TaskPool` parameter is sort of meaningless if the only correct one is to use the one provided by `Res<ComputeTaskPool>` all the time. ## Solution Have `QueryState` save a clone of the `ComputeTaskPool` which is used for all `par_for_each` functions. ~~Adds a small overhead of the internal `Arc` clone as a part of the startup, but the ergonomics win should be well worth this hardly-noticable overhead.~~ Updated the docs to note that it will panic the task pool is not present as a resource. # Future Work If bevyengine/rfcs#54 is approved, we can replace these resource lookups with a static function call instead to get the `ComputeTaskPool`. --- ## Changelog Removed: The `task_pool` parameter of `Query(State)::par_for_each(_mut)`. These calls will use the `World`'s `ComputeTaskPool` resource instead. ## Migration Guide The `task_pool` parameter for `Query(State)::par_for_each(_mut)` has been removed. Remove these parameters from all calls to these functions. Before: ```rust fn parallel_system( task_pool: Res<ComputeTaskPool>, query: Query<&MyComponent>, ) { query.par_for_each(&task_pool, 32, |comp| { ... }); } ``` After: ```rust fn parallel_system(query: Query<&MyComponent>) { query.par_for_each(32, |comp| { ... }); } ``` If using `Query(State)` outside of a system run by the scheduler, you may need to manually configure and initialize a `ComputeTaskPool` as a resource in the `World`.
# Objective Fixes bevyengine#3183. Requiring a `&TaskPool` parameter is sort of meaningless if the only correct one is to use the one provided by `Res<ComputeTaskPool>` all the time. ## Solution Have `QueryState` save a clone of the `ComputeTaskPool` which is used for all `par_for_each` functions. ~~Adds a small overhead of the internal `Arc` clone as a part of the startup, but the ergonomics win should be well worth this hardly-noticable overhead.~~ Updated the docs to note that it will panic the task pool is not present as a resource. # Future Work If bevyengine/rfcs#54 is approved, we can replace these resource lookups with a static function call instead to get the `ComputeTaskPool`. --- ## Changelog Removed: The `task_pool` parameter of `Query(State)::par_for_each(_mut)`. These calls will use the `World`'s `ComputeTaskPool` resource instead. ## Migration Guide The `task_pool` parameter for `Query(State)::par_for_each(_mut)` has been removed. Remove these parameters from all calls to these functions. Before: ```rust fn parallel_system( task_pool: Res<ComputeTaskPool>, query: Query<&MyComponent>, ) { query.par_for_each(&task_pool, 32, |comp| { ... }); } ``` After: ```rust fn parallel_system(query: Query<&MyComponent>) { query.par_for_each(32, |comp| { ... }); } ``` If using `Query(State)` outside of a system run by the scheduler, you may need to manually configure and initialize a `ComputeTaskPool` as a resource in the `World`.
Objective
Fixes #3183. Requiring a
&TaskPool
parameter is sort of meaningless if the only correct one is to use the one provided byRes<ComputeTaskPool>
all the time.Solution
Have
QueryState
save a clone of theComputeTaskPool
which is used for allpar_for_each
functions.Adds a small overhead of the internalArc
clone as a part of the startup, but the ergonomics win should be well worth this hardly-noticable overhead.Updated the docs to note that it will panic the task pool is not present as a resource.
Future Work
If bevyengine/rfcs#54 is approved, we can replace these resource lookups with a static function call instead to get the
ComputeTaskPool
.Changelog
Removed: The
task_pool
parameter ofQuery(State)::par_for_each(_mut)
. These calls will use theWorld
'sComputeTaskPool
resource instead.Migration Guide
The
task_pool
parameter forQuery(State)::par_for_each(_mut)
has been removed. Remove these parameters from all calls to these functions.Before:
After:
If using
Query(State)
outside of a system run by the scheduler, you may need to manually configure and initialize aComputeTaskPool
as a resource in theWorld
.