Get Bevy building for WebAssembly with multithreading #12205

kettle11 · 2024-02-29T17:29:00Z

Objective

This gets Bevy building on Wasm when the atomics flag is enabled. This does not yet multithread Bevy itself, but it allows Bevy users to use a crate like wasm_thread to spawn their own threads and manually parallelize work. This is a first step towards resolving #4078 . Also fixes #9304.

This provides a foothold so that Bevy contributors can begin to think about multithreaded Wasm's constraints and Bevy can work towards changes to get the engine itself multithreaded.

Some flags need to be set on the Rust compiler when compiling for Wasm multithreading. Here's what my build script looks like, with the correct flags set, to test out Bevy examples on web:

set -e
RUSTFLAGS='-C target-feature=+atomics,+bulk-memory,+mutable-globals' \
     cargo build --example breakout --target wasm32-unknown-unknown -Z build-std=std,panic_abort --release
 wasm-bindgen --out-name wasm_example \
   --out-dir examples/wasm/target \
   --target web target/wasm32-unknown-unknown/release/examples/breakout.wasm
 devserver --header Cross-Origin-Opener-Policy='same-origin' --header Cross-Origin-Embedder-Policy='require-corp' --path examples/wasm

A few notes:

cpal crashes immediately when the atomics flag is set. That is patched in Fix crash on Web / Wasm when 'atomics' flag is enabled RustAudio/cpal#837, but not yet in the latest crates.io release.

That can be temporarily worked around by patching Cpal like so:

[patch.crates-io]
cpal = { git = "https://github.com/RustAudio/cpal" }

When testing out wasm_thread you need to enable the es_modules feature.

Solution

The largest obstacle to compiling Bevy with atomics on web is that wgpu types are not Send and Sync. Longer term Bevy will need an approach to handle that, but in the near term Bevy is already configured to be single-threaded on web.

Therefor it is enough to wrap wgpu types in a send_wrapper::SendWrapper that is Send / Sync, but panics if accessed off the wgpu thread.

Changelog

wgpu types that are not Send are wrapped in send_wrapper::SendWrapper on Wasm + 'atomics'
CommandBuffers are not generated in parallel on Wasm + 'atomics'

Questions

Bevy should probably add CI checks to make sure this doesn't regress. Should that go in this PR or a separate PR? Edit: Added checks to build Wasm with atomics

alice-i-cecile · 2024-02-29T17:33:54Z

Bevy should probably add CI checks to make sure this doesn't regress. Should that go in this PR or a separate PR?

Same PR please :) It would be nice to be able to use that check to ensure that this PR is green.

* Move a comment * Run CI on nightly

mockersf · 2024-02-29T21:15:37Z

would you need to disable this feature to check that wgpu works correctly there?

bevy/crates/bevy_render/Cargo.toml

Line 75 in 599e5e4

"fragile-send-sync-non-atomic-wasm",

.github/workflows/ci.yml

kettle11 · 2024-02-29T21:32:11Z

would you need to disable this feature to check that wgpu works correctly there?

bevy/crates/bevy_render/Cargo.toml

Line 75 in 599e5e4

"fragile-send-sync-non-atomic-wasm",

wgpu ignores that feature internally if the atomics flag is set, so it's not strictly necessary to disable it.

Co-authored-by: François <mockersf@gmail.com>

crates/bevy_render/src/renderer/mod.rs

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

james7132 · 2024-03-02T03:54:03Z

crates/bevy_render/src/renderer/mod.rs

+/// On other platforms the wrapper simply contains the wrapped value.
+#[cfg(not(all(target_arch = "wasm32", target_feature = "atomics")))]
+#[derive(Debug, Clone)]
+pub struct WgpuWrapper<T>(T);


It'd be ideal if this doesn't need to be public, as it's sort of an implementation detail. Perhaps we should make a wrapper for Surface since it's the only type used outside of bevy_render that needs this explicitly.

@james7132 I'd also prefer it to be non-public, but the RenderQueue, RenderAdapter, RenderInstance, and RenderAdapterInfo structs all publicly expose their inner wgpu struct which needs to be wrapped in WgpuWrapper.

OK sounds good to me. I believe @robtfm recently found that we may not need these wrappers anymore, but we can tackle that in a separate PR.

james7132

Overall I'm OK with this change. I'm glad we're starting to peak into enabling actual paralellism on the web.

With that said, I'm getting very concerned with how much cfg(target_arch = "wasm32"...) we're seeing across the engine. Much of this is teething pains for wasm support in Rust, which is understandable, particularly as it's the job of the engine to handle platform abstraction. However, problems arise when that platform abstraction itself is leaky, and plugins in the ecosystem and end users end up needing to deal with it, as evidenced by the endless stream of issues coming in. These problems are endemic of a deeper problem that needs to be addressed in the foundational crates for wasm support (i.e. wasm-bindgen), as the problems raised are seemingly viral.

james7132 · 2024-03-07T00:32:14Z

crates/bevy_render/src/renderer/mod.rs

+/// On other platforms the wrapper simply contains the wrapped value.
+#[cfg(not(all(target_arch = "wasm32", target_feature = "atomics")))]
+#[derive(Debug, Clone)]
+pub struct WgpuWrapper<T>(T);


OK sounds good to me. I believe @robtfm recently found that we may not need these wrappers anymore, but we can tackle that in a separate PR.

crates/bevy_render/src/renderer/mod.rs

.github/workflows/ci.yml

Co-authored-by: daxpedda <daxpedda@gmail.com>

mockersf · 2024-03-09T06:50:25Z

.github/workflows/ci.yml

+      - uses: dtolnay/rust-toolchain@master
+        with:
+          toolchain: ${{ env.NIGHTLY_TOOLCHAIN }}
+          target: wasm32-unknown-unknown


Suggested change

target: wasm32-unknown-unknown

targets: wasm32-unknown-unknown,x86_64-unknown-linux-gnu

CI is failing because it also needs this target

That wasn't the issue, CI is still complaining:

"/home/runner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/Cargo.lock" does not exist, unable to build with the standard library, try: rustup component add rust-src --toolchain nightly-x86_64-unknown-linux-gnu

Co-authored-by: François <francois.mockers@vleue.com>

mockersf · 2024-03-09T16:01:20Z

PR looks OK to me, but I'm not sure why the new CI job build-wasm-atomics is failing

kettle11 · 2024-03-09T17:54:43Z

PR looks OK to me, but I'm not sure why the new CI job build-wasm-atomics is failing

@mockersf I got it fixed after a few attempts. I needed to add components: rust-src.

james7132 · 2024-03-14T06:13:12Z

Is there another blocker to removing the fragile-send-sync-non-atomic-wasm feature on wgpu with this change?

kettle11 · 2024-03-16T00:02:24Z

Is there another blocker to removing the fragile-send-sync-non-atomic-wasm feature on wgpu with this change?

There isn't, but fragile-send-sync-non-atomic-wasm doesn't do anything in wgpu when the atomics flag is enabled, so there's no reason to bother enabling or disabling it.

daxpedda

LGTM.

I believe removing fragile-send-sync-non-atomic-wasm would indeed be better. This should remove a bunch of cfgs as types would be more consistent between feature = "atomics" and not(feature = "atomics").

daxpedda · 2024-03-16T08:20:19Z

crates/bevy_render/src/renderer/mod.rs

+/// On web with atomics enabled the inner value can only be accessed on the
+/// `wgpu` thread or else a panic will occur.


It should probably be noted that it will panic if dropped in another thread as well.

@daxpedda Good call out. I updated the comment.

.github/workflows/ci.yml

Co-authored-by: daxpedda <daxpedda@gmail.com>

james7132 · 2024-03-23T10:45:22Z

I think this PR also resolves #9304. Marking up the priority on this given the potential unsoundness implications.

JoJoJet

LGTM

fragile-send-sync-non-atomic-wasm doesn't do anything in wgpu when the atomics flag is enabled, so there's no reason to bother enabling or disabling it.

Can you at least update the comments in the Cargo.toml? Currently it says this which no longer captures the whole picture

# `fragile-send-sync-non-atomic-wasm` feature means we can't use WASM threads for rendering
# It is enabled for now to avoid having to do a significant overhaul of the renderer just for wasm

kettle11 · 2024-03-23T20:13:43Z

Can you at least update the comments in the Cargo.toml? Currently it says this which no longer captures the whole picture

Done!

kettle11 added 2 commits February 29, 2024 00:08

Get Bevy building when on Wasm when 'atomics' flag is enabled

9bfeb2e

Merge branch 'bevyengine:main' into multithreaded_wasm

9e5fc79

alice-i-cecile added C-Performance A change motivated by improving speed, memory usage or compile times O-Web Specific to web (WASM) builds A-Tasks Tools for parallel and async work C-Needs-Release-Note Work that should be called out in the blog due to impact labels Feb 29, 2024

alice-i-cecile requested review from james7132, hymm and mockersf February 29, 2024 17:33

kettle11 added 2 commits February 29, 2024 15:25

Add CI checks to ensure building Wasm with atomics doesn't regress

eb846f7

Fix CI errors.

93a6042

* Move a comment * Run CI on nightly

mockersf reviewed Feb 29, 2024

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

Use NIGHTLY_TOOLCHAIN env variable

59eddb5

Co-authored-by: François <mockersf@gmail.com>

alice-i-cecile reviewed Mar 1, 2024

View reviewed changes

crates/bevy_render/src/renderer/mod.rs Outdated Show resolved Hide resolved

Update crates/bevy_render/src/renderer/mod.rs

106f892

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

james7132 reviewed Mar 2, 2024

View reviewed changes

james7132 mentioned this pull request Mar 5, 2024

Lift Send requirement in wasm builds #9157

Closed

james7132 approved these changes Mar 7, 2024

View reviewed changes

alice-i-cecile requested a review from JoJoJet March 7, 2024 01:08

kettle11 added 2 commits March 6, 2024 22:24

Use derive instead of manual Deref / DerefMut impl

431aa37

Merge remote-tracking branch 'upstream/main' into multithreaded_wasm

1c3d4da

daxpedda reviewed Mar 7, 2024

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

Remove unnecessary +mutable-globals target-feature

9783726

Co-authored-by: daxpedda <daxpedda@gmail.com>

alice-i-cecile added this to the 0.14 milestone Mar 7, 2024

mockersf reviewed Mar 9, 2024

View reviewed changes

kettle11 and others added 2 commits March 9, 2024 10:38

Add x86_64-unknown-linux-gnu target to fix failing CI

e183932

Co-authored-by: François <francois.mockers@vleue.com>

Merge branch 'main' into multithreaded_wasm

7e34b01

kettle11 added 3 commits March 9, 2024 12:11

Attempting to fix build-wasm-atomics CI

a14f55e

Another attempt at CI fix: add back x86_64-unknown-linux-gnu target

65cca19

CI fix to download rust-src

0636540

james7132 requested review from alice-i-cecile, mockersf and daxpedda March 16, 2024 01:53

daxpedda reviewed Mar 16, 2024

View reviewed changes

Try removing x86_64-unknown-linux-gnu from ci.yml

dd7240c

Co-authored-by: daxpedda <daxpedda@gmail.com>

james7132 added P-High This is particularly urgent, and deserves immediate attention C-Unsoundness A bug that results in undefined compiler behavior labels Mar 23, 2024

JoJoJet approved these changes Mar 23, 2024

View reviewed changes

kettle11 added 2 commits March 23, 2024 16:07

Amend comments to add clarifications

e8fab22

Merge remote-tracking branch 'upstream/main' into multithreaded_wasm

b99c936

Formatting tweak: remove trailing space in comment

29fcef3

james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 23, 2024

alice-i-cecile added this pull request to the merge queue Mar 25, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 25, 2024

alice-i-cecile added this pull request to the merge queue Mar 25, 2024

Merged via the queue into bevyengine:main with commit b359740 Mar 25, 2024
31 of 32 checks passed

bugsweeper mentioned this pull request Mar 26, 2024

Error info has been added to LoadState::Failed #12709

Merged

kettle11 mentioned this pull request Apr 1, 2024

WebAssembly multithreading tracking issue #4078

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get Bevy building for WebAssembly with multithreading #12205

Get Bevy building for WebAssembly with multithreading #12205

kettle11 commented Feb 29, 2024 •

edited by alice-i-cecile

alice-i-cecile commented Feb 29, 2024 •

edited

mockersf commented Feb 29, 2024

kettle11 commented Feb 29, 2024

james7132 Mar 2, 2024

kettle11 Mar 5, 2024

james7132 Mar 7, 2024

james7132 left a comment

james7132 Mar 7, 2024

mockersf Mar 9, 2024

mockersf Mar 9, 2024

mockersf commented Mar 9, 2024

kettle11 commented Mar 9, 2024

james7132 commented Mar 14, 2024

kettle11 commented Mar 16, 2024

daxpedda left a comment

daxpedda Mar 16, 2024

kettle11 Mar 23, 2024

james7132 commented Mar 23, 2024

JoJoJet left a comment

kettle11 commented Mar 23, 2024

	target: wasm32-unknown-unknown
	targets: wasm32-unknown-unknown,x86_64-unknown-linux-gnu

		/// On web with atomics enabled the inner value can only be accessed on the
		/// `wgpu` thread or else a panic will occur.

Get Bevy building for WebAssembly with multithreading #12205

Get Bevy building for WebAssembly with multithreading #12205

Conversation

kettle11 commented Feb 29, 2024 • edited by alice-i-cecile

Objective

Solution

Changelog

Questions

alice-i-cecile commented Feb 29, 2024 • edited

mockersf commented Feb 29, 2024

kettle11 commented Feb 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james7132 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

mockersf commented Mar 9, 2024

kettle11 commented Mar 9, 2024

james7132 commented Mar 14, 2024

kettle11 commented Mar 16, 2024

daxpedda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james7132 commented Mar 23, 2024

JoJoJet left a comment

Choose a reason for hiding this comment

kettle11 commented Mar 23, 2024

kettle11 commented Feb 29, 2024 •

edited by alice-i-cecile

alice-i-cecile commented Feb 29, 2024 •

edited