Merged
Conversation
bors bot
pushed a commit
to bevyengine/bevy
that referenced
this pull request
May 30, 2022
# 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`.
bors bot
pushed a commit
to bevyengine/bevy
that referenced
this pull request
May 30, 2022
# Objective Fixes #4791. `ParallelExecutor` inserts a default `CompteTaskPool` if there isn't one stored as a resource, including when it runs on a different world. When spawning the render sub-app, the main world's `ComputeTaskPool` is not cloned and inserted into the render app's, which causes a second `ComputeTaskPool` with the default configuration to be spawned. This results in an excess number of threads being spawned. ## Solution Copy the task pools from the main world to the subapps upon creating them. ## Alternative An alternative to this would be to make the task pools global, as seen in #2250 or bevyengine/rfcs#54.
cart
approved these changes
May 30, 2022
Member
cart
left a comment
cart
left a comment
There was a problem hiding this comment.
I'm convinced! The ergonomics wins are hard to ignore and given that this is pretty much the pattern adopted everywhere, it seems like a safe bet. Globals do create "conflict" problems and feel more than a little bit dirty. I generally don't want to embrace this pattern in Bevy, but task pools do seem like an exceptional case. As we've recently discussed, I do think that the unified task pool in bevyengine/bevy#4740 is the right general direction, but I'm comfortable merging this in the current state as it feels orthogonal from a design standpoint.
james7132
added a commit
to james7132/bevy
that referenced
this pull request
Jun 7, 2022
# 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`.
james7132
added a commit
to james7132/bevy
that referenced
this pull request
Jun 7, 2022
# Objective Fixes bevyengine#4791. `ParallelExecutor` inserts a default `CompteTaskPool` if there isn't one stored as a resource, including when it runs on a different world. When spawning the render sub-app, the main world's `ComputeTaskPool` is not cloned and inserted into the render app's, which causes a second `ComputeTaskPool` with the default configuration to be spawned. This results in an excess number of threads being spawned. ## Solution Copy the task pools from the main world to the subapps upon creating them. ## Alternative An alternative to this would be to make the task pools global, as seen in bevyengine#2250 or bevyengine/rfcs#54.
LegNeato
pushed a commit
to LegNeato/bevy-rfcs
that referenced
this pull request
Jul 4, 2022
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
# 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`.
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
# Objective Fixes bevyengine#4791. `ParallelExecutor` inserts a default `CompteTaskPool` if there isn't one stored as a resource, including when it runs on a different world. When spawning the render sub-app, the main world's `ComputeTaskPool` is not cloned and inserted into the render app's, which causes a second `ComputeTaskPool` with the default configuration to be spawned. This results in an excess number of threads being spawned. ## Solution Copy the task pools from the main world to the subapps upon creating them. ## Alternative An alternative to this would be to make the task pools global, as seen in bevyengine#2250 or bevyengine/rfcs#54.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RENDERED
This proposes changes to the way the three main TaskPools are initialized and used in Bevy.