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

Replace some !Send resources with thread_local! #17730

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

joshua-holmes
Copy link

@joshua-holmes joshua-holmes commented Feb 7, 2025

Objective

Work for issue #17682

What's in this PR:

  • Removal of some !Send resources that Bevy uses internally
  • Replaces !Send resources with thread_local! static

What this PR does not cover:

  • The ability to create !Send resources still exists
  • Tests that test !Send resources are present (and should not be removed until the ability to create !Send resources is removed)
  • The example log_layers_ecs still uses a !Send resource. In this example, removing the !Send resource results in the system that uses it running on a thread other than the main thread, which doesn't work with lazily initialized thread_local! static data. Removing this !Send resource will need to be deferred until the System API is extended to support configuring which thread the System runs on. Once an issue for this work is created, it will be mentioned in 🐢 Eliminate !Send resources by supporting Send and !Send World in the same binary #17667

Once the System API is extended to allow control of which thread the System runs on, the rest of the !Send resources can be removed in a different PR.

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events M-Needs-Release-Note Work that should be called out in the blog due to impact S-Needs-Review Needs reviewer attention (from anyone!) to move forward S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 9, 2025
@joshua-holmes joshua-holmes marked this pull request as ready for review February 16, 2025 03:06
@urben1680
Copy link
Contributor

urben1680 commented Feb 18, 2025

Won't this cause multiple worlds to share the same !Send resources?

@joshua-holmes
Copy link
Author

joshua-holmes commented Feb 19, 2025

Yes, in theory multiple worlds could share these resources. However, in practice in this PR, that is not happening.

Keep in mind, I am not changing the API in any way. I am only changing how a few internal variables are stored.

@joshua-holmes
Copy link
Author

@alice-i-cecile, just pinging you here for review on GitHub as well, in case that's your preferred workflow

@joshua-holmes joshua-holmes changed the title Replace !Send resources with thread_local! Replace some !Send resources with thread_local! Feb 22, 2025
Copy link
Contributor

@chescock chescock left a comment

Choose a reason for hiding this comment

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

Looks good!

@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Feb 23, 2025
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.

Code quality is good, and I'm broadly on board with the direction, but I don't personally have the experience with these primitives required to verify correctness. @maniwani or @hymm , can I defer to you?

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-Feature A new feature, making something new possible M-Needs-Release-Note Work that should be called out in the blog due to impact S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants