Initial Wasm Stack Switching support #16779

kripken · 2022-04-21T20:14:46Z

This uses the Promise API for Wasm Stack Switching as an alternative to
Asyncify. That is, instead of modifying the wasm like Asyncify does, we have
the VM pause and resume us. When we want to pause an import returns a
Promise, and when we want to resume we resolve the Promise, basically.

There is more work to do, like getting all the tests to pass, but for now this
gets one test working + adds a code size test that shows the benefit here.

Marked as draft as while this seems to work, I'd appreciate feedback on the
API. I have it for now as -sASYNCIFY=2, that is, another mode of the
ASYNCIFY flag. That's the smallest diff, and we do still use the Asyncify
API (both in C and in JS, just the impl differs), but maybe we want something like
a new flag -sSTACK_SWITCHING, which would be similar to how we have
-sEXCEPTION_HANDLING for wasm exceptions - however, that is an
internal flag (users do -fwasm-exceptions), so it's not a 100% relevant
precedent.

sbc100

Very cool!

I didn't did a full review yet but this looks like a nice minimal first step.

In terms of naming I think we should do with whatever the standard clang flag is for enabling stack switching. Then we can just keep the normal -sASYNCIFY and it will use whatever features it can internally.

BTW, how does this effect fibres. My understanding is that with fibres we can have any number of outstanding/sleeping/suspended stacks and resume them in any order. IIRC that stack switch proposal doesn't allow for multiple outstanding stacks?

sbc100 · 2022-04-21T20:18:40Z

emscripten.py

@@ -840,7 +840,7 @@ def create_module(sending, receiving, invoke_funcs, metadata):
  module = []

  module.append('var asmLibraryArg = %s;\n' % sending)
-  if settings.ASYNCIFY and settings.ASSERTIONS:
+  if settings.ASYNCIFY:


I guess this was just for debugging?

Ah, this is indeed wrong, but it is for more than debugging. I'll fix + add a comment. We now need to instrument imports for either stack switching (to use the Promise API) or for assertions (to add asserts on proper import handling).

kripken · 2022-04-21T20:27:00Z

In terms of naming I think we should do with whatever the standard clang flag is for enabling stack switching.

Ah, I wasn't aware clang had a flag for it. That sounds like a good idea, to match it the way we do for wasm exceptions.

sbc100 · 2022-04-21T20:28:58Z

In terms of naming I think we should do with whatever the standard clang flag is for enabling stack switching.

Ah, I wasn't aware clang had a flag for it. That sounds like a good idea, to match it the way we do for wasm exceptions.

I'm not sure it does.. I was just assuming there would be one.. but maybe that won't happen.

I guess if it doesn't happen when we will need our own flag .. we can bikeshed the name later.

kripken · 2022-04-21T20:30:33Z

BTW, how does this effect fibres. My understanding is that with fibres we can have any number of outstanding/sleeping/suspended stacks and resume them in any order. IIRC that stack switch proposal doesn't allow for multiple outstanding stacks?

I think you are right. We can't support fibers naively with stack switching due to that. However, I wonder if we can perhaps create more instances of the module and each fiber would have one. A little like pthreads has one instance in each Worker, actually... Anyhow, in the long term wasm stack switching will solve this even if the Promise API does not.

fgmccabe · 2022-04-21T21:22:27Z

The fibers question has come up with us before. The current API will likely not allow it; however, there is a V1.1 (my terminology) of the API that would support fibers in an analogous way. This is on our roadmap.

kripken · 2022-05-24T22:36:01Z

After more thought I'm not sure we have a better idea for the option name here. So perhaps it's better not to decide on a permanent long-term name atm, and stick with ASYNCIFY=2 (which can remain the internal option for it later, if we add a more user-friendly name).

If that makes sense to people then I think this PR is ready for review.

fgmccabe · 2022-05-24T23:51:42Z

Ok with me. Movement is better than perfect stillness.

kripken · 2022-05-25T19:49:43Z

Fixed some tests now.

sbc100

lgtm % comments

sbc100 · 2022-05-27T02:02:18Z

emcc.py

@@ -2223,6 +2225,7 @@ def check_memory_setting(setting):
      not settings.ASSERTIONS and \
      not settings.RELOCATABLE and \
      not settings.ASYNCIFY_LAZY_LOAD_CODE and \
+      not settings.ASYNCIFY == 2 and \


sbc100 · 2022-05-27T02:03:28Z

emcc.py

@@ -2512,6 +2515,9 @@ def get_full_import_name(name):

    settings.ASYNCIFY_IMPORTS = [get_full_import_name(i) for i in settings.ASYNCIFY_IMPORTS]

+    if settings.ASYNCIFY == 2 and not settings.EXIT_RUNTIME:
+      diagnostics.warning('experimental', 'ASYNCIFY with stack switching is only tested with EXIT_RUNTIME atm')


How about just always emitting the warning when ASYNCIFY=2 is specified? It is experimental right now for all uses, right?

Sounds good, I'll warn all the time. I'll also add an error here with exit runtime, as it looks like that is just broken atm in d8 (which is the only place we can test so far).

sbc100 · 2022-05-27T02:05:33Z

src/library_async.js

+    // API.
+    suspender: null,
+    // The promise that is being suspended on in the VM atm, or null.
+    promise: null,


What happens if I make a call into the module while the is promise oustanding?

Can we at least assert if I try to do that?

I think that should just work. What won't work is trying to suspend again IIUC.

Actually, if you try to call in to a module via a wrapped export, and there is an outstanding suspension, then the engine will/should trap. However, this will not be the case with the static API; in that case, it will be permitted to reenter a statically wrapped export. In order to support reentrancy, something will have to be done with the shadow stack.

I see, thanks! We should probably move over to the static API here eventually, then, even before fibers, since we do want to support "small" calls back into the wasm during a suspend, e.g. to do a malloc. OTOH, maybe that's not urgent as malloc would not be wrapped, so it should just work already.

sbc100 · 2022-05-27T02:06:32Z

src/library_async.js

+#if ASYNCIFY == 2
+      // TODO we could perhaps add an init function and put this there, but
+      //      this should work for now.
+      Asyncify.suspender = new WebAssembly.Suspender();


You can't do $suspender: "new WebAssembly.Suspender()" as the top level of the library object?

I think that would resolve as a string? Though there might be a way with postSet to do this... but initing the suspender right before we use it to instrument imports seems more "local" anyhow.

sbc100 · 2022-05-27T02:10:22Z

src/library_async.js

@@ -341,9 +424,11 @@ mergeInto(LibraryManager.library, {
    },
  },

+  emscripten_sleep__sig: 'vi', // TODO: others


Do we need that comment?

I think so, we'll need to add sigs for all methods in this file eventually? We depend on that with the new wasm API.

I'm adding a better comment for this.

What I means is, all library functions could benefit from adding as .sig so this comment could equally well apply to all __sig lines in the whole codebase.

In order words, that TODO to add more __sigs is more of a codebase-wide TODO.. I'm not sure it makes sense to mention it in just one specific place.

It is far more urgent here, though: literally it will trap without a sig, when using stack switching.

Thats that same for all functions though. For example, if you try to call addFunction with any JS library function that does't have a __sig it will crash/trap.

(Sorry let me rephrase that: if you try to take the address of a library function within a SIDE_MODULE, and that library function doesn't have a __sig, it will fail).

Yes, I guess to me this feels more urgent since I think users are more likely to run into it... but maybe that's wrong.

I can remove the TODO if you prefer.

OK, maybe elaborate a little if you want to keep it. TODO: add sigs to other functions in the files so that ...

Ok, I edited it now to focus on the overall work to get this entire file to work with stack switching. The sigs are just a part of that.

sbc100 · 2022-05-27T02:10:45Z

src/library_async.js

  emscripten_sleep__deps: ['$safeSetTimeout'],
  emscripten_sleep: function(ms) {
-    Asyncify.handleSleep((wakeUp) => safeSetTimeout(wakeUp, ms));
+    // TODO: add return in the others.


What others?

All other suspending methods. I'm adding a better comment now. Basically, for the new API to work sleep etc. all need to have a sig, and to return a value (the Promise).

sbc100 · 2022-05-27T02:11:58Z

src/runtime_functions.js

@@ -16,6 +16,30 @@ function uleb128Encode(n) {
  return [(n % 128) | 128, n >> 7];
 }

+function getTypeDescription(sig) {


How about sigStringToTypes? Or sigStringToWasmTypes?

Good idea. I'll rename to sigToWasmTypes.

sbc100 · 2022-05-27T02:12:13Z

src/settings.js

+//  2: Depend on VM support for the wasm stack switching proposal. This allows
+//     async operations to happen without the overhead of modifying the wasm.
+//     TODO: document which of the following flags are still relevant in this
+//           mode (e.g. IGNORE_INDIRECT etc. are not needed)


Document this as experimental? Mention where this is currently supported? Link to the SS proposal?

sbc100 · 2022-05-27T02:14:50Z

tests/test_other.py

+
+    # stack switching does not asyncify the code, which means it is very close
+    # to the normal "nothing" size, and much smaller than the asyncified size
+    self.assertLess(stack_switching_size, 0.60 * asyncify_size)


Why would the wasm file not be 100% identical with stack switching?

Very minor changes might exist. For example enabling asyncify with or without stack switching will force malloc to be included, to allocate the stack. We could actually remove that in stack switching eventually, at least for the non-fiber case. Also asyncify may require different dynCalls, keeping more of them alive (it does an indirect call to resume, if we paused in one).

Akaricchi · 2022-06-06T01:25:01Z

I think you are right. We can't support fibers naively with stack switching due to that. However, I wonder if we can perhaps create more instances of the module and each fiber would have one. A little like pthreads has one instance in each Worker, actually... Anyhow, in the long term wasm stack switching will solve this even if the Promise API does not.

@kripken that's an interesting idea. However, mind that fibers are intended to be "lightweight", and a program may feasibly want to create hundreds, if not thousands, of short-lived fibers. This is the case in Taisei, which was the motivating project for the fibers API. The cost of instantiating new modules can be mitigated if we can pool and reuse them, but I don't know about the memory footprint of keeping so many instances. I'd be interested in testing it if this ever gets implemented, though.

kripken · 2022-06-06T20:54:21Z

@Akaricchi Good point, yeah, if you want hundreds or thousands than the cost of Modules can be noticeable, even if each is just 1MB or so (which I think is the right order of magnitude). So we may need to wait for full stack switching inside wasm.

fgmccabe · 2022-06-06T21:01:50Z

Neither of the above conclusions is necessarily warranted. The so-called static JS Promise integration API is a very small extension to what has already been done. That will allow for fibers; however, they will have to be managed by JS code.
OTOH, having wasm-level stack switching for one proposal will make things a lot smoother

kripken · 2022-06-06T22:37:09Z

Sounds good @fgmccabe , we can experiment with fibers that way earlier, then.

hoodmane · 2022-06-08T22:16:04Z

This is super exciting! Thanks @kripken et al!

rth · 2022-06-09T06:36:55Z

This is exciting news! @kripken we were wondering,

what's the relationship of this PR with the WASM stack switching proposal?
this would work even without any browser-specific implementations for stack switching then? And if so does it mean that that proposal is no longer necessary? @hoodmane is updating the Emscripten version used in Pyodide so we can try it. Also, there is no changelog entry for this PR, was it intentional?

hoodmane · 2022-06-09T06:45:26Z

I'm pretty sure this is dependent on the wasm stack switching proposal:
Asyncify.suspender = new WebAssembly.Suspender();
There is no such thing as WebAssembly.Suspender() in current browsers. So it's a ways off still unfortunately but it's still very exciting.

rth · 2022-06-09T08:19:17Z

OK, makes sense https://twitter.com/emscripten/status/1530272023431114752 (and the below comments) answer my questions.

kripken · 2022-06-09T16:41:07Z

Yes, this basically replaces the "userspace" Asyncify transform of the wasm with the wasm stack switching proposal. So it is 100% dependent on that support in browsers.

We didn't add this to the changelog because it is still far from stable. You can test it now, if you use latest v8 and flip a flag (--experimental-wasm-stack-switching; and you can get Chrome canary and run it with that flag for browser testing). I'd be curious for any findings from you or anyone else that wants to test it, but please expect that many things may just be broken atm.

kripken added 13 commits April 21, 2022 09:30

yolo [ci skip]

0432b81

fix

e6d6b13

work [ci skip]

cb25ead

work [ci skip]

77d55ee

work [ci skip]

1b09814

progress [ci skip]

597901e

fixes [ci skip]

4d61bcf

work towards tests [ci skip]

38405a1

test not passing [ci skip]

0507058

work towards tests [ci skip]

80b7796

size test passes [ci skip]

767c3df

passing test

951d77a

cleanup

793558b

kripken requested a review from sbc100 April 21, 2022 20:14

sbc100 reviewed Apr 21, 2022

View reviewed changes

kripken added 11 commits April 21, 2022 16:19

Merge remote-tracking branch 'origin/main' into asyncify-stack-switching

b3491ea

skip

f158b87

fix

b5d9c10

clean

a5b81bc

debugging [ci skip]

fc69835

more debugging [ci skip]

bd76cba

fix

4e83182

Merge remote-tracking branch 'origin/main' into asyncify-stack-switching

f4e5c85

cleanup

9135867

cleanup

e5ea8d5

todo [ci skip]

f5b79e1

kripken marked this pull request as ready for review May 24, 2022 22:34

kripken added 3 commits May 25, 2022 12:47

fix

4425e68

tests

34dbc13

Merge remote-tracking branch 'origin/main' into asyncify-stack-switching

12295d7

kripken requested a review from sbc100 May 25, 2022 19:49

warn+fix

7ea35b2

sbc100 approved these changes May 27, 2022

View reviewed changes

kripken added 7 commits May 27, 2022 10:05

feedback

42d7783

warn always + error on no exit-runtime

936b338

clarify

3c2b122

rename to sigToWasmTypes

86a0d0b

add experimental note + link to proposal

d224492

ignore new warning in tests

fbf7bbf

improve comment [ci skip]

70918a4

kripken merged commit 2ac0877 into main May 27, 2022

kripken deleted the asyncify-stack-switching branch May 27, 2022 19:35

antocuni mentioned this pull request Jun 8, 2022

Green threads support / greenlet pyodide/pyodide#2664

Open

hoodmane mentioned this pull request Jun 8, 2022

Incorrect time.sleep pyscript/pyscript#324

Closed

Initial Wasm Stack Switching support #16779

Initial Wasm Stack Switching support #16779

Conversation

kripken commented Apr 21, 2022

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

kripken commented Apr 21, 2022

sbc100 commented Apr 21, 2022

kripken commented Apr 21, 2022

fgmccabe commented Apr 21, 2022

kripken commented May 24, 2022

fgmccabe commented May 24, 2022

kripken commented May 25, 2022

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

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

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

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

Akaricchi commented Jun 6, 2022

kripken commented Jun 6, 2022

fgmccabe commented Jun 6, 2022 • edited Loading

kripken commented Jun 6, 2022

hoodmane commented Jun 8, 2022

rth commented Jun 9, 2022

hoodmane commented Jun 9, 2022

rth commented Jun 9, 2022

kripken commented Jun 9, 2022

fgmccabe commented Jun 6, 2022 •

edited

Loading