Skip to content

Create nothreads-compatible thread local #1105

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
+281 −16
Closed

Create nothreads-compatible thread local #1105

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ad8f0bd
create godot_thread_local
PgBiel Mar 29, 2025
a9e7698
use godot_thread_local instead of thread_local
PgBiel Mar 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion godot-core/src/meta/signature.rs
View file
Original file line number Diff line number Diff line change
@@ -739,7 +739,7 @@ pub mod trace {
TRACE.set(Some(report));
}

thread_local! {
crate::private::godot_thread_local! {
static TRACE: Cell<Option<CallReport>> = Cell::default();
}
}
289 changes: 277 additions & 12 deletions godot-core/src/private.rs
View file
Original file line number Diff line number Diff line change
@@ -17,8 +17,7 @@ pub use sys::out;

#[cfg(feature = "trace")]
pub use crate::meta::trace;
#[cfg(all(debug_assertions, not(wasm_nothreads)))]
use std::cell::RefCell;
use std::cell::{Cell, RefCell};

use crate::global::godot_error;
use crate::meta::error::CallError;
@@ -321,12 +320,12 @@ pub(crate) fn has_error_print_level(level: u8) -> bool {
/// Internal type used to store context information for debug purposes. Debug context is stored on the thread-local
/// ERROR_CONTEXT_STACK, which can later be used to retrieve the current context in the event of a panic. This value
/// probably shouldn't be used directly; use ['get_gdext_panic_context()'](get_gdext_panic_context) instead.
#[cfg(all(debug_assertions, not(wasm_nothreads)))]
#[cfg(debug_assertions)]
struct ScopedFunctionStack {
functions: Vec<*const dyn Fn() -> String>,
}

#[cfg(all(debug_assertions, not(wasm_nothreads)))]
#[cfg(debug_assertions)]
impl ScopedFunctionStack {
/// # Safety
/// Function must be removed (using [`pop_function()`](Self::pop_function)) before lifetime is invalidated.
@@ -351,19 +350,285 @@ impl ScopedFunctionStack {
}
}

#[cfg(all(debug_assertions, not(wasm_nothreads)))]
thread_local! {
/// A thread local which adequately behaves as a global variable when compiling under `experimental-wasm-nothreads`, as it does
/// not support thread locals. Aims to support similar APIs as [`std::thread::LocalKey`].
pub(crate) struct GodotThreadLocal<T: 'static> {
#[cfg(not(wasm_nothreads))]
threaded_val: &'static std::thread::LocalKey<T>,

#[cfg(wasm_nothreads)]
non_threaded_val: std::cell::OnceCell<T>,

#[cfg(wasm_nothreads)]
initializer: fn() -> T,
}

// SAFETY: there can only be one thread with `wasm_nothreads`.
#[cfg(wasm_nothreads)]
unsafe impl<T: 'static> Sync for GodotThreadLocal<T> {}

impl<T: 'static> GodotThreadLocal<T> {
#[cfg(not(wasm_nothreads))]
pub const fn new_threads(key: &'static std::thread::LocalKey<T>) -> Self {
Self { threaded_val: key }
}

#[cfg(wasm_nothreads)]
pub const fn new_nothreads(initializer: fn() -> T) -> Self {
Self {
non_threaded_val: std::cell::OnceCell::new(),
initializer,
}
}

/// Acquires a reference to the value in this TLS key.
///
/// See [`std::thread::LocalKey::with`] for details.
pub fn with<F, R>(&'static self, f: F) -> R
where
F: FnOnce(&T) -> R,
{
#[cfg(not(wasm_nothreads))]
return self.threaded_val.with(f);

#[cfg(wasm_nothreads)]
f(self.non_threaded_val.get_or_init(self.initializer))
}

/// Acquires a reference to the value in this TLS key.
///
/// See [`std::thread::LocalKey::try_with`] for details.
#[allow(dead_code)]
#[inline]
pub fn try_with<F, R>(&'static self, f: F) -> Result<R, std::thread::AccessError>
where
F: FnOnce(&T) -> R,
{
#[cfg(not(wasm_nothreads))]
return self.threaded_val.try_with(f);

#[cfg(wasm_nothreads)]
Ok(self.with(f))
}
}

#[allow(dead_code)]
impl<T: 'static> GodotThreadLocal<Cell<T>> {
/// Sets or initializes the contained value.
///
/// See [`std::thread::LocalKey::set`] for details.
pub fn set(&'static self, value: T) {
#[cfg(not(wasm_nothreads))]
return self.threaded_val.set(value);

// According to `LocalKey` docs, this method must not call the default initializer.
#[cfg(wasm_nothreads)]
if let Some(initialized) = self.non_threaded_val.get() {
initialized.set(value);
} else {
self.non_threaded_val.get_or_init(|| Cell::new(value));
}
}

/// Returns a copy of the contained value.
///
/// See [`std::thread::LocalKey::get`] for details.
pub fn get(&'static self) -> T
where
T: Copy,
{
#[cfg(not(wasm_nothreads))]
return self.threaded_val.get();

#[cfg(wasm_nothreads)]
self.with(Cell::get)
Comment on lines +440 to +444
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@PgBiel PgBiel Mar 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

}

/// Takes the contained value, leaving `Default::default()` in its place.
///
/// See [`std::thread::LocalKey::take`] for details.
pub fn take(&'static self) -> T
where
T: Default,
{
#[cfg(not(wasm_nothreads))]
return self.threaded_val.take();

#[cfg(wasm_nothreads)]
self.with(Cell::take)
}

/// Replaces the contained value, returning the old value.
///
/// See [`std::thread::LocalKey::replace`] for details.
pub fn replace(&'static self, value: T) -> T {
#[cfg(not(wasm_nothreads))]
return self.threaded_val.replace(value);

#[cfg(wasm_nothreads)]
self.with(|cell| cell.replace(value))
}
}

#[allow(dead_code)]
impl<T: 'static> GodotThreadLocal<RefCell<T>> {
/// Acquires a reference to the contained value.
///
/// See [`std::thread::LocalKey::with_borrow`] for details.
pub fn with_borrow<F, R>(&'static self, f: F) -> R
where
F: FnOnce(&T) -> R,
{
#[cfg(not(wasm_nothreads))]
return self.threaded_val.with_borrow(f);

#[cfg(wasm_nothreads)]
self.with(|cell| f(&cell.borrow()))
}

/// Acquires a mutable reference to the contained value.
///
/// See [`std::thread::LocalKey::with_borrow_mut`] for details.
pub fn with_borrow_mut<F, R>(&'static self, f: F) -> R
where
F: FnOnce(&mut T) -> R,
{
#[cfg(not(wasm_nothreads))]
return self.threaded_val.with_borrow_mut(f);

#[cfg(wasm_nothreads)]
self.with(|cell| f(&mut cell.borrow_mut()))
}

/// Sets or initializes the contained value.
///
/// See [`std::thread::LocalKey::set`] for details.
pub fn set(&'static self, value: T) {
#[cfg(not(wasm_nothreads))]
return self.threaded_val.set(value);

// According to `LocalKey` docs, this method must not call the default initializer.
#[cfg(wasm_nothreads)]
if let Some(initialized) = self.non_threaded_val.get() {
*initialized.borrow_mut() = value;
} else {
self.non_threaded_val.get_or_init(|| RefCell::new(value));
}
}

/// Takes the contained value, leaving `Default::default()` in its place.
///
/// See [`std::thread::LocalKey::take`] for details.
pub fn take(&'static self) -> T
where
T: Default,
{
#[cfg(not(wasm_nothreads))]
return self.threaded_val.take();

#[cfg(wasm_nothreads)]
self.with(RefCell::take)
}

/// Replaces the contained value, returning the old value.
///
/// See [`std::thread::LocalKey::replace`] for details.
pub fn replace(&'static self, value: T) -> T {
#[cfg(not(wasm_nothreads))]
return self.threaded_val.replace(value);

#[cfg(wasm_nothreads)]
self.with(|cell| cell.replace(value))
}
}

impl<T: 'static> std::fmt::Debug for GodotThreadLocal<T> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_struct("GodotThreadLocal").finish_non_exhaustive()
}
}

#[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);
};
}
Comment on lines +551 to +615
Copy link
Member

Choose a reason for hiding this comment

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

Some questions here:

  1. 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.

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

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

  4. 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

  1. 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

  1. 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.

  1. 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.

  1. 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.

Copy link
Member

Choose a reason for hiding this comment

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

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


pub(crate) use godot_thread_local;

#[cfg(debug_assertions)]
godot_thread_local! {
static ERROR_CONTEXT_STACK: RefCell<ScopedFunctionStack> = const {
RefCell::new(ScopedFunctionStack { functions: Vec::new() })
}
}

// Value may return `None`, even from panic hook, if called from a non-Godot thread.
pub fn get_gdext_panic_context() -> Option<String> {
#[cfg(all(debug_assertions, not(wasm_nothreads)))]
#[cfg(debug_assertions)]
return ERROR_CONTEXT_STACK.with(|cell| cell.borrow().get_last());

#[cfg(not(all(debug_assertions, not(wasm_nothreads))))]
#[cfg(not(debug_assertions))]
None
}

@@ -378,10 +643,10 @@ where
E: Fn() -> String,
F: FnOnce() -> R + std::panic::UnwindSafe,
{
#[cfg(not(all(debug_assertions, not(wasm_nothreads))))]
let _ = error_context; // Unused in Release or `wasm_nothreads` builds.
#[cfg(not(debug_assertions))]
let _ = error_context; // Unused in Release.

#[cfg(all(debug_assertions, not(wasm_nothreads)))]
#[cfg(debug_assertions)]
ERROR_CONTEXT_STACK.with(|cell| unsafe {
// SAFETY: &error_context is valid for lifetime of function, and is removed from LAST_ERROR_CONTEXT before end of function.
cell.borrow_mut().push_function(&error_context)
@@ -390,7 +655,7 @@ where
let result =
std::panic::catch_unwind(code).map_err(|payload| extract_panic_message(payload.as_ref()));

#[cfg(all(debug_assertions, not(wasm_nothreads)))]
#[cfg(debug_assertions)]
ERROR_CONTEXT_STACK.with(|cell| cell.borrow_mut().pop_function());
result
}
6 changes: 3 additions & 3 deletions godot-core/src/task/async_runtime.rs
View file
Original file line number Diff line number Diff line change
@@ -12,7 +12,7 @@ use std::panic::AssertUnwindSafe;
use std::pin::Pin;
use std::sync::Arc;
use std::task::{Context, Poll, Wake, Waker};
use std::thread::{self, LocalKey, ThreadId};
use std::thread::{self, ThreadId};

use crate::builtin::{Callable, Variant};
use crate::private::handle_panic;
@@ -147,7 +147,7 @@ impl TaskHandle {

const ASYNC_RUNTIME_DEINIT_PANIC_MESSAGE: &str = "The async runtime is being accessed after it has been deinitialized. This should not be possible and is most likely a bug.";

thread_local! {
crate::private::godot_thread_local! {
/// The thread local is only initialized the first time it's used. This means the async runtime won't be allocated until a task is
/// spawned.
static ASYNC_RUNTIME: RefCell<Option<AsyncRuntime>> = RefCell::new(Some(AsyncRuntime::new()));
@@ -346,7 +346,7 @@ trait WithRuntime {
fn with_runtime_mut<R>(&'static self, f: impl FnOnce(&mut AsyncRuntime) -> R) -> R;
}

impl WithRuntime for LocalKey<RefCell<Option<AsyncRuntime>>> {
impl WithRuntime for crate::private::GodotThreadLocal<RefCell<Option<AsyncRuntime>>> {
fn with_runtime<R>(&'static self, f: impl FnOnce(&AsyncRuntime) -> R) -> R {
self.with_borrow(|rt| {
let rt_ref = rt.as_ref().expect(ASYNC_RUNTIME_DEINIT_PANIC_MESSAGE);