-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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] - Copy TaskPool resoures to subapps #4792
Conversation
This makes sense to me. The task pool configuration should be managing all the threads in the app. Ran many_cubes. Unsurprisingly seeing a small perf hit with this change. This is mostly expected since on my machine (12 logical cores) there are only 6 compute threads available, while on main the default pool was allocating 12 to the render world. The perf regression will be fixed by #4740. We could potentially roll up this change with that PR. I'm also ok merging this in by itself too, since the behavior isn't currently correct. |
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 clearly the correct behavior. Generating a new taskpool for each subapp is too inflexible and surprising.
bors r+ |
# 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.
# 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.
# 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.
Objective
Fixes #4791.
ParallelExecutor
inserts a defaultCompteTaskPool
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'sComputeTaskPool
is not cloned and inserted into the render app's, which causes a secondComputeTaskPool
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.