Multithreading 9/N: MAIN_THREAD_EM_ASM()

[juj]

This adds support for routing EM_ASM() calls to either synchronously or asynchronously proxy to the main thread. This is conceptually symmetric to #5531, which does sync and async calls to JS library functions.

Requires emscripten-core/emscripten-fastcomp#196 to land at the same time.

[kripken]

Could this be done as a pure-C utility function? without compiler changes to fastcomp which would also require wasm backend changes?

[juj]

I don't see how. There is no way to take an address of an EM_ASM function that I could pass on to another thread, and it would come down to somehow creating a lambda function in C side on the spot, and taking its address to carry over to another thread to call. Or do you have a proof of concept on how to implement?

[kripken]

Maybe I'm missing something, but here is what an EM_ASM compiles into:

var ASM_CONSTS = [function() { Module.print('hai') }];
function _emscripten_asm_const_i(code) {
  return ASM_CONSTS[code]();
}
[..]
function _main() {
 [..]
 $call = _emscripten_asm_const_i(0)|0;
 [..]
}

We could have the macro for EM_ASM emit the code to do the sync wait if necessary, as well as add the proxying code in JS, something conceptually like

#define MAIN_THREAD_EM_ASM(code) \
  { \
    EM_ASM( Module.proxyToMainThread(code) ); \ // wrap the user JS code with proxy code
    DO_SYNC_WAIT(); \ // use C pthreads to wait (or, do it in JS?)
  }

We do need to add some code for the JS glue on the other side (main thread), but it seems like we don't need to modify fastcomp/wasm backend code?

[dschuff]

+cc @jgravelle-google

[jgravelle-google]

At a high level this doesn't fundamentally break the structure of how the wasm backend handles EM_ASM, but we will need a patch in s2wasm to support generating the same metadata.

Though it turns out we don't currently share how we parse EM_ASM metadata, so if this landed as-is it technically wouldn't regress existing EM_ASM calls. However I'd much rather not diverge, because therein lies madness.

[jgravelle-google]

That is uh, non-obvious
Instead of interleaving signature and calltype, can we make them separate sub-arrays? So this can be just:

call_types = v[1]
sigs = v[2]

[jgravelle-google]

Also apparently I missed this obvious step back in #5137, and create_asm_consts_wasm isn't calling all_asm_consts, which would make our lives easier.

[juj]

Instead of interleaving signature and calltype, can we make them separate sub-arrays?

Ok, that is also backwards compatible, so probably better.

[jgravelle-google]

This will all need to be repeated in create_asm_consts_wasm. Good opportunity to extract more common parts?

[juj]

Not sure what to extract here. Refactoring the code a little, as requested in your comment below as well.

[jgravelle-google]

These lines are pretty long. Can they be broken up?
There's a lot of repetition as well, where things only differ slightly (e.g., Runtime.warnOnce(message)), some intermediate variables would probably go a long way here.

[jgravelle-google]

What on earth is '(-1 - ' + ','.join(all_args) + '); doing? Call this but with the first argument (the function index iirc) plus one, negated? Why?
My guess is it's a sign to defer the call to the main thread, undo the transform, and call the index there, but if so that is magic, and not in a good way.

[juj]

What on earth is '(-1 - ' + ','.join(all_args) + '); doing?

The proxying mechanism here is the same as the proxying mechanism for regular compiled C function pointers, so the concept here is that positive ordinals 1, 2, 3, ... are regarded as pointers to compiled code functions, and to reuse the same mechanism, negative ordinals -1, -2, -3, ... are used to refer to EM_ASM blocks. So this code is remapping the sequence 0, 1, 2, ... over to -1, -2, -3, .... I think I had a comment somewhere about this, but adding one here as well.

[juj]

We could have the macro for EM_ASM emit the code to do the sync wait if necessary, as well as add the proxying code in JS, something conceptually like

Tried to do this via a macro approach, but I don't see how to do this without doing some changes at least to the backend. The issue is that at #define EM_ASM() time in C code, there is no concept of the ID ordinal of the EM_ASM block, and when the EM_ASM block is executing, it does not get passed an integer that would denote what its own EM_ASM ordinal number is that could be passed out to reference the right block. In the current form, this would require sending the whole EM_ASM block as an eval()able string.

Having the proxying decision happen inside the invocation instead of the ASM_CONSTS block does have a small code size win, since there are fewer signatures than there are functions, so if multiple functions with same signature need to be proxied, there is code size overhead added only in the particular signature invoker.

[juj]

Updated this PR and addressed comments. This is good for a second pass of review?

[jgravelle-google]

Overall seems fine to me. No support included for wasm backend, but we I believe we should be able to implement the current approach in roughly the same way, and I also believe this won't change anything in that pipeline.

[jgravelle-google]

@no_wasm_backend these please

[juj]

check

[saschanaz]

This caused total CI failure because of syntax error. (@@no_wasm_backend is not a valid decorator syntax)

[juj]

Ops, thanks for the quick observation on this!

[saschanaz]

Some tests are still failing, should be investigated more.

[saschanaz]

Closure complains with Variable _emscripten_asm_const_ii declared more than once.

[juj]

Oh, we have the issue that we need a new compiler build tag go live first before this will work. Disabled the relevant tests for now, pending that update - because otherwise we'll get annoying unrelated failures that diminish the value of the PR try builds.

[saschanaz]

Except that many more tests are failing... includes:

• browser.test_binaryen_native
• browser.test_fs_lz4fs_package
• browser.test_html5_mouse
• browser.test_html5_webgl_create_context
• browser.test_sdl2glshader
• browser.test_separate_asm
• asm2/asm2f/asm2i/asm3.test_fs_nodefs_rw
• asm2.test_webidl

[kripken]

If this is a large amount of tests and it can't be fixed quickly, perhaps we should back out this PR?

In the future, we should be careful to look at CI results before merging.

If this is going to be a general problem, of any change to the LLVM build causing this type of situation, then perhaps we need to rethink our CI approach here. Right now it gets emsdk latest, but maybe that's not good enough.

[juj]

Sorry for the breakage - fix in #5838.

[saschanaz]

Right now it gets emsdk latest, but maybe that's not good enough.

1. We may create emsdk install latest-beta which would be updated whenever any breakage occurs on latest.
2. Otherwise we may maintain our own Docker image with emscripten SDK built-in, which would also be updated in the same way.

[kripken]

Maybe we really should maintain our own Docker image for testing. (It might not need to use the emsdk, whichever way is easiest.) I've not really used Docker that way myself, not sure how much effort it would be to keep an image constantly up to date.

[kripken]

I pushed a 1.37.23 version, and now there is a failing test because the emsdk gets 1.37.22. @juj's bots should finish building it and then things will work. But I think this makes it even more important that we have more control over our CI testing. Optimally when we push a new version we'd update a Docker image with it.

[saschanaz]

I think pushing a new version will always cause a test failure as long as we depend on other tools having same version tags. Building Docker image won't help here as it also takes time as much as @juj's bots do.