[spec] Normative: Permit work in parallel during instantiation

[littledan]

On JSC, some compilation work takes place the first time that a
Module is instantiated. If the module is instantiated in an
asynchronous way, the work happens off the main thread. This patch
loosens the specification of WebAssembly module instantiation
to permit this specific type of delay.

Based on skimming the code (but not running tests), I believe that
the new specification matches 3/4 of the implementations I examined.
The one which might not fit is V8, which defers reading properties
of the import object to a microtask queue item at the beginning
of WebAssembly.instantiate(Module), unlike the others, which read it
synchronously.

The previous specification didn't match any implementations, as it
specified to queue an HTML task at the beginning of the
WebAssembly.instantiate(Module) method, which no implementation
actually did to my knowledge. Punting to a queue there doesn't really
serve any purpose and was simply an error in drafting the previous
specification.

Closes #741
See also webpack/webpack#6433

[littledan]

cc @kmiller68 @tschneidereit @domenic @lukewagner

[tschneidereit]

This looks good to me as a solution to the problem at hand. However, I do wonder if it wouldn't make sense to be more strict about this: I could well imagine the kinds of timing differences permitted here to cause compatibility issues at some point.

The alternative would be to specify something matching 2/4 implementations. Assuming v8 removes the additional microtask queue item before reading import object properties, that'd go up to 3/4.

@kmiller68, could you imagine living with not being compliant for a while until you've made the necessary changes? I guess we could also just make the change in this PR, have all engines be compliant for now, and tighten things later. I'm not quite sure what that'd gain, though.

[littledan]

@tschneidereit Did you see @linclark's explanation of the motivation for JSC's implementation strategy in webpack/webpack#6433 ?

[tschneidereit]

@tschneidereit Did you see @linclark's explanation of the motivation for JSC's implementation strategy in webpack/webpack#6433 ?

I did see that, yes. Your and @kmiller68's comments[1][2] in #741 led me to believe that this is a slightly different case. If JSC won't change its behavior in the foreseeable future, then we should have that be reflected in the spec for sure.

[1] "@kmiller68 raised the concern that this is a change over what was previously agreed, and that JSC does make this kind of delay, though it'd be possible to remove it."
[2] "I don't think this behavior is essential and we could probably implement something different with enough effort."

[littledan]

OK, let's get more input from JSC, but this seems like part of what should go into V1 either way we decide.

[littledan]

We discussed this change in the WebAssembly WG meeting on April 19th, and my understanding is that it had consensus, to continue permitting the flexibility. Should we land this patch?

[binji]

@littledan I don't see the notes for this discussion (also that date seems wrong, perhaps a typo?)

If this was discussed and agreed, we should merge, though it currently has conflicts.

[littledan]

The meeting date was clearly a typo. I've pushed a rebase. I can't find any meeting notes which refer to this PR. Maybe we should discuss it in the future. cc @kmiller68

[littledan]

In a later WebAssembly CG meeting, we discussed this issue further, with the suggestion to always take an event loop turn, even if it's not necessary, for consistency across platforms. In effect, this means each module instantiation has an extra setImmediate(). Presumably this would carry forward to ESM integration as well.

I'm a little worried about the performance cost here. Is anyone else? How serious would the compatibility issue be of permitting this timing difference across implementations?

@kmiller68 Could you clarify, is there any chance you could move the slow operations from the instantiate phase to the compile phase, as @tschneidereit was asking about above?

[littledan]

I've pushed a patch to make the change as described in #745 (comment) of including an event loop turn unconditionally for asynchronous instantiation. This is important to get right, partly because the semantics here also inform the WebAssembly/ESM integration semantics (where I imagine that this event loop turn will take place during module evaluation, using top-level await).

Reviews welcome. cc @Ms2ger @xtuc

[xtuc]

"core of a WebAssembly module" sounds strange to me, what about "internals of a WebAssembly module"?

[littledan]

@Ms2ger You've incrementally landed many parts of this PR. Would you be interested in rebasing it?

[Ms2ger]

I rebased the PR; it should now be straightforward to review. @tschneidereit, others: thoughts?

[lukewagner]

As discussed in the last CG call, I landed this change in FF. (It should make it into the next Nightly.) I'll report back if we hear of any breakage, although I don't expect any.

[littledan]

@kmiller68, @tschneidereit and I have been discussing the semantics here. With this PR, each time a WebAssembly module is instantiated, there would be an event loop turn, in order to leave time for compilation. It sounds like this is needed for JSC the way it's implemented right now; it may be possible to rearchitect JSC for this purpose, but it's not clear yet whether that would lead to performance regressions.

I'd just like to confirm that we're comfortable with these semantics, as they're rather different from JavaScript semantics. The ESM integration proposal would carry over this per-module event loop turn.

In whatwg/html#4400 , @nyaxt , @smaug---- and @rniwa expressed some skepticism about yielding to the event loop for each JavaScript module; this patch does something similar for WebAssembly.

It may be that this is OK for WebAssembly and not for JavaScript as WebAssembly is generally a small number of big modules, whereas JavaScript may be deployed in many small modules. We may also be able to specify an event loop turn here, and later decide that it's optional or missing (however, such a two-step change could have compatibility risk).

[rossberg]

WebAssembly is generally a small number of big modules, whereas JavaScript may be deployed in many small modules.

I'm not sure that we should be making assumptions of this sort.

[littledan]

I worded that a bit clumsily; I meant more like, if we expect that, then that would be one story for why not to worry about this impact/alignment with JS.

[smaug----]

Currently the web doesn't really have explicit event loop spinning (modulo some implementation details and the somewhat unclear alert() handling in the spec). I'd rather not see us adding such. It complicates many things quite a lot. You may want to also ask blink folks why they wanted to remove showModalDialog rather quickly after finding some issue (IIRC it was causing major issues there because of nested event loop spinning, but perhaps I misremember).

If we really want event loop to spin (why?), then it needs to be clearly defined what happens if the world dies during spinning (closing window, or removing script element or modifying the contents of the script element etc.).

[littledan]

To clarify, no one here is talking about spinning the event loop. Instead, instantiating a WebAssembly module would, in this patch, have the possibility to do asynchronous work (e.g., compilation based on the imports), before queueing a task to finish the instantiation.

[lukewagner]

While writing the above-mentioned patch, I realized another use case for being able to do non-trivial (off-main-thread) compilation during instantiation after the import values are known: Web IDL Bindings where, for best perf, you want to generate thunks specialized to the caller+callee.

[smaug----]

To clarify, no one here is talking about spinning the event loop.

I see. Well, then just queue a task and continue. Of course the programming model is then a tad more error prone since the world may change before the task runs, but that happens with other APIs too.

What needs to be defined is that does the new task block page's load event firing, if the initial step is executed during pageload. And same with DOMContentLoaded.

But some concrete example somewhere would help random reader to understand the issue better.

[littledan]

@smaug---- Nothing in this PR would affect when the loaded event fires, because loading a WebAssembly module does not relate to that event. WebAssembly is loaded with APIs like instantiateStreaming, which would see one more task queued internally before running the start function; you can find examples of usage in that MDN page. With <script type=module> in the context of Wasm/ESM integration, the effects would be similar, but also not change when those events fire.

[lukewagner]

As an update, the patch is about to advance to Beta. No breakage yet reported.

[smaug----]

Are there wpt tests ensuring page's load event isn't affected?
I ask because in Gecko network requests before page's load event by default postpone page's load event to fire. (and if that behavior isn't wanted, request need to be marked with a certain flag.)

[littledan]

I don't know how I would write such a test, since I am not sure what scenario you are concerned about. It doesn't seem plausible to me that the JS instantiateStreaming API would block the load event; there is currently no declarative way to fetch Wasm.

[smaug----]

It has nothing to do with declarative or imperative. Just about fetching wasm.
If fetch is started right before load event for the page would otherwise fire, is the fetch finished before or after page's load event actually fires?

[rossberg]

This PR is super-stale. Is it still relevant?

[Ms2ger]

I'm not sure why it never landed. It seems like SpiderMonkey and JSC might implement the change already? I can try to find time to rebase.

[rossberg]

@Ms2ger, ping on this one.

[rossberg]

Any updates on this PR?

[rossberg]

This PR is 5 years old and received no reaction from multiple pings over the past year. Unless there is visible activity, I will take the liberty to close it.

[Ms2ger]

As far as I can tell, this is ready to land.

[rossberg]

@Ms2ger, sounds good. I'm not sure if @littledan is still available, could you take over landing this?

[tomstuart]

This PR was opened against master, which was the default branch at the time, but five years later the default branch is now main and it should’ve been merged into that branch instead. I’ve opened #1606 to target the correct branch.