Skip to content

Wasm threading fixes #1093

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

Merged
merged 2 commits into from
Mar 24, 2025
Merged

Wasm threading fixes #1093

merged 2 commits into from
Mar 24, 2025

Conversation

PgBiel
Copy link
Contributor

@PgBiel PgBiel commented Mar 23, 2025

Currently implemented:

  • Adds custom wasm binary name support proposed in WebAssembly support #438 (comment), sample usage:

    struct MyExtension;
    
    #[gdextension]
    unsafe impl ExtensionLibrary for MyExtension {
     fn override_wasm_binary() -> Option<&'static str> {
          // Binary name unchanged ("mycrate.wasm") without thread support.
          #[cfg(feature = "nothreads")]
          return None;
    
          // Tell gdext we add a custom suffix to the binary with thread support.
          // Please note that this is not needed if "mycrate.threads.wasm" is used.
          // (You could return `None` as well in that particular case.)
          #[cfg(not(feature = "nothreads"))]
          Some("mycrate-with-threads.wasm")
      }
    }
  • Fixes wasm_nothreads compilation by not using compiled out function in async_runtime.rs (regression from Async Signals #1043)

  • Fixes wasm_nothreads runtime errors on init due to thread locals being used for panic handling (regression from Panic handling: thread safety; set hook once and not repeatedly #1037)

TODO

Working on these, but feel free to review or merge what's already there.

  • Potentially fix the rest of async_runtime to e.g. not use std::thread::current on nothreads builds (might need to be in a separate PR Let's do that in a separate PR (edit: using that function is actually fine, let's keep an eye on it in case anything else could be a problem)
  • Automatically detect .threads.wasm when it still uses the crate name to avoid having to use the option introduced above outside atypical cases

@PgBiel PgBiel marked this pull request as ready for review March 23, 2025 08:31
@PgBiel PgBiel force-pushed the fix-wasm-threading branch from 6774535 to 267c573 Compare March 23, 2025 08:33
@GodotRust
Copy link

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

1 similar comment
@GodotRust
Copy link

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

@Bromeon Bromeon added bug c: wasm WebAssembly export target labels Mar 23, 2025
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@PgBiel PgBiel force-pushed the fix-wasm-threading branch 4 times, most recently from e7fdeb7 to 09dc8b8 Compare March 23, 2025 17:18
@PgBiel
Copy link
Contributor Author

PgBiel commented Mar 23, 2025

News:

  • I've implemented a built-in check for .threads.wasm so the book instructions can be more easily followed. The wasm_binary option is now only used for non-standard names.
  • I've added the requested docs, not sure if the example is too ideal given this, let me know if it should be changed or if it's ok for now

Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Looks mostly good, thanks!

@Bromeon
Copy link
Member

Bromeon commented Mar 23, 2025

Spontaneous thought, since wasm_binary is "dynamic" in nature (can be any string, not hardcoded identifier), did you consider making it a trait method of ExtensionLibrary rather than a proc-macro argument of #[gdextension]?

That way, it would be clearer to users that they can't just specify literals. Or would that complicate fetching the value?

@PgBiel PgBiel force-pushed the fix-wasm-threading branch from 09dc8b8 to fedbc24 Compare March 23, 2025 23:14
@PgBiel
Copy link
Contributor Author

PgBiel commented Mar 23, 2025

Spontaneous thought, since wasm_binary is "dynamic" in nature (can be any string, not hardcoded identifier), did you consider making it a trait method of ExtensionLibrary rather than a proc-macro argument of #[gdextension]?

Nope, did not. Just changed it and seems to work nicely. Thanks for the suggestion!

I considered making it a const, but it didn't seem consistent with the rest, so I made it a method.

@PgBiel PgBiel force-pushed the fix-wasm-threading branch from fedbc24 to fd752ba Compare March 23, 2025 23:20
@PgBiel
Copy link
Contributor Author

PgBiel commented Mar 23, 2025

Been testing locally, worth mentioning. :)

Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Thank you!

@Bromeon Bromeon added this pull request to the merge queue Mar 24, 2025
Merged via the queue into godot-rust:master with commit 78ca889 Mar 24, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug c: wasm WebAssembly export target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants