WIP: await promise code from sync c #23043

hedwigz · 2024-12-01T21:41:01Z

Introduced a new macro MAIN_THREAD_EM_ASM_PROMISE_AWAIT which is used to write javascript code that returns a promise and block the C code from progressing until the promise is resolved (or errors).

int main()
{
  printf("1st print\n");
  // This call will block until the internal promise resolves (at least 1 second in this case) 
  MAIN_THREAD_EM_ASM_PROMISE_AWAIT({
    out('2nd print');
    // do some async operation that involves the main thread
    return new Promise((resolve,reject) => {
      setTimeout(() => {
        out('3rd print');
        resolve();
      }, 1000);
    });
  });
  printf("4th print\n");
  return 0;
}

The motivation for this was discussed before
We have multiple web workers, each of them calls javascript code which is async and has to be ran on the main thread and need to wait for the promise to be resolved.
I am open for suggestions on how to add this more elegantly

TODO:

add test for promise rejection
handle call from main thread
support returning value (or acknowledging known-issue)
docs

sbc100 · 2024-12-03T00:41:32Z

src/library.js


  emscripten_asm_const_double_sync_on_main_thread: 'emscripten_asm_const_int_sync_on_main_thread',
  emscripten_asm_const_async_on_main_thread__deps: ['$runMainThreadEmAsm'],
-  emscripten_asm_const_async_on_main_thread: (emAsmAddr, sigPtr, argbuf) => runMainThreadEmAsm(emAsmAddr, sigPtr, argbuf, 0),
+  emscripten_asm_const_async_on_main_thread: (emAsmAddr, sigPtr, argbuf) => runMainThreadEmAsm(emAsmAddr, sigPtr, argbuf, 0, 0),


I think you can just leave off the final argument here and elsewhere that you want promise to be false. This will result in promise being undefined at runtime.

sbc100 · 2024-12-03T00:44:37Z

test/test_core.py

@@ -1896,6 +1896,13 @@ def test_main_thread_em_asm(self, args):
  def test_main_thread_async_em_asm(self, args, force_c=False):
    self.do_core_test('test_main_thread_async_em_asm.cpp', emcc_args=args, force_c=force_c)

+  @needs_dylink
+  @parameterized({
+    'pthreads': (['-pthread', '-sPROXY_TO_PTHREAD', '-sEXIT_RUNTIME'], False),


Normally with a parameterized test I would expect at least two different variants (normally we have a default/empty one).

I added the empty one, and the test expects for an error. I did not handle the case of calling this macro from the main thread. Do you think we should add that?

test/core/test_main_thread_async_em_asm_promise_await.cpp

sbc100 · 2024-12-03T00:47:39Z

src/library_pthread.js

+#endif
+      __emscripten_proxy_promise_finish(res, promiseCtx);
+      }).catch(err => {
+        __emscripten_proxy_promise_finish(err, promiseCtx);


I don't see where this is defined

It's defined here
I was not sure how to add an internal C function that is available to the runtime so I added it to REQUIRED_EXPORTS here
Is there a better way to do this?

hedwigz · 2024-12-06T15:45:50Z

@sbc100 Thanks for taking the time to review this. I addressed all of your comments and did the following:

Added an error if the macro is called from main thread (and added test)
Added support for receiving a return integer from the macro
Added documentation
LMK what you think :)

hedwigz · 2024-12-13T05:56:11Z

@sbc100 ping 🤞

sbc100 · 2024-12-13T18:41:16Z

src/library.js

@@ -1655,6 +1655,16 @@ addToLibrary({
  emscripten_asm_const_int_sync_on_main_thread__deps: ['$runMainThreadEmAsm'],
  emscripten_asm_const_int_sync_on_main_thread: (emAsmAddr, sigPtr, argbuf) => runMainThreadEmAsm(emAsmAddr, sigPtr, argbuf, 1),

+  emscripten_asm_const_int_await_promise_on_main_thread__deps: ['$runMainThreadEmAsm'],
+  emscripten_asm_const_int_await_promise_on_main_thread: (emAsmAddr, sigPtr, argbuf) => {


How about we drop the promise part so this becomes emscripten_asm_const_int_await_on_main_thread and the macro is MAIN_THREAD_EM_ASM_AWAIT?

Yes, I like that better as well

sbc100 · 2024-12-13T18:42:32Z

src/library.js

@@ -1631,7 +1631,7 @@ addToLibrary({
    '$proxyToMainThread'
 #endif
  ],
-  $runMainThreadEmAsm: (emAsmAddr, sigPtr, argbuf, sync) => {
+  $runMainThreadEmAsm: (emAsmAddr, sigPtr, argbuf, sync, promise) => {


Maybe call this argument "await" too.. although that could be confused with the JS keyword. Maybe "asyncAwait"?

sbc100 · 2024-12-13T18:44:36Z

src/library.js

+      return runMainThreadEmAsm(emAsmAddr, sigPtr, argbuf, 1, 1);
+    }
+    #endif
+    throw new Error('call to emscripten_asm_const_int_await_promise_on_main_thread is only supported from pthread (but was called from main thread)');


We probably don't want this long error message in release builds. How about:

#if ASSERTIONS assert(ENVIRONMENT_IS_PTHREAD. "emscripten_asm_const_int_await_promise_on_main_thread is not available on the main thread"); #endif return runMainThreadEmAsm(emAsmAddr, sigPtr, argbuf, 1, 1);

I had to modify the assert condition because it errored: ReferenceError: ENVIRONMENT_IS_PTHREAD is not defined

assert((typeof ENVIRONMENT_IS_PTHREAD !== 'undefined' && ENVIRONMENT_IS_PTHREAD), "emscripten_asm_const_int_await_on_main_thread is not available on the main thread");

Not sure if there's a prettier way

Use #if ASSERTIONS && PTHREADS

sbc100 · 2024-12-13T18:45:35Z

src/library.js

+  emscripten_asm_const_int_await_promise_on_main_thread: (emAsmAddr, sigPtr, argbuf) => {
+    #if PTHREADS
+    if (ENVIRONMENT_IS_PTHREAD) {
+      return runMainThreadEmAsm(emAsmAddr, sigPtr, argbuf, 1, 1);


Can you add comments here for these contants e.g.

return runMainThreadEmAsm(emAsmAddr, sigPtr, argbuf, /*sync=*/1, /*asyncAwait=*/1);

sbc100 · 2024-12-13T18:46:30Z

src/library_pthread.js

+#endif
+      if (!rtn.then) {
+        throw new Error('Return value of proxied function expected to be a Promise but got' + rtn);
+      }


Need for both that assertion and runtime check here. I think you can just remove these 3 lines.

oops. nice catch

sbc100 · 2024-12-13T18:48:42Z

system/lib/pthread/proxying.c

+    f->funcIndex, f->emAsmAddr, f->callingThread, f->numArgs, f->argBuffer, (void*)f->ctx);
+}
+
+void _emscripten_proxy_promise_finish(void* res, em_proxying_ctx* ctx) {


I would expect the context to be the first arugment here.

sbc100 · 2024-12-13T18:51:46Z

system/lib/pthread/proxying.c

+      assert(false && "emscripten_proxy_promise failed");
+      return 0;
+    }
+    return f.result;
  }


Maybe structure this as:

if (sync) { if (promise) { } else { } return f.result; }

Also the fact that we have int sync and int promise here as a separate arguments, but it doesn't make sense for promise to be set without sync seem a little odd.

At the very least we should probably assert that promise is never true when sync is not.

sbc100

@tlively would you mind taking a look, since this contains changes to proxying.c

tlively · 2024-12-14T00:49:19Z

system/lib/pthread/proxying.c

Since the new code in this file does not implement the API surface in proxying.h, it would be best to move it to a different file and leave proxying.c unchanged. It would also be best to implement the new functionality in terms of the public API in proxying.h rather than using internal implementation details. For example, there should be no need to reach inside the em_proxying_ctx object.

Although I see we already have _emscripten_run_on_main_thread_js at the end of proxying.c. Adding more code there would be fine, but it should still use only the public API from proxying.h.

Do you prefer that I introduce a new function similar to _emscripten_run_on_main_thread_js but that uses only public proxying.c API?
The problem with introducing a new function similar to emscripten_run_on_main_thread_js is the "path" to that function call.
It'd a bit tedious. And it'd modify the whole stack leading to this logic.
This is why I chose to introduce the new function param asyncAwait (previously promise) just so I could "ride" the whole functions call path.
But, I don't insist on this at all.
As for using the public API - I agree that we can do that if I indeed fork from _emscripten_run_on_main_thread_js
Let me know what you think

Why do you need to use internal details of the proxying mechanism if you don't create an entirely new version of _emscripten_run_on_main_thread_js? Can't you use emscripten_proxy_sync_with_ctx to synchronously wait for the promise to be resolved on the main thread instead of introducing a new emscripten_proxy_async_await function?

The challenging part was to pass the em_proxying_ctx all the way down to _emscripten_receive_on_main_thread_js, through the task and proxied_js_func_t wrappers, because I have to call _emscripten_proxy_promise_finish from within _emscripten_receive_on_main_thread_js.
This is also why I added em_proxying_ctx * ctx to proxied_js_func_t and moved it upwards.
I just pushed a significant simplification and deleted most of the previous code in proxying.c
WDYT?

Now that I think of it, it is a little weird that proxied_js_func_t has a field ctx that if set, causes _emscripten_receive_on_main_thread_js to expect a promise. Maybe I should name it differently?

I think the ctx and promiseCtx names are fine, but it would be good to add comments explaining that they can be null.

hedwigz · 2024-12-14T18:44:14Z

@sbc100 @tlively 🙏 I really appreciate the thorough review and comments!
I addressed all of your comments and I think the code looks way better now.

tlively

Great, this looks a lot simpler overall, even though I agree that it's a little awkward to get the ctx into the right place. Thanks!

tlively · 2024-12-17T22:22:10Z

system/lib/pthread/proxying.c

@@ -13,6 +13,7 @@
 #include <stdbool.h>
 #include <stdlib.h>
 #include <string.h>
+#include <stdio.h>


Looks like this was left over from debugging.

tlively · 2024-12-17T22:22:24Z

system/lib/pthread/proxying.c

@@ -35,6 +36,17 @@ static em_proxying_queue system_proxying_queue = {
  .capacity = 0,
 };

+typedef struct proxied_js_func_t {


Let's move this down to its original location to make the diff smaller.

system/lib/pthread/proxying.c

tlively · 2024-12-17T23:16:07Z

system/lib/pthread/proxying.c

@@ -642,3 +651,34 @@ double _emscripten_run_on_main_thread_js(int func_index,
  }
  return 0;
 }
+
+double _emscripten_await_on_main_thread_js(int func_index,


I do think there is enough code shared with _emscripten_run_on_main_thread_js that it may make sense to combine them, but I don't feel strongly about it. If you do keep them separate, you should explicitly initialize the .ctx field to NULL in _emscripten_run_on_main_thread_js to make sure it is initialized .

tlively · 2024-12-17T23:18:11Z

system/lib/pthread/proxying.c

+  proxied_js_func_t* func = (proxied_js_func_t*)t->arg;
+  func->result = (double)(intptr_t)res;
+  emscripten_proxy_finish(ctx);
+}


Please make sure there is a newline at the end of the file. It would also be good to run git clang-format to fix any formatting errors.

tlively · 2024-12-17T23:18:22Z

system/lib/pthread/proxying.c

I think the ctx and promiseCtx names are fine, but it would be good to add comments explaining that they can be null.

hedwigz · 2024-12-18T20:22:40Z

Great, this looks a lot simpler overall, even though I agree that it's a little awkward to get the ctx into the right place. Thanks!

I am sorry I completely messed up git while trying to rebase.

hedwigz · 2024-12-18T20:37:28Z

@tlively any tips for passing all the CI tests?

tlively · 2024-12-18T21:47:24Z

proxying.c looks good to me.

It looks like the flake8 bot is failing because of a formatting issue. The other CI bots might be aborting early because of that issue. Merging in main to pick up any unrelated fixes could also help.

sbc100 · 2024-12-18T22:07:21Z

The fact that flake8 is being included at all in CI makes me think that perhaps you are not up-to-date on your branch? The flake8 step was replaced with a step call ruff few days back.

sbc100 · 2024-12-18T22:07:49Z

Can you try rebase onto to merging with the latest changes on main?

sbc100 reviewed Dec 3, 2024

View reviewed changes

hedwigz requested a review from sbc100 December 6, 2024 15:46

sbc100 reviewed Dec 13, 2024

View reviewed changes

tlively reviewed Dec 14, 2024

View reviewed changes

hedwigz requested review from tlively and sbc100 December 14, 2024 18:44

tlively reviewed Dec 17, 2024

View reviewed changes

hedwigz closed this Dec 18, 2024

hedwigz force-pushed the inline-javascript-sync-awaitable-promises branch from 5aef76b to 58889f9 Compare December 18, 2024 20:04

hedwigz reopened this Dec 18, 2024

hedwigz force-pushed the inline-javascript-sync-awaitable-promises branch from 84fcbd2 to ae2357d Compare December 18, 2024 20:32

hedwigz marked this pull request as ready for review December 18, 2024 20:37

hedwigz requested a review from tlively December 18, 2024 20:37

hedwigz force-pushed the inline-javascript-sync-awaitable-promises branch from 464bc2f to 4416f4d Compare December 21, 2024 16:47

hedwigz force-pushed the inline-javascript-sync-awaitable-promises branch from 4416f4d to 6ebb010 Compare December 30, 2024 19:42

hedwigz force-pushed the inline-javascript-sync-awaitable-promises branch 5 times, most recently from b8e7971 to 2f9607c Compare January 6, 2025 21:27

awaitable inlined js

b71710b

hedwigz force-pushed the inline-javascript-sync-awaitable-promises branch from 2f9607c to b71710b Compare January 7, 2025 04:59

WIP: closure ignore

a048fad

WIP: await promise code from sync c #23043

Are you sure you want to change the base?

WIP: await promise code from sync c #23043

Conversation

hedwigz commented Dec 1, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hedwigz commented Dec 6, 2024 • edited Loading

hedwigz commented Dec 13, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbc100 Dec 14, 2024 • edited Loading

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

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

sbc100 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

Choose a reason for hiding this comment

hedwigz Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hedwigz commented Dec 14, 2024

tlively 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hedwigz commented Dec 18, 2024 • edited Loading

hedwigz commented Dec 18, 2024

tlively commented Dec 18, 2024

sbc100 commented Dec 18, 2024

sbc100 commented Dec 18, 2024

hedwigz commented Dec 1, 2024 •

edited

Loading

hedwigz commented Dec 6, 2024 •

edited

Loading

sbc100 Dec 14, 2024 •

edited

Loading

hedwigz Dec 17, 2024 •

edited

Loading

hedwigz commented Dec 18, 2024 •

edited

Loading