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

Add optional single-threaded feature to bevy_ecs/bevy_tasks #6690

Merged
merged 20 commits into from
Jul 9, 2023

Conversation

james7132
Copy link
Member

@james7132 james7132 commented Nov 19, 2022

Objective

Fixes #6689.

Solution

Add single-threaded as an optional non-default feature to bevy_ecs and bevy_tasks that:

  • disable the ParallelExecutor as a default runner
  • disables the multi-threaded TaskPool
  • internally replace QueryParIter::for_each calls with Query::for_each.

Removed the Mutex and Arc usage in the single-threaded task pool.

image

Future Work/TODO

Create type aliases for Mutex, Arc that change to single-threaaded equivalents where possible.


Changelog

Added: Optional default feature multi-theaded to that enables multithreaded parallelism in the engine. Disabling it disables all multithreading in exchange for higher single threaded performance. Does nothing on WASM targets.

@james7132 james7132 added C-Enhancement A new feature A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times A-Tasks Tools for parallel and async work labels Nov 19, 2022
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I think I'm sold on the feature based approach. It makes this relatively maintainable, and very easy to toggle on and off at a high-level.

@james7132
Copy link
Member Author

There is some open space for discussion on whether we want it to be a feature that is turned on or off to go single threaded. tokio uses rt-multi-thread to enable multi threading and I don't think it's on by default, but we could do that instead. Requiring disabling default-features to switch to single threaded. It'd also make the CI job that runs with --all-features default to what we have now.

@alice-i-cecile
Copy link
Member

Conceptually it feels like "multithreading" is a feature we should enable, which should be turned on by default.

We should make sure that both single-threaded and multithreaded paths are being checked in CI (and maybe in miri even): there's a lot of room for subtle mistakes to be introduced.

@recatek
Copy link
Contributor

recatek commented Jan 4, 2023

Could this also allow us to make bevy_tasks an optional dependency for bevy_ecs? The task pools are only used in a few places. Specifically, the par_for_each methods on World (which would be reduced to shims on serial for_each), the parallel executor (which could be #[cfg] disabled entirely) and in benches (which could keep bevy_tasks as a dev-dependency).

@james7132
Copy link
Member Author

We could. However, this would mean that types like ParallelExeuctor straight up don't exist when single threaded is disabled, which can cause compile failures when the feature is enabled. Other parts of the engine will still rely on it since we still need an async executor for async tasks (i.e. asset loading, maybe networking).

@recatek
Copy link
Contributor

recatek commented Jan 5, 2023

Other parts of the engine rely on bevy_tasks or on the parallel executor? If it's the former, could they just pull the parts of bevy_tasks they need out of their own dependency on it?

@Guvante
Copy link
Contributor

Guvante commented Jan 15, 2023

This looks in flight but I would note the current version has some sections having a single-threaded feature and others having a default multi-threaded feature, certainly unifying them would be important before merging.

@james7132 james7132 added this to the 0.11 milestone Feb 27, 2023
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like the right approach to me!

@cart cart enabled auto-merge June 18, 2023 20:25
@james7132 james7132 disabled auto-merge June 18, 2023 20:25
@james7132
Copy link
Member Author

Will re-enable once the checks pass.

@github-actions
Copy link
Contributor

You added a new feature but didn't update the readme. Please run cargo run -p build-templated-pages -- update features to update it, and commit the file change.

1 similar comment
@github-actions
Copy link
Contributor

You added a new feature but didn't update the readme. Please run cargo run -p build-templated-pages -- update features to update it, and commit the file change.

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jun 19, 2023

I want to review this but the diff looks very messed up. It appears to have incorporated all of the PRs I merged today in the merge train :(

Can you fix / remake this?

@github-actions
Copy link
Contributor

github-actions bot commented Jul 4, 2023

You added a new feature but didn't update the readme. Please run cargo run -p build-templated-pages -- update features to update it, and commit the file change.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 5, 2023

You added a new feature but didn't update the readme. Please run cargo run -p build-templated-pages -- update features to update it, and commit the file change.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 5, 2023

You added a new feature but didn't update the readme. Please run cargo run -p build-templated-pages -- update features to update it, and commit the file change.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 5, 2023

You added a new feature but didn't update the readme. Please run cargo run -p build-templated-pages -- update features to update it, and commit the file change.

Cargo.toml Outdated Show resolved Hide resolved
@james7132 james7132 requested a review from mockersf July 6, 2023 17:23
@cart cart added this pull request to the merge queue Jul 7, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 7, 2023
@cart cart enabled auto-merge July 8, 2023 00:22
@cart cart disabled auto-merge July 9, 2023 03:59
@cart cart enabled auto-merge July 9, 2023 04:00
@cart cart added this pull request to the merge queue Jul 9, 2023
Merged via the queue into bevyengine:main with commit d33f5c7 Jul 9, 2023
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events A-Tasks Tools for parallel and async work C-Enhancement A new feature C-Performance A change motivated by improving speed, memory usage or compile times
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Options for disabling all parallelism for single-threaded performance
6 participants