Skip to content
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

Optimize TaskPools for use in static variables. #12990

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

james7132
Copy link
Member

@james7132 james7132 commented Apr 16, 2024

Objective

Fixes #11849. async_executor internally locks a Mutex each time it spawns and finishes a task. This adds a significant amount of overhead for any operation interacting with the task pools, even in single-threaded cases. For more information, see smol-rs/async-executor#112.

Solution

Create a const-constructible version of TaskPool that uses StaticExecutor instead of Executor. Use it to back the static TaskPools instead. This also eliminates the OnceLock on trying to fetch the task pool as the a raw 'static borrow on the static variable can be returned without issue. This solution is loosely based on the work in #4740 to specialize a fork of async_executor for Bevy.

This is blocked on async_executor and concurrent_queue pushing new releases with the necessary changes.

TODO: This PR duplicates a huge amount of code currently due to the change in the lifetime requirements on self needed to use StaticExecutor's API.

TODO: The docs are still copied over from TaskPool and needs to be rewritten.

Performance

See the benchmarks in smol-rs/async-executor#112. For both single threaded and multithreaded use cases, the overhead of launching, ticking, and finishing tasks is lower, potentially upwards of 9x lower in multithreaded cases under contention.

Future Work

To lower the overhead other use cases like scopes, the LocalExecutors and ThreadExecutors may also benefit from being replaced in this fashion too.


Changelog

TODO

Migration Guide

TODO

@james7132 james7132 added C-Performance A change motivated by improving speed, memory usage or compile times A-Tasks Tools for parallel and async work M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Apr 16, 2024
@james7132 james7132 requested a review from hymm April 16, 2024 05:32
@james7132 james7132 added the S-Blocked This cannot move forward until something else changes label Apr 16, 2024
@james7132
Copy link
Member Author

A quick test against many_foxes shows a notable improvement to the multithreaded executor, shaving up to 2/3s of the time spent in it.

image

@james7132
Copy link
Member Author

This should be ready to go other than the blocking releases from async_executor and concurrent_queue. Took another look at the cumulative time savings in the multithreaded executor and it shows the savings much more visibly than the prior histogram:

image

crates/bevy_tasks/src/static_task_pool.rs Outdated Show resolved Hide resolved
crates/bevy_tasks/src/static_task_pool.rs Outdated Show resolved Hide resolved
crates/bevy_tasks/src/static_task_pool.rs Outdated Show resolved Hide resolved
@hymm
Copy link
Contributor

hymm commented Apr 23, 2024

Do you see much change in the actual frame time? The duplication of all the unsafety of Scope is really not great. If the improvement isn't measurable for the actual frame time, it's probably be better to wait until we can do it without the duplication.

@james7132
Copy link
Member Author

james7132 commented Apr 23, 2024

With this PR, we could probably just remove and/or replace TaskPool as we do not actually rely on non-static usages of the types anymore, but that may break some other use cases in the ecosystem. I remember seeing some frame time improvements with many_foxes, albeit not the greatest gains due to already heavily optimizing our task usage. The main gains were from less overhead in launching a large number of small systems, and systems with internal parallelism like transform propagation or visibility checks. I'll get some concrete numbers soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Tasks Tools for parallel and async work C-Performance A change motivated by improving speed, memory usage or compile times M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Blocked This cannot move forward until something else changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch to using static async executors
3 participants