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

opt-out multi-threaded feature flag #9269

Merged
merged 1 commit into from
Aug 3, 2023
Merged

Conversation

flisky
Copy link
Contributor

@flisky flisky commented Jul 25, 2023

Objective

Fixes #9113

Solution

disable multi-threaded default feature

Migration Guide

The multi-threaded feature in bevy_ecs and bevy_tasks is no longer enabled by default. However, this remains a default feature for the umbrella bevy crate. If you depend on bevy_ecs or bevy_tasks directly, you should consider enabling this to allow systems to run in parallel.

@github-actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed C-Bug An unexpected or incorrect behavior labels Jul 25, 2023
@alice-i-cecile
Copy link
Member

Hmm. Why make this a non-default feature rather than making sure users can set a single top-level feature flag?

Enabling multithreading seems like a defensible default behavior for bevy_ecs by itself to me.

@flisky
Copy link
Contributor Author

flisky commented Jul 25, 2023

Sorry, I don't quite get the making sure users can set a single top-level feature flag.

if we enable default multi-threaded for bevy_ecs, other bevy crates depends on bevy_ecs needs bevy_ecs = { ... default-features = false, features = ["bevy_reflect"]}, and (optional) expose multi-threaded by themselves, which I think it's maintain burden. I think most users use the umbrella bevy crate, which enables multi-threaded by default.

@alice-i-cecile alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jul 25, 2023
@github-actions
Copy link
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@alice-i-cecile
Copy link
Member

Okay, I'm content with that justification. @james7132 and @mockersf will likely have opinions too on the best way to do this.

Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

This definitely worsens the experience for people using bevy_ecs by itself, but I don't think we should negatively impact the experience of using it as a dependency for everyone else by requiring them to disable default features every time they use it.

We may need to explicitly document this as it does seem like most consumers of bevy_ecs, by default, want multi threaded execution.

This is also a breaking change and needs a migration guide.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jul 27, 2023
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jul 31, 2023

@flisky can you please add a migration guide section to the PR description and then I'll merge this. Let us know if you'd like a hand with that!

@flisky
Copy link
Contributor Author

flisky commented Aug 1, 2023

@alice-i-cecile thanks for the reminding, and any suggestion is appreciated.

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Aug 3, 2023

The multithreaded feature inbevy_ecs and bevy_tasks is no longer enabled by default. However, this remains a default feature for the umbrella bevy crate. If you depend on bevy_ecs or bevy_tasks directly, you should consider enabling this to allow systems to run in parallel.

Improved wording for you!

@flisky
Copy link
Contributor Author

flisky commented Aug 3, 2023

@alice-i-cecile Thank you!

@james7132 james7132 added this to the 0.12 milestone Aug 3, 2023
@james7132
Copy link
Member

This should still be explicitly documented, either in Cargo.toml for bevy_ecs or in some README, but I don't think we should block this PR on this.

@james7132 james7132 added this pull request to the merge queue Aug 3, 2023
Merged via the queue into bevyengine:main with commit d9702d3 Aug 3, 2023
25 of 26 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 C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New multi-threaded feature is always enabled
3 participants