Replace some `!Send` resources with `thread_local!` #17730

joshua-holmes · 2025-02-07T20:45:48Z

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.

This reverts commit 0319455.

urben1680 · 2025-02-18T14:29:31Z

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

joshua-holmes · 2025-02-19T01:18:59Z

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 · 2025-02-19T07:58:25Z

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

crates/bevy_gilrs/src/gilrs_system.rs

crates/bevy_winit/src/state.rs

crates/bevy_gilrs/src/gilrs_system.rs

…local!`

chescock

Looks good!

crates/bevy_gilrs/src/gilrs_system.rs

alice-i-cecile

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?

hymm

Changes are fairly straightforward. I think passing the EventLoop through a closure makes way more sense than smuggling it through a non send resource. My one worry is that nothing is forcing the gilrs systems to run on the main thread anymore other than the fact that wasm is single threaded. Maybe not too big of a deal though as it'll just panic with the thread local not being initialized and should be easy to fix if we do get wasm multithreading.

aloucks · 2025-03-01T04:55:44Z

gilrs uses mpsc::{Sender, Receiver} under the hood. From what I can tell, the Receiver is what causes Gilrs to be !Sync. I suspect that if the system is bouncing between threads and hitting different Gilrs instances (i.e. with different pairs of sender/receivers), you're going to have broken gamepad input. I haven't actually tested with a gamepad or Gilrs and I've come to this conclusion only by looking through the Gilrs code.

EDIT1: Was there some reason why you used thread_local! instead of Mutex<Gilrs> ?
EDIT2: I now see that it's also !Send in addition to !Sync on wasm32.

I think passing the EventLoop through a closure makes way more sense than smuggling it through a non send resource.

Agreed. I think this will help. However, the WinitWindows struct is still added via init_non_send_resource, which still prevents the App/World from being Send.

Allowing the App/World to be Send opens up some other options I'd like to explore with bevy_winit. I'm wondering if there's some design work opportunities for some kind of "lower level" systems that do not live inside the World and only executes on the "main" thread. These hypothetical type of systems would live along side the runner and the runner may be responsible for triggering them. I'll try to articulate this in more detail later this weekend.

joshua-holmes · 2025-03-03T02:11:16Z

@aloucks

Was there some reason why you used thread_local! instead of Mutex ?

I don't recall ever discussing that, so I'm going to say no. Looking at the gilrs code, I see your concern for using Receiver with thread_local! if we were using multiple threads with thread_local!. However, considering we are only using thread_local! with wasm here, I can't see a way that it would break anything.

However, the WinitWindows struct is still added via init_non_send_resource

You are correct. This PR does not completely get rid of !Send resources. We will need one more PR for that.

which still prevents the App/World from being Send.

Correct me if I'm wrong, but I believe the world is Send with the exception of !Send resources. We force the World to be Send here and just tell the user not to access !Send resources in a different thread, or else the program will panic.

# Objective Remaining work for and closes #17682. First half of work for that issue was completed in [PR 17730](#17730). However, the rest of the work ended up getting blocked because we needed a way of forcing systems to run on the main thread without the use of `!Send` resources. That was unblocked by [PR 18301](#18301). This work should finish unblocking the resources-as-components effort. # Testing Ran several examples using my Linux machine, just to make sure things are working as expected and no surprises pop up. --------- Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com> Co-authored-by: François Mockers <francois.mockers@vleue.com>

# Objective Remaining work for and closes bevyengine#17682. First half of work for that issue was completed in [PR 17730](bevyengine#17730). However, the rest of the work ended up getting blocked because we needed a way of forcing systems to run on the main thread without the use of `!Send` resources. That was unblocked by [PR 18301](bevyengine#18301). This work should finish unblocking the resources-as-components effort. # Testing Ran several examples using my Linux machine, just to make sure things are working as expected and no surprises pop up. --------- Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com> Co-authored-by: François Mockers <francois.mockers@vleue.com>

joshua-holmes added 6 commits February 5, 2025 08:19

Play around with using thread_local!

0319455

Revert "Play around with using thread_local!"

3422c49

This reverts commit 0319455.

Begin moving gilrs from NonSend resource to thread_local!

22c00cc

Use RefCell instead of OnceLock

75326fa

Use thread_local! in rumble.rs

fb464a7

Try not using 'static

38ffe2c

joshua-holmes force-pushed the thread-local branch from b0e821c to 38ffe2c Compare February 7, 2025 23:15

Revert all

ca9829c

joshua-holmes force-pushed the thread-local branch from 1a9c211 to 789a9bf Compare February 8, 2025 18:34

Use thread_local! to replace !Send gilrs resource

df85779

joshua-holmes force-pushed the thread-local branch from 789a9bf to df85779 Compare February 10, 2025 23:19

joshua-holmes added 11 commits February 10, 2025 17:11

Replace NonSend resources in Winit

2ba4687

Add missing panic messages

4ec1dab

Revert GILRS work

662aec4

Replace NonSend resources with thread_local! in bevy_gilrs

c8e428b

Format

f35bfda

Remove std imports

6991515

Add docs for EVENT_LOOP

be4bc13

Properly gate non-wasm imports behind target flag

8627eb3

Fix differing mutability

2bd208c

Add doc to GILRS thread_local!

2ad552b

Clean up comments and docs

6536e85

joshua-holmes marked this pull request as ready for review February 16, 2025 03:06

chescock reviewed Feb 21, 2025

View reviewed changes

crates/bevy_gilrs/src/gilrs_system.rs Outdated Show resolved Hide resolved

crates/bevy_winit/src/state.rs Outdated Show resolved Hide resolved

crates/bevy_gilrs/src/gilrs_system.rs Outdated Show resolved Hide resolved

joshua-holmes added 3 commits February 22, 2025 08:26

Pass event loop directly into runner method instead of using `thread_…

266c204

…local!`

Move platform-dependent logic to new Gilrs::with() method

3105668

Update GILRS comment to include more detail

fdf099d

joshua-holmes changed the title ~~Replace !Send resources with thread_local!~~ Replace some !Send resources with thread_local! Feb 22, 2025

chescock approved these changes Feb 23, 2025

View reviewed changes

crates/bevy_gilrs/src/gilrs_system.rs Outdated Show resolved Hide resolved

Replace g with gilrs for cleaner git diff

a89d19e

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

joshua-holmes force-pushed the thread-local branch from 751410f to a114044 Compare February 25, 2025 16:24

Merge remote-tracking branch 'upstream/main' into thread-local

c62693c

joshua-holmes force-pushed the thread-local branch from a114044 to c62693c Compare February 25, 2025 16:24

alice-i-cecile reviewed Feb 25, 2025

View reviewed changes

hymm approved these changes Feb 26, 2025

View reviewed changes

alice-i-cecile added this pull request to the merge queue Mar 4, 2025

Merged via the queue into bevyengine:main with commit df6e136 Mar 4, 2025
30 checks passed

joshua-holmes mentioned this pull request Mar 6, 2025

🐢 Create inter-thread executor API #18172

Open

joshua-holmes mentioned this pull request Mar 18, 2025

Remove remaining internal use of !Send resources #18386

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Replace some `!Send` resources with `thread_local!` #17730

Replace some `!Send` resources with `thread_local!` #17730

Uh oh!

joshua-holmes commented Feb 7, 2025 •

edited

Loading

Uh oh!

urben1680 commented Feb 18, 2025 •

edited

Loading

Uh oh!

joshua-holmes commented Feb 19, 2025 •

edited

Loading

Uh oh!

joshua-holmes commented Feb 19, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chescock left a comment

Uh oh!

Uh oh!

alice-i-cecile left a comment

Uh oh!

hymm left a comment

Uh oh!

aloucks commented Mar 1, 2025 •

edited

Loading

Uh oh!

joshua-holmes commented Mar 3, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Replace some !Send resources with thread_local! #17730

Replace some !Send resources with thread_local! #17730

Uh oh!

Conversation

joshua-holmes commented Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Uh oh!

urben1680 commented Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshua-holmes commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshua-holmes commented Feb 19, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chescock left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

hymm left a comment

Choose a reason for hiding this comment

Uh oh!

aloucks commented Mar 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshua-holmes commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Replace some `!Send` resources with `thread_local!` #17730

Replace some `!Send` resources with `thread_local!` #17730

joshua-holmes commented Feb 7, 2025 •

edited

Loading

urben1680 commented Feb 18, 2025 •

edited

Loading

joshua-holmes commented Feb 19, 2025 •

edited

Loading

aloucks commented Mar 1, 2025 •

edited

Loading

joshua-holmes commented Mar 3, 2025 •

edited

Loading