Wasm async compilation

[kripken]

This builds on #4859 (and must wait for it to land) to implement async compilation using WebAssembly.instantiate. As discussed in the design repo, this is enabled by default in all recommended compilation modes (everything but -O0, and it can be manually flipped off but no one should do that).

As also discussed in the design repo, this works around the problems with doing async compilation (see details there) by wrapping the exports in an indirection layer. We already had one lying around, SWAPPABLE_ASM_MODULE, so this PR reuses that. Basically, all calls to exports call Module.asm.*, so that the asm part can be replaced. Here we start with nothing, and replace it with the compiled module when it arrives. This adds overhead when calling exports, but the hope is that it isn't too bad.

@juj, the tests here require that bots have wasm enabled in the browser test suite, let me know if that's ok.

[kripken]

(first commit is from the dependency PR, second is the new stuff)

[dschuff]

Why keep the synchronous compilation in O0 and allow turning it off at all?

[kripken]

Synchronous compilation is useful in some use cases, e.g. a library like ammo or box2d, especially if it's small, might not want users to need an async step before it is ready. Also, an option to turn it off can help debugging in some cases.

Regarding -O0, we do need async on in all release modes because of what the browser people said in that design issue. So that forces us on -O1+. Perhaps we could also enable it in -O0, but it is an optimization, so makes sense not to. Another reason is it ties into existing precedent, where for asm.js -O0 is synchronous now, while optimized modes are not due to the mem init file (which we load with an xhr) - so this keeps us close to how things currently work, less surprises for people.

[kripken]

Added a commit that enables async only in wasm-only mode. I realized while talking to @juj about debugging wasm/asm.js differences that a build that can run as asm.js as well (i.e. not wasm-only) shouldn't be compiled asynchronously (for one thing it's a difference that is confusing, for another, the asm.js xhr for the mem init file, if used, can race with this async step - fixable, but maybe not worth it).

[dschuff]

#4859 LGTM, if you can rebase this I'll take a look today.

[kripken]

Rebased after the required PR landed.

[dschuff]

OK, so again just to make sure I understand:
In SWAPPABLE_ASM_MODULE mode, The global var foos and Module['foo']s that previously just pointed to the wasm exports become shims that call through the asm object instead. integrateWasmJS() sets up Module['asm']' which does sync or async instantiation and returns either real or empty imports, but I don't see where that's actually called for wasm? In the case of async instantiation it kicks off compilation and then removeRunDependency results in the wasm code actually running.

So I guess a question then: Why have the indirection shims? Why not make all the Module['foo']s point to one throwing stub (or even leave them undefined), and then when the instantiation promise resolves, point them to the real exports? With the code as-is, if the user tries to call an export before the compilation is done, the stubs will be called, but then fail when they try to call apply on asm['foo']. That does't seem any different than just having Module['foo']() fail directly.

[kripken]

Yes, in SWAPPABLE_ASM_MODULE mode, each export is actually something like

function foo(x) {
  return Module.asm.foo(x);
}

So it looks up the asm property on each call, which allows it to be replaced over time. In async mode, we set Module.asm when async compilation ends (when we do Module['asm'] = exports;). Does that answer your question? If not, then I'm not sure what you mean in

but I don't see where that's actually called for wasm?

specifically what does "that" refer to? I might be missing what you're asking here.

The reason for the indirection is that the exports can be captured synchronously:

..the JS output..
// some user code
var theMethod = Module.theMethod;
setInterval(function() {
  if (okToCall()) {
    console.log(theMethod());
  }
});

So we need to provide a capture-able thing immediately. It's not valid to call it until the runtime is set up, which may be asynchronous for various reasons - filesystem loading, mem init file, or wasm async compilation. Wasm async compilation is trickier than the others because we don't have the exports themselves yet, so we need the indirection for it.

In the future if the indirection adds overhead we can investigate adding a new compilation mode, in which it is not ok to capture the exports immediately. But that would be a breaking change, so we'd need to do it carefully.

[kripken]

Testing this in chrome (latest dev on linux), the async promise returns an instance, instead of an object containing an instance property as firefox does.

I think that might have been what an older spec said. Maybe fixed on canary already?

[dschuff]

By "that" I meant that integrateWasmJS() sets Module['asm'] to a function, but it's not clear where that function gets called.

Also ok, I hadn't thought about capturing synchronously and then calling later. Do you have a sense of whether that's the common practice, or if it's more common to just call directly like Module['foo']()?
Actually could we do the following: Continue to set up the shims as in this PR, but when the instantiation finishes, replace them with direct references to the exports, so that if the exports are not captured during the invalid time before the promise resolves, they will never be used.

Wrt what the promise returns: I don't know, I haven't tested that. I'll see if I can find out.

[kripken]

By "that" I meant that integrateWasmJS() sets Module['asm'] to a function, but it's not clear where that function gets called.

The function gets called right in between the preamble and postamble, which is right after all the JS imports are ready. We then instantiate the asm.js/wasm module by calling that function (with the imports as a param).

Might be confusing that we use Module.asm both for that function and for the output from it later, now that I think of it. Perhaps worth refactoring later, but I'm not sure offhand what that would entail.

Do you have a sense of whether that's the common practice

Not sure. For things like a game engine, usually nothing is captured anyhow (main is called automatically and that's it). For something like a library though, like ammo.js or box2d, then definitely there is plenty of capturing. But how common libraries are, I'm not sure.

Continue to set up the shims as in this PR, but when the instantiation finishes, replace them with direct references to the exports, so that if the exports are not captured during the invalid time before the promise resolves, they will never be used.

Hmm, possible in theory, but it means we create a difference between the two cases. That seems like a risk for bugs/confusion.

[dschuff]

Also it seems V8's implementation of WebAssembly.instantiate was broken in https://codereview.chromium.org/2629523007/

[dschuff]

Hmm, possible in theory, but it means we create a difference between the two cases. That seems like a risk for bugs/confusion.

How would the difference even be observable? Would people be comparing their capture === with Module['foo']? I can't think of any other way.

[kripken]

Should we wait to land this on chromium fixing instantiate? Otherwise stuff won't work in it.

The difference might be observable if someone swaps Module.asm later (the late capture wouldn't update). Could happen when using SWAPPABLE_ASM_MODULE for example, or when modifying Module.asm for debugging purposes or something like that. I agree those are corner cases, but the difference worries me.

The advantage is avoiding the wrapper overhead, but that is achievable otherwise (e.g. the user can grab Module.asm.foo after the module is ready, that's the direct method).

[dschuff]

The advantage is avoiding the wrapper overhead, but that is achievable otherwise (e.g. the user can grab Module.asm.foo after the module is ready, that's the direct method).

... except that Module.asm is supposed to be an implementation detail and not a public interface? With the LLVM wasm backend Module.asm.foo has a different name, for example. But I guess nothing is really private in JS and people can do whatever they want and live with the consequences :) I guess all that really means is that we should make sure we document the JS interfaces we expect people to use, and make sure those don't break.

V8 should be fixed with https://codereview.chromium.org/2644993002/. I think it needs to get merged onto some branch but it should end up on Canary in a day or 2.

[kripken]

Yeah, it's true Module.asm is not a public interface we recommend people use. Aside from that, though, we do use it internally, e.g. in SWAPPABLE_ASM_MODULE and perhaps in other places I can't think of right now. So a change there adds internal complexity for us, is what concerns me.

Thanks for the v8 update. Ok, seems like there is no rush here anyhow, let's wait a while before landing this. Also it seems there is still spec discussion on it, maybe things will change?

[lukewagner]

(That issue is now closed.) If there were to be a change, at most it would be a renaming, but that also seems unlikely given that both V8/SM have landed instantiate as speced in JS.md. I'd move forward with using instantiate.

[kripken]

@dschuff, do you know if now is ok to land this? (no chrome canary on linux, so i can't test if those fixes landed)

[dschuff]

This should be good to go; verified locally with d8 on trunk and 5.7.492 branch (https://omahaproxy.appspot.com/ tells me those are rolled in)

[kripken]

Ok, thanks.

Then the last blocker here is confirmation from @juj about the bots having wasm turned on, as the new tests here require that.

[dschuff]

it's not clear from this limited slice of context, but is there something somewhere explaining in more detail what SWAPPABLE_ASM_MODULE means?

[kripken]

There is a little detail in src/settings.js where the setting is defined. I improved it now.

[kripken]

Ok, after discussion with @juj, let's land this. We don't cover it properly on the bots yet, we'll fix that next week probably, til then we can just ignore these tests on the bots (but they pass when running the suite locally, which is already helpful).