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

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

Open
juj opened this issue Sep 3, 2024 · 11 comments
Assignees

Comments

@juj
Copy link
Collaborator

juj commented Sep 3, 2024

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
Copy link
Collaborator Author

juj commented Sep 3, 2024

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

@juj
Copy link
Collaborator Author

juj commented Sep 3, 2024

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

@juj
Copy link
Collaborator Author

juj commented Sep 3, 2024

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 added a commit to juj/wasm_webgpu that referenced this issue Sep 3, 2024
…_sync() sample, and replace it with a new custom function wgpu_request_animation_frame() that is compatible with JSPI. See emscripten-core/emscripten#22493 for details.
@juj
Copy link
Collaborator Author

juj commented Sep 3, 2024

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 added a commit to juj/wasm_webgpu that referenced this issue Sep 3, 2024
…equest_animation_frame() API, and replace it with a wgpu_request_animation_frame_loop() style of call so that the WebAssembly.promising() function only needs to be called once for the whole animation loop, avoiding temporary JS garbage. See emscripten-core/emscripten#22493
@juj
Copy link
Collaborator Author

juj commented Sep 3, 2024

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 juj/wasm_webgpu@a363e1a

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
Copy link
Collaborator Author

juj commented Sep 4, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator Author

juj commented Sep 4, 2024

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 juj changed the title JSPI with Wasm regression? "Uncaught RuntimeError: attempting to suspend without a WebAssembly.promising export" emscripten_request_animation_frame_loop(), emscripten_set_main_loop(), emscripten_set_timeout() etc. are not compatible with JSPI Sep 4, 2024
@juj
Copy link
Collaborator Author

juj commented Sep 4, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

sbc100 commented Sep 4, 2024

makeSuspendingDynCall?

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

No branches or pull requests

3 participants