Create nothreads-compatible thread local #1105

PgBiel · 2025-03-29T07:59:48Z

This implements a custom thread_local impl which just defaults to a global variable on wasm_nothreads, which otherwise wouldn't support thread locals due to an apparent lack of support in Rust's std¹, allowing for an alternative fix to the thread local stuff from #1093. This is inherently unsafe so we trust the user to only use experimental-wasm-nothreads if they have indeed disabled threading. (I wonder if there's some way we could detect bad usage at runtime.)

I've tested and this makes the panic context tracker work. Next up, I'd like to test async signals before undrafting, though comments are welcome as the implementation won't change.

Questions

Should we expose godot_thread_local! to all gdext users? I think it'd be a nice utility to have once we add more polyfills to it.. wonder if there are any downsides to that.

I guess it just doesn't have a way to check if threads are available or not, unlike our flag... ↩

GodotRust · 2025-03-29T08:10:37Z

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1105

PgBiel · 2025-03-29T22:24:30Z

Okay, this should be ready for initial review. Some notes:

The LocalKey api is fairly small, so I just implemented everything from it, regardless of usage. Though I wonder if it should go to a separate file.
I'm thinking that we could consider waiting a bit before making it public. But if you think it's fine as is, we could touch up the docs for that.

Both async signals and panic context tracking are working on both threaded and non-threaded builds in my tests now.

Bromeon

Thank you so much! 🙂

I'm probably not a huge fan of the standard library's thread_local! API, but it's probably the best we can do and at least we build on known patterns. The idea to unify thread-local and wasm-nothread code is definitely great! 👍

All in all looks good, added some comments about potential deduplication.

If the per-method #[cfg] turns out to be hard to extract, it may also be possible to work with macros. I wouldn't use traits here, because that just means we need to duplicate 3 times (trait, impl 1, impl 2), i.e. even worse.

Did you consider

#[cfg(not(wasm_nothreads))]
pub type GodotThreadLocal<T> = &'static ThreadLocalKey;

(maybe without the reference) or doesn't that map nicely with the ref/lifetime?

Bromeon · 2025-03-29T22:35:26Z

godot-core/src/private.rs

+#[cfg(not(wasm_nothreads))]
+macro_rules! godot_thread_local {
+    // empty (base case for the recursion)
+    () => {};
+
+    ($(#[$attr:meta])* $vis:vis static $name:ident: $ty:ty = const $init:block; $($rest:tt)*) => {
+        $crate::private::godot_thread_local!($(#[$attr])* $vis static $name: $ty = const $init);
+        $crate::private::godot_thread_local!($($rest)*);
+    };
+
+    ($(#[$attr:meta])* $vis:vis static $name:ident: $ty:ty = const $init:block) => {
+        $(#[$attr])*
+        $vis static $name: $crate::private::GodotThreadLocal<$ty> = {
+            ::std::thread_local! {
+                static $name: $ty = const $init
+            }
+
+            $crate::private::GodotThreadLocal::new_threads(&$name)
+        };
+    };
+
+    ($(#[$attr:meta])* $vis:vis static $name:ident: $ty:ty = $init:expr; $($rest:tt)*) => {
+        $crate::private::godot_thread_local!($(#[$attr])* $vis static $name: $ty = $init);
+        $crate::private::godot_thread_local!($($rest)*);
+    };
+
+    ($(#[$attr:meta])* $vis:vis static $name:ident: $ty:ty = $init:expr) => {
+        $(#[$attr])*
+        $vis static $name: $crate::private::GodotThreadLocal<$ty> = {
+            ::std::thread_local! {
+                static $name: $ty = $init
+            }
+
+            $crate::private::GodotThreadLocal::new_threads(&$name)
+        };
+    };
+}
+
+#[cfg(wasm_nothreads)]
+macro_rules! godot_thread_local {
+    // empty (base case for the recursion)
+    () => {};
+
+    ($(#[$attr:meta])* $vis:vis static $name:ident: $ty:ty = const $init:block; $($rest:tt)*) => {
+        $crate::private::godot_thread_local!($(#[$attr])* $vis static $name: $ty = const $init);
+        $crate::private::godot_thread_local!($($rest)*);
+    };
+
+    ($(#[$attr:meta])* $vis:vis static $name:ident: $ty:ty = const $init:block) => {
+        $(#[$attr])*
+        $vis static $name: $crate::private::GodotThreadLocal<$ty> =
+            $crate::private::GodotThreadLocal::new_nothreads(|| $init);
+    };
+
+    ($(#[$attr:meta])* $vis:vis static $name:ident: $ty:ty = $init:expr; $($rest:tt)*) => {
+        $crate::private::godot_thread_local!($(#[$attr])* $vis static $name: $ty = $init);
+        $crate::private::godot_thread_local!($($rest)*);
+    };
+
+    ($(#[$attr:meta])* $vis:vis static $name:ident: $ty:ty = $init:expr) => {
+        $(#[$attr])*
+        $vis static $name: $crate::private::GodotThreadLocal<$ty> =
+            $crate::private::GodotThreadLocal::new_nothreads(|| $init);
+    };
+}


Some questions here:

Could you please consistently use the macro! { ... } syntax instead of macro!(...) for any "declaration-like" arguments? Maybe with ... on a new line if it helps readability.

Is the recursive approach really easier to understand than a big $( ... )* surrounding the pattern? Or are there technical reasons for it?

In the #[cfg(wasm_nothreads)] macro, case (3) and (5) have identical implementation, could you use $(const)? here?

These two cases

($(#[$attr:meta])* $vis:vis static $name:ident: $ty:ty = const $init:block; $($rest:tt)*) ($(#[$attr:meta])* $vis:vis static $name:ident: $ty:ty = const $init:block)

cannot be combined somehow, with $(;)? or so? Or just requiring the semicolon?

I'll note that I took the macro code structure from stdlib for consistency, which felt like the easiest route for correctness at first. One goal here is that the macro should be drop-in compatible with the original one. But we can certainly deliver some minor improvements.

Could you please consistently use the macro! { ... } syntax instead of macro!(...) for any "declaration-like" arguments? Maybe with ... on a new line if it helps readability.

Sure, though I wonder if rustfmt would force it to be broken into separate lines... hopefully not though

Is the recursive approach really easier to understand than a big $( ... )* surrounding the pattern? Or are there technical reasons for it?

Seems like a non-recursive approach would force all thread locals in the same block to be either const or not, instead of being able to mix them up.

In the #[cfg(wasm_nothreads)] macro, case (3) and (5) have identical implementation, could you use $(const)? here?

Technically they are not identical since the const case requires a block for consistency with the original macro. We could decide on being more permissive in that case though. which would allow using the $(...)* trick for nothreads only.

These two cases
($(#[$attr:meta])* $vis:vis static $name:ident: $ty:ty = const $init:block; $($rest:tt)*) ($(#[$attr:meta])* $vis:vis static $name:ident: $ty:ty = const $init:block)
cannot be combined somehow, with $(;)? or so? Or just requiring the semicolon?

We would have to require a semicolon, yeah.

Fair points, then let's only do the improvements that have no downsides.

Bromeon · 2025-03-29T22:40:00Z

godot-core/src/private.rs

+        #[cfg(not(wasm_nothreads))]
+        return self.threaded_val.get();
+
+        #[cfg(wasm_nothreads)]
+        self.with(Cell::get)


This differentiation occurs a dozen times, is it not possible to abstract?

Something like

self.value().get()

where value() is #[cfg]ed?

Or is the with essential in all methods?

I'm using with even for nothreads to keep the default initialization logic confined to with, for convenience (I used to have a separate method like the one you propose, but it felt like a waste of space). Also, this let me simply copy and paste the important part of the implementation of each method from stdlib, giving me confidence that I'm not messing it up. :p

Sorry I might have misunderstood - if you mean that we could replace all cfg calls with .value(), then no, as LocalKey imposes the usage of .with with a callback to read the value.

Some of the cfgs are not really required (particularly some on the RefCell and Cell helpers), but I felt like keeping them in case some implementation detail on their side changes in a future rust version.

But we can remove them if you prefer, as the current implementation should be identical in these cases.

PgBiel · 2025-03-30T05:34:50Z

After further research, and in particular this StackOverflow answer, turns out the whole premise behind this PR was wrong:

(...) on wasm_nothreads, which otherwise wouldn't support thread locals due to an apparent lack of support in Rust's std (footnote: I guess it just doesn't have a way to check if threads are available or not, unlike our flag...) (...)

They do have a way to check if threads are available or not: through target feature flags.

In the recommended book instructions for single-threaded builds, we remove -pthread from link args (so emscripten doesn't bother to generate proper threading code), but not the atomics target feature flag, so the built std still thinks we have threading, leading to those weird bugs. Simply compiling without those additional feature flags (atomics, bulk-memory and mutable-globals) is enough to make thread locals work for me on nothreads builds (both panic context tracking and async signals).

Therefore, I'll be closing this PR to preserve this implementation if we ever need it again, and I'll open a separate PR partially reverting the fixes in #1093 instead (specifically the changes related to thread locals on panic context tracking - the rest is still needed).

This should be accompanied by a book PR with the correct instructions.

Bromeon added c: wasm WebAssembly export target feature Adds functionality to the library labels Mar 29, 2025

PgBiel force-pushed the gd-thread-local branch 6 times, most recently from 1c47175 to 6162734 Compare March 29, 2025 22:19

PgBiel marked this pull request as ready for review March 29, 2025 22:19

PgBiel force-pushed the gd-thread-local branch from 6162734 to 64b8e5d Compare March 29, 2025 22:21

PgBiel added 2 commits March 29, 2025 19:26

create godot_thread_local

ad8f0bd

use godot_thread_local instead of thread_local

a9e7698

PgBiel force-pushed the gd-thread-local branch from 64b8e5d to a9e7698 Compare March 29, 2025 22:26

Bromeon reviewed Mar 29, 2025

View reviewed changes

PgBiel closed this Mar 30, 2025

This was referenced Mar 30, 2025

Undo Wasm threading fix on panic context tracking #1107

Merged

Adjust target feature instructions for Wasm threading godot-rust/book#88

Merged

WebAssembly support #438

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create nothreads-compatible thread local #1105

Create nothreads-compatible thread local #1105

PgBiel commented Mar 29, 2025 •

edited

Loading

GodotRust commented Mar 29, 2025

PgBiel commented Mar 29, 2025 •

edited

Loading

Bromeon left a comment

Bromeon Mar 29, 2025

PgBiel Mar 29, 2025

Bromeon Mar 29, 2025

Bromeon Mar 29, 2025

PgBiel Mar 29, 2025

PgBiel Mar 29, 2025 •

edited

Loading

PgBiel commented Mar 30, 2025 •

edited

Loading

Create nothreads-compatible thread local #1105

Create nothreads-compatible thread local #1105

Conversation

PgBiel commented Mar 29, 2025 • edited Loading

Questions

Footnotes

GodotRust commented Mar 29, 2025

PgBiel commented Mar 29, 2025 • edited Loading

Bromeon left a comment

Choose a reason for hiding this comment

Bromeon Mar 29, 2025

Choose a reason for hiding this comment

PgBiel Mar 29, 2025

Choose a reason for hiding this comment

Bromeon Mar 29, 2025

Choose a reason for hiding this comment

Bromeon Mar 29, 2025

Choose a reason for hiding this comment

PgBiel Mar 29, 2025

Choose a reason for hiding this comment

PgBiel Mar 29, 2025 • edited Loading

Choose a reason for hiding this comment

PgBiel commented Mar 30, 2025 • edited Loading

PgBiel commented Mar 29, 2025 •

edited

Loading

PgBiel commented Mar 29, 2025 •

edited

Loading

PgBiel Mar 29, 2025 •

edited

Loading

PgBiel commented Mar 30, 2025 •

edited

Loading