Skip to content
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

Enabling WASM workers breaks WebGPU writeTexture on Firefox #23149

Open
slowriot opened this issue Dec 13, 2024 · 9 comments
Open

Enabling WASM workers breaks WebGPU writeTexture on Firefox #23149

slowriot opened this issue Dec 13, 2024 · 9 comments
Labels

Comments

@slowriot
Copy link
Contributor

slowriot commented Dec 13, 2024

Tested with version 3.1.73.

I am playing with WASM audio worklets, and in the process of enabling various compile and link flags, noticed an unexpected change in behaviour of WebGPU.

The initial code works under Firefox Nightly and Chromium, live demo here: https://armchair-software.github.io/webgpu-demo2/ (source).

Adding the following compile commands:

  • -mbulk-memory
  • -matomics

And the following link commands:

  • -sWASM_WORKERS
  • -sENVIRONMENT=web,worker (previously -sENVIRONMENT=web)

...causes WebGPU code that was previously working to break on Firefox Nightly only. The code continues to work under Chromium. Firefox produces the following error, triggered by queue.writeTexture:

Uncaught TypeError: GPUQueue.writeTexture: ArrayBufferView branch of (ArrayBufferView or ArrayBuffer) can't be a SharedArrayBuffer or an ArrayBufferView backed by a SharedArrayBuffer
    _wgpuQueueWriteTexture http://localhost:6938/client.js:7422
    iterFunc http://localhost:6938/client.js:5951
    callUserCallback http://localhost:6938/client.js:1177
    runIter http://localhost:6938/client.js:5543
    MainLoop_runner http://localhost:6938/client.js:5467
[client.js:7422:9](http://localhost:6938/client.js)
    _wgpuQueueWriteTexture http://localhost:6938/client.js:7422
    client.wasm.ImGui_ImplWGPU_CreateFontsTexture() http://localhost:6938/client.wasm:2150214
    client.wasm.ImGui_ImplWGPU_CreateDeviceObjects() http://localhost:6938/client.wasm:2147558
    client.wasm.ImGui_ImplWGPU_NewFrame() http://localhost:6938/client.wasm:2154411
    client.wasm.gui::gui_renderer::draw() const http://localhost:6938/client.wasm:369723
    client.wasm.game_manager::loop_main() http://localhost:6938/client.wasm:47210
    client.wasm.game_manager::game_manager()::$_1::operator()() const http://localhost:6938/client.wasm:140802
    client.wasm.decltype(std::declval<game_manager::game_manager()::$_1&>()()) std::__2::__invoke[abi:ne180100]<game_manager::game_manager()::$_1&>(game_manager::game_manager()::$_1&) http://localhost:6938/client.wasm:140680
    client.wasm.void std::__2::__invoke_void_return_wrapper<void, true>::__call[abi:ne180100]<game_manager::game_manager()::$_1&>(game_manager::game_manager()::$_1&) http://localhost:6938/client.wasm:140606
    client.wasm.std::__2::__function::__alloc_func<game_manager::game_manager()::$_1, std::__2::allocator<game_manager::game_manager()::$_1>, void ()>::operator()[abi:ne180100]() http://localhost:6938/client.wasm:136054
    client.wasm.std::__2::__function::__func<game_manager::game_manager()::$_1, std::__2::allocator<game_manager::game_manager()::$_1>, void ()>::operator()() http://localhost:6938/client.wasm:135970
    client.wasm.std::__2::__function::__value_func<void ()>::operator()[abi:ne180100]() const http://localhost:6938/client.wasm:611810
    client.wasm.std::__2::function<void ()>::operator()() const http://localhost:6938/client.wasm:611668
    client.wasm.render::webgpu_renderer::wait_to_configure_loop()::$_0::operator()(void*) const http://localhost:6938/client.wasm:611594
    client.wasm.render::webgpu_renderer::wait_to_configure_loop()::$_0::__invoke(void*) http://localhost:6938/client.wasm:395507
    iterFunc http://localhost:6938/client.js:5951
    callUserCallback http://localhost:6938/client.js:1177
    runIter http://localhost:6938/client.js:5543
    MainLoop_runner

Live demo of the broken variant: https://armchair-software.github.io/emscripten-sound-demo/ (source - this one's built in release mode so has no debugging symbols in the console, unlike the dump above).

I presume that some WebGPU behaviour changes when it detects that workers are available, but I've looked at library_webgpu.js and don't see any obvious branching that could cause this. Note that pthreads are not enabled for this project.

Whatever behaviour switch this is, it would be good to be able to control it explicitly, rather than having it turn on automatically when workers are enabled.

@kripken
Copy link
Member

kripken commented Dec 13, 2024

Sounds like a Firefox-specific bug, which is worth filing there?

Though if we can find a workaround here, that may also be worth looking into.

@slowriot
Copy link
Contributor Author

Well it's here because the fundamental question I have is: why does Emscripten's compiled output change for writeTexture when those worker-related compile commands are added (which aren't documented to have any effect on WebGPU-related functionality)? And can we make any such behaviour switches explicitly controlled by user flags?

My expectation is that adding -sWASM_WORKERS etc shouldn't change WebGPU-related code emitted by Emscripten, unless I enable further flags to make whatever changes those are. If it's intentional, then such behaviour change should at least be documented.

@sbc100
Copy link
Collaborator

sbc100 commented Dec 13, 2024

When you enable wasm workers (or pthreads) the wasm memory becomes backed by a SharedArrayBuffer. I see on place in library_webgpu.js where we currently use .slice instead of .subarray in that case:

#if PTHREADS
// Chrome can't currently handle a SharedArrayBuffer view here, so make a copy.
desc["code"] = HEAPU32.slice(offset, offset + count);
#else
desc["code"] = HEAPU32.subarray(offset, offset + count);
#endif

Its likely the same thing might been to be done in writeTexture here:

var subarray = HEAPU8.subarray(data, data + dataSize);

It also seems like we should be using SHARED_MEMORY and not PTHREADS for these kinds of checks which leads me to believe don't currently have any testing to wasm workers with webgpu.

@sbc100
Copy link
Collaborator

sbc100 commented Dec 13, 2024

@kainino0x

@kainino0x
Copy link
Collaborator

Its likely the same thing might been to be done in writeTexture here:

We can do that, but it would be really ideal to avoid because it's very costly (makes an extra copy of an often-large chunk of memory). The case you pointed to in wgpuDeviceCreateShaderModule is effectively a dead code path because Chromium no longer exposes SPIR-V as an input even behind a flag.

We certainly could add a workaround for Firefox, but given the high cost, I really think it should be fixed in Firefox instead. Otherwise any projects that get compiled with the workaround will keep using it until they recompile after it's removed. I guess we could do:

#if SHARED_MEMORY
  try
#endif
  {
    var subarray = HEAPU8.subarray(data, data + dataSize);
    queue.writeTexture(destination, subarray, dataLayout, writeSize);
  }
#if SHARED_MEMORY
catch (ex) {
    var slice = HEAPU8.subarray(data, data + dataSize);
    queue.writeTexture(destination, slice, dataLayout, writeSize);
  }
#endif

if we really want to.

I looked for a Firefox bug report but I'm surprised not to find one.
I have submitted one: https://bugzilla.mozilla.org/show_bug.cgi?id=1938257
and I would like to write a conformance test (gpuweb/cts#3241) though I may not have time very soon, it's not trivial to add because of the COOP/COEP requirement.

It also seems like we should be using SHARED_MEMORY and not PTHREADS for these kinds of checks

That sounds right.

which leads me to believe don't currently have any testing to wasm workers with webgpu.

I thought we had compile tests... I guess we don't, though I don't think that wouldn't catch this anyway. That said I'm pretty sure we have customers using -sWEBGPU in multithreaded wasm modules; they're just probably only testing in Chromium.

@slowriot
Copy link
Contributor Author

Hang on, though. My main problem, these Firefox issues aside, is that the behaviour of WebGPU code changes when I enable -sWASM_WORKERS. If I'm enabling workers for something specific (in my case, the audio worklets), I don't necessarily want behaviour of WebGPU to be altered in any way just because I added that flag.

The current switch is on PTHREADS:

#if PTHREADS
// Chrome can't currently handle a SharedArrayBuffer view here, so make a copy.
desc["code"] = HEAPU32.slice(offset, offset + count);
#else
desc["code"] = HEAPU32.subarray(offset, offset + count);
#endif

But in my example, pthreads are not enabled (no -pthread flag is being passed). I should ideally be able to control the behaviour of WebGPU explicitly by some separate flag - just because I've enabled WASM_WORKERS for one purpose, doesn't mean I want every other subsystem to spontaneously start using SHARED_MEMORY (or enabling PTHREADS features) as well. Can we make it possible to control these behaviour changes separately from each other?

@sbc100
Copy link
Collaborator

sbc100 commented Dec 20, 2024

These changes in behaviour are because of the underlying memory being a SharedArrayBuffer. Certain API don't work with SharedArrayBuffer so this changes is required, whenever you build with WASM_WORKERS or PTHREADS. Both WASM_WORKER and PTHREADS force the memory to be a SharedArrayBuffer so its not something we would want to extra settings for. We are either using a SAB or we are not.

@slowriot
Copy link
Contributor Author

Oh I see, so enabling WASM_WORKERS necessarily affects the entire heap and hence everything that uses it? In that case what you're saying makes a lot of sense. Thanks for taking the time to explain. Looks like Firefox needs to fix the bug on their end for this to be resolved, then.

@sbc100
Copy link
Collaborator

sbc100 commented Dec 22, 2024

Yes, that is correct, WASM_WORKERS effect the type of the memory itself which effects all users of the memory.

We likely need to change PTHREADS to SHARED_MEMORY in library_webgpu.js and make sure we cover all the places where HEAPU32.subarray is used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants