emscripten-core / emscripten

Emscripten: An LLVM-to-WebAssembly Compiler
Other
25.63k stars 3.28k forks source link

emscripten_request_animation_frame_loop(), emscripten_set_main_loop(), emscripten_set_timeout() etc. are not compatible with JSPI #22493

Open juj opened 1 week ago

juj commented 1 week ago

I'm taking a peek at my WebGPU bindings JSPI support test, and I find that after updating to latest Emscripten, I now start getting this error:

Uncaught RuntimeError: attempting to suspend without a WebAssembly.promising export

image

I wonder if something might have changed in the JSPI implementation since an earlier Emscripten version, that my code is now out of date?

Here is how I handle JSPI:

https://github.com/juj/wasm_webgpu/blob/16bbfed542979a2f15f789cd0461cce7bb30e0e6/lib/lib_webgpu.js#L1427-L1449

i.e. I use a pattern

lib_webgpu.js
  wgpu_buffer_map_sync__sig: 'ii',
  wgpu_buffer_map_sync__async: true,
  wgpu_buffer_map_sync: function(buffer) {
    return Asyncify.handleAsync(() => {
      return wgpu[buffer].mapAsync(); // .mapAsync() returns a promise
  },

This used to work with both Asyncify and JSPI at some point - but maybe the syntax, or something else has changed?

It's been a while since I followed this feature. I tried to search https://emscripten.org/docs/porting/asyncify.html for documentation example of how to use Asyncify.handleAsync(), though there is only a EM_JS() based example, which I don't want to use. So I wonder if that changes anything, or if the code should work identically as a JS lib?

juj commented 1 week ago

A SINGLE_FILE build expressing the failure can be seen in https://oummg.com/dump/buffer_map_sync.html

juj commented 1 week ago

What is odd here is that the error occurs in the next run of the requestAnimationFrame event loop..

juj commented 1 week ago

Ah actually it is not the next run of the requestAnimationFrame event loop, but the current call.

So I need the WebAssembly.promising() call. I find this patch fixes the issue:

diff --git a/src/library_html5.js b/src/library_html5.js
index 4e73df86c..3b1c899d2 100644
--- a/src/library_html5.js
+++ b/src/library_html5.js
@@ -2472,7 +2472,11 @@ var LibraryHTML5 = {

   emscripten_request_animation_frame_loop: (cb, userData) => {
     function tick(timeStamp) {
+#if JSPI
+      if (WebAssembly.promising({{{ makeDynCall('idp', 'cb') }}})(timeStamp, userData)) {
+#else
       if ({{{ makeDynCall('idp', 'cb') }}}(timeStamp, userData)) {
+#endif
         requestAnimationFrame(tick);
       }
     }

What is the overhead of a WebAssembly.promising()? I ponder if we should add the above patch to all our settimeouts/setintervals/requestAnimationFrame/set_main_loop calls(), or even go as far as to add it to all {{{ makeDynCall() }}} calls?

Although I recall there are some nesting problems with mixed stacks of Wasm and JS calll frames, where JS -> Wasm -> JS -> Wasm -> JS calls need some kind of manual unwinding strategy to fully yield back from innermost wasm call to outermost JS scope?

juj commented 1 week ago

For now I removed use of emscripten_set_animation_frame_loop() as it is incompatible with JSPI. In wasm_webgpu bindings I added a custom function wgpu_request_animation_frame() that works in a manner that is JSPI-aware, and holds rAF()s until JSPI suspend is finished.

I wonder if we will want to do something like that for all top level event handler functions.. or not quite sure what would be the best general strategy to handle this - this feels a bit challenging requirement for the user to need to know if their code execution might lead to a JSPI suspend before calling into Wasm? Would we want to build a double API set for JSPI vs non-JSPI functions?

juj commented 1 week ago

I observe that calling WebAssembly.promising() generates temporary garbage, so to avoid calling that function each frame to wrap the same function pointer, I wanted to optimize the wrapping to only occur once, to avoid runaway JS garbage.

In https://github.com/juj/wasm_webgpu/commit/a363e1a39440fcd522d95b3ee9768390d6a7fce1

i.e.

https://github.com/juj/wasm_webgpu/blob/a363e1a39440fcd522d95b3ee9768390d6a7fce1/lib/lib_webgpu.js#L219-L229

I switched to a mechanism that would achieve that, so WebAssembly.promising() is called only once for the whole rAF loop.

However, oddly, after that the animation that it results in only works for some number of seconds, before breaking in that same error:

image

I've uploaded a test build of the above code at https://oummg.com/dump/buffer_map_sync_breaks_after_a_few_seconds.html

On my system it successfully animates for some number of seconds, before pausing in an exception.

Is there a restriction that the function object returned by WebAssembly.promising() may only be called once/be involved only in one suspend/resume event?

juj commented 1 week ago

The more I poke at https://oummg.com/dump/buffer_map_sync_breaks_after_a_few_seconds.html RuntimeError, I get a feeling like that would be a browser bug. I can't see any reason why execution of such a requestAnimationFrame loop should spontaneously stop like that.

brendandahl commented 1 week ago

To respond to the original issue, JSPI has changed behind the scenes, but I'm not aware of anything that would have caused your example to break if it was working before with JSPI.

Is there a restriction that the function object returned by WebAssembly.promising() may only be called once/be involved only in one suspend/resume event?

No, you should be able to call it as many times as you want. FWIW, on my macbook m1 w/ chrome 128 I have no issue with your demo above.

juj commented 1 week ago

I reported the bug in Chromium bug tracker at https://issues.chromium.org/issues/364667545.

The difference between the working and failing runs is as small as

image

juj commented 1 week ago

Revised the title of this bug to focus on the Emscripten side challenge, namely that none of the callback-taking functions work with JSPI, unless one does a custom wrapper like e.g. here:

https://github.com/juj/wasm_webgpu/blob/280bbcfcef90b3ea6a515eeae8414b45aedabdeb/lib/lib_webgpu.js#L203-L220

brendandahl commented 1 week ago

I've run into this a number of other places (e.g. starting threads). With JSPI we basically need to add wrappers around most entry points into wasm. Adding the wrapper everywhere by default won't work since a lot of code is not expecting a promise return value.

It still needs some work, but I'm hoping that we can add an annotation to functions that need the JSPI wrapper. Alternatively, we could make a preprocessor similar to makeDynCall that will auto add the wrapper when built with JSPI.

sbc100 commented 1 week ago

makeSuspendingDynCall?