Add support for nothreads wasm builds (Godot 4.3+) #794

PgBiel · 2024-07-10T18:00:40Z

Adds support for linking with Godot 4.3 web export built with threads=no (see godotengine/godot#85939). Requires NOT specifying the -pthread emscripten flag.

I'm not sure if it's possible to build with threads=no outside of the web export template, so for now I've restricted the fix to wasm targets. We could potentially remove that restriction if we find that Godot can be built that way for non-wasm targets as well (for maximum compatibility).

This fix consists of adding a flag, experimental-wasm-nothreads (bikesheddable) which removes calls to std::thread::current(), as this appears to panic without emscripten pthread support (see #438 (comment)).

Right now, I've only applied the fix to single-threaded gdext code, that is, without the experimental-threads feature toggled on. I've tested this with a very simple project and it seems to work, in principle (haven't tested more complex projects).

I have yet to add support for experimental-threads (which is needed since the extension's code might depend on that feature even when exported to a single-threaded environment, due to Sync/Send trait impls). Therefore, this PR is marked as draft until I add it. (EDIT: Decided to postpone this to a future PR, especially because experimental-threads isn't really usable on Wasm at the moment, at least to export to release.)

GodotRust · 2024-07-10T18:08:16Z

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

Ughuuu · 2024-07-10T19:08:02Z

godot-ffi/src/binding/single_threaded.rs

 use std::thread::ThreadId;

 use super::GodotBinding;
 use crate::ManualInitCell;

 pub(super) struct BindingStorage {
+    // No threading when linking against Godot with a nothreads Wasm build.
+    #[cfg(all(target_family = "wasm", feature = "experimental-wasm-nothreads"))]
+    main_thread_id: Cell<Option<()>>,


do we even need the variable in case of no threads? Can't we remove it? And also the if's where this is used?

Also, Option<()> is bool with extra steps 😉

Or is there existing code that just checks for presence without getting the inner value?
If we can omit the field entirely, even better.

Yeah, it was convenient to do this to avoid adding more cfgs where we check for initialization by checking for Some in other methods. We can replace the field with a simple bool one, though that'll lead to more conditional compilation. Maybe it doesn't matter that much though, since it'd be restricted to just this file.

We can't remove the field entirely as we still need to check for initialization, we just don't need the thread ID itself anymore.

Would there be a lot to change?

If yes,Option<()> is probably OK but then a comment should explain why not bool 🙂

I've changed it to bool. It wasn't that much tbh.

Bromeon

Thanks a lot! 💪

Right now, I've only applied the fix to single-threaded gdext code, that is, without the experimental-threads feature toggled on.

You should probably add another compile error if both features are simultaneously specified, mentioning this combination is not yet implemented. You can place it next to the existing ones:

gdext/godot/src/lib.rs

Lines 124 to 132 in 79edae3

    
           #[cfg(all(feature = "lazy-function-tables", feature = "experimental-threads"))] 
        
           compile_error!("Thread safety for lazy function pointers is not yet implemented."); 
        
           #[cfg(all(target_family = "wasm", not(feature = "experimental-wasm")))] 
        
           compile_error!("Must opt-in using `experimental-wasm` Cargo feature; keep in mind that this is work in progress"); 
        
           // See also https://github.com/godotengine/godot/issues/86346. 
        
           #[cfg(all(feature = "double-precision", not(feature = "api-custom")))] 
        
           compile_error!("The feature `double-precision` currently requires `api-custom` due to incompatibilities in the GDExtension API JSON.");

There are now a lot of complex cfgs, like

#[cfg(any(
    not(target_family = "wasm"),
    not(feature = "experimental-wasm-nothreads")
))]

I wonder if we could maybe check these two features in godot-ffi/build.rs and emit a custom cfg (see e.g. here) in their place, which is checked within godot-ffi/src files?

Bromeon · 2024-07-10T19:28:11Z

godot-ffi/src/binding/single_threaded.rs

 use std::thread::ThreadId;

 use super::GodotBinding;
 use crate::ManualInitCell;

 pub(super) struct BindingStorage {
+    // No threading when linking against Godot with a nothreads Wasm build.
+    #[cfg(all(target_family = "wasm", feature = "experimental-wasm-nothreads"))]
+    main_thread_id: Cell<Option<()>>,


Also, Option<()> is bool with extra steps 😉

Or is there existing code that just checks for presence without getting the inner value?
If we can omit the field entirely, even better.

Bromeon · 2024-07-10T19:36:03Z

godot/Cargo.toml

 experimental-wasm = []
+experimental-wasm-nothreads = [
+    "experimental-wasm",
+    "godot-core/experimental-wasm-nothreads",
+]


I was thinking about adding a compile_error! to detect when both experimental-wasm and experimental-wasm-nothreads are specified, but since this is always done here, I don't think it's possible?

We would need to treat the two features separately and complicate some existing code in godot to use #[cfg(any(...))] instead 🤔

I think we can remove the dependency on "experimental-wasm" and instead gate the changes behind all(target_family = "wasm", feature = "experimental-wasm-nothreads"), which should be enough as target_family = "wasm" implies feature = "experimental-wasm" (we have a compiler error set in place if you use wasm without that feature).

PgBiel · 2024-07-10T22:22:07Z

You should probably add another compile error if both features are simultaneously specified, mentioning this combination is not yet implemented.

I think that's very smart actually, and should help make this fix land earlier. We could add a fix for experimental-threads in a later PR, especially considering I don't really have, at the moment, a complex enough project with which to extensively test the interactions between experimental-threads and this flag.

EDIT: To be fair, we can't effectively use experimental-threads in a Wasm project, at least when compiling to release, since compiling Wasm to release currently requires lazy-function-tables (lest compilation consumes tens of gigabytes of memory), which is not yet thread-safe, so this is less important. Of course, you can always ship a debug binary, though that's not very ideal.

I wonder if we could maybe check these two features in godot-ffi/build.rs and emit a custom cfg (see e.g. here) in their place, which is checked within godot-ffi/src files?

I think that could be nice, as long as we ensure it doesn't become less readable rather than the opposite. Something like #[cfg(wasm-nothreads)] could work.

One alternative is to not apply De Morgan's laws here and just write #[cfg(not(all(target_family = "wasm", feature = "experimental-wasm-nothreads")))]. Though sometimes that's a bit less readable at a glance compared to the status quo, at least in terms of quickly visualizing the effective conditions that trigger that cfg.

EDIT: To be honest, I think I'm in favor of keeping the cfgs as they are for now. I feel like the complexity of creating the build-time function would currently outweigh the readability problems regarding the current cfg assertions, since they are restricted to a single, small file. We could reconsider this once we adapt the feature to experimental-threads, as I believe this set of cfg assertions would then be used more frequently.

A better readability boost would be to use cfg_if::cfg_if!, but that does imply an additional dependency (unless we find a way to inline it).

So, I think that should be it for now.

Bromeon

EDIT: To be honest, I think I'm in favor of keeping the cfgs as they are for now. I feel like the complexity of creating the build-time function would currently outweigh the readability problems regarding the current cfg assertions, since they are restricted to a single, small file. We could reconsider this once we adapt the feature to experimental-threads, as I believe this set of cfg assertions would then be used more frequently.

I don't agree here 😉
You currently have 12 (!) occurrences of the #[cfg], with varying flavors of:

#[cfg(all(target_family = "wasm", feature = "experimental-wasm-nothreads"))]

#[cfg(any(
    not(target_family = "wasm"),
    not(feature = "experimental-wasm-nothreads")
))]

#[cfg(all(
    debug_assertions,
    any(
        not(target_family = "wasm"),
        not(feature = "experimental-wasm-nothreads")
    )
))]

#[cfg(any(
    not(target_family = "wasm"),
    not(feature = "experimental-wasm-nothreads")
))]

Emitting a custom cfg is not complex, it's two lines in build.rs (I hope I got syntax right):

println!(r#"cargo:rustc-check-cfg=cfg(wasm_nothreads, values(none()))"#);
println!(r#"cargo:rustc-cfg=wasm_nothreads"#);

And this will simplify all of the above to single-line #[cfg] statements, and it makes the special ones also easier to read:

#[cfg(all(debug_assertions, wasm_nothreads))]

But I added some other ideas on how to simplify code by setting the right abstraction boundaries, and maybe they could be combined with your approach of Option<()>. It really seems that could cut down on many #[cfg]s.

Bromeon · 2024-07-11T08:30:25Z

godot-ffi/src/binding/single_threaded.rs

+        #[cfg(all(target_family = "wasm", feature = "experimental-wasm-nothreads"))]
+        {
+            if !storage.initialized.get() {
+                panic!("deinitialize without prior initialize");
+            }

-        storage.main_thread_id.set(None);
+            storage.initialized.set(false);
+        }
+
+        #[cfg(any(
+            not(target_family = "wasm"),
+            not(feature = "experimental-wasm-nothreads")
+        ))]
+        {
+            storage
+                .main_thread_id
+                .get()
+                .expect("deinitialize without prior initialize");
+
+            storage.main_thread_id.set(None);
+        }


This can be simplified to

assert!(self.is_initialized(), "deinitialize without prior initialize"); self.set_initialized(false);

for both cases. set_initialized() would be a function that either sets the cell to false or to None.

If you use Option<()>, the implementation will not even need to differentiate cases.

Bromeon · 2024-07-11T08:32:02Z

godot-ffi/src/binding/single_threaded.rs

+        // 'std::thread::current()' fails when linking to a Godot build without threads. When this feature is enabled,
+        // we assume it is impossible to have multi-threading, so checking if we are in the main thread is not needed.
+        // Therefore, we assign a bogus value for the thread ID instead, so we can still check for prior initialization
+        // through this field later.
+        #[cfg(all(target_family = "wasm", feature = "experimental-wasm-nothreads"))]
+        {
+            storage.initialized.set(true);
+        }
+
+        #[cfg(any(
+            not(target_family = "wasm"),
+            not(feature = "experimental-wasm-nothreads")
+        ))]
+        {
+            storage
+                .main_thread_id
+                .set(Some(std::thread::current().id()));
+        }


This could also become

self.set_initialized(true)

which then has a diverging implementation based on the #[cfg], just like is_initialized().

PgBiel · 2024-07-11T21:29:55Z

Okay, I've applied your suggestions. Let me know what you think!

Bromeon

Looks much nicer, thanks a lot! I also appreciate the detailed comments and docs about safety 👍

Bromeon · 2024-07-12T09:18:56Z

godot-bindings/src/lib.rs

+    if std::env::var("CARGO_CFG_TARGET_FAMILY")
+        .is_ok_and(|families| families.split(',').any(|family| family == "wasm"))


I assume you cannot check directly against wasm here?

Yeah, for Emscripten it comes as "unix,wasm".

I see, maybe add that as a comment.

Also, isn't that env var always set? If yes, you should .expect() it rather than .is_ok_and().

Bromeon · 2024-07-12T09:22:58Z

godot-ffi/src/binding/single_threaded.rs

+    /// # Panics
+    /// If attempting to deinitialize before initializing, or vice-versa.


The 2nd part is not checked though.

I think the precondition can be verified outside the #[cfg], e.g. like this:

if initialized == self.is_initialized() { if initialized { panic!("already initialized"); } else { panic!("deinitialize without prior initialize"); } }

Interesting, I think this means it wasn't checked before either. So we'll get a free bug fix :)

Done! Seems to be working fine.

Thanks! Yes, it's always nice to have these drive-by bugfixes 🙂

Bromeon · 2024-07-12T09:25:18Z

godot-ffi/src/binding/single_threaded.rs

+        #[cfg(not(wasm_nothreads))]
+        {
+            if initialized {
+                self.main_thread_id.set(Some(std::thread::current().id()));
+            } else {
+                self.main_thread_id
+                    .get()
+                    .expect("deinitialize without prior initialize");
+
+                self.main_thread_id.set(None);
+            }
+        }
+    }


With the above changes, this can then be:

Suggested change

#[cfg(not(wasm_nothreads))]

{

if initialized {

self.main_thread_id.set(Some(std::thread::current().id()));

} else {

self.main_thread_id

.get()

.expect("deinitialize without prior initialize");

self.main_thread_id.set(None);

}

}

}

#[cfg(not(wasm_nothreads))]

{

let thread_id = initialized.then(|| std::thread::current().id());

self.main_thread_id.set(thread_id);

}

}

Same for the other block.

Bromeon · 2024-07-14T07:35:05Z

Thank you! 🙂

Ughuuu reviewed Jul 10, 2024

View reviewed changes

Bromeon added feature Adds functionality to the library c: ffi Low-level components and interaction with GDExtension API labels Jul 10, 2024

Bromeon reviewed Jul 10, 2024

View reviewed changes

Bromeon added c: wasm WebAssembly export target and removed c: ffi Low-level components and interaction with GDExtension API labels Jul 10, 2024

PgBiel force-pushed the wasm-nothreads branch 3 times, most recently from 7742933 to 26db942 Compare July 11, 2024 02:23

PgBiel marked this pull request as ready for review July 11, 2024 03:47

Bromeon reviewed Jul 11, 2024

View reviewed changes

PgBiel force-pushed the wasm-nothreads branch 5 times, most recently from 15e2ba4 to 66d2c61 Compare July 11, 2024 21:29

Bromeon reviewed Jul 12, 2024

View reviewed changes

add support for nothreads build to single threaded init

1481ed1

PgBiel force-pushed the wasm-nothreads branch from 66d2c61 to 1481ed1 Compare July 13, 2024 23:45

Bromeon added this pull request to the merge queue Jul 14, 2024

Merged via the queue into godot-rust:master with commit d35869f Jul 14, 2024
15 checks passed

Bromeon mentioned this pull request Jul 14, 2024

Disable main_thread_id assertion for Android debug build #780

Merged

PgBiel deleted the wasm-nothreads branch July 16, 2024 03:02

Ughuuu mentioned this pull request Aug 16, 2024

Add support for nothreads build appsinacup/godot-rapier-physics#201

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for nothreads wasm builds (Godot 4.3+) #794

Add support for nothreads wasm builds (Godot 4.3+) #794

PgBiel commented Jul 10, 2024 •

edited

Loading

GodotRust commented Jul 10, 2024

Ughuuu Jul 10, 2024 •

edited

Loading

Bromeon Jul 10, 2024 •

edited

Loading

PgBiel Jul 10, 2024 •

edited

Loading

Bromeon Jul 10, 2024

PgBiel Jul 11, 2024

Bromeon left a comment

Bromeon Jul 10, 2024 •

edited

Loading

Bromeon Jul 10, 2024

PgBiel Jul 11, 2024

PgBiel commented Jul 10, 2024 •

edited

Loading

Bromeon left a comment

Bromeon Jul 11, 2024

Bromeon Jul 11, 2024

PgBiel commented Jul 11, 2024

Bromeon left a comment

Bromeon Jul 12, 2024

PgBiel Jul 12, 2024

Bromeon Jul 13, 2024

Bromeon Jul 12, 2024

PgBiel Jul 13, 2024

PgBiel Jul 14, 2024

Bromeon Jul 14, 2024

Bromeon Jul 12, 2024

Bromeon commented Jul 14, 2024

	#[cfg(all(feature = "lazy-function-tables", feature = "experimental-threads"))]
	compile_error!("Thread safety for lazy function pointers is not yet implemented.");

	#[cfg(all(target_family = "wasm", not(feature = "experimental-wasm")))]
	compile_error!("Must opt-in using `experimental-wasm` Cargo feature; keep in mind that this is work in progress");

	// See also https://github.com/godotengine/godot/issues/86346.
	#[cfg(all(feature = "double-precision", not(feature = "api-custom")))]
	compile_error!("The feature `double-precision` currently requires `api-custom` due to incompatibilities in the GDExtension API JSON.");

		if std::env::var("CARGO_CFG_TARGET_FAMILY")
		.is_ok_and(\|families\| families.split(',').any(\|family\| family == "wasm"))

		/// # Panics
		/// If attempting to deinitialize before initializing, or vice-versa.

Add support for nothreads wasm builds (Godot 4.3+) #794

Add support for nothreads wasm builds (Godot 4.3+) #794

Conversation

PgBiel commented Jul 10, 2024 • edited Loading

GodotRust commented Jul 10, 2024

Ughuuu Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Bromeon Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

PgBiel Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon left a comment

Choose a reason for hiding this comment

Bromeon Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PgBiel commented Jul 10, 2024 • edited Loading

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PgBiel commented Jul 11, 2024

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon commented Jul 14, 2024

PgBiel commented Jul 10, 2024 •

edited

Loading

Ughuuu Jul 10, 2024 •

edited

Loading

Bromeon Jul 10, 2024 •

edited

Loading

PgBiel Jul 10, 2024 •

edited

Loading

Bromeon Jul 10, 2024 •

edited

Loading

PgBiel commented Jul 10, 2024 •

edited

Loading