spec icon indicating copy to clipboard operation
spec copied to clipboard

[spec] Normative: Permit work in parallel during instantiation

Open littledan opened this issue 7 years ago • 28 comments

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 https://github.com/webpack/webpack/issues/6433

littledan avatar Mar 07 '18 18:03 littledan

cc @kmiller68 @tschneidereit @domenic @lukewagner

littledan avatar Mar 08 '18 10:03 littledan

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.

tschneidereit avatar Mar 08 '18 13:03 tschneidereit

@tschneidereit Did you see @linclark's explanation of the motivation for JSC's implementation strategy in https://github.com/webpack/webpack/issues/6433 ?

littledan avatar Mar 08 '18 13:03 littledan

@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."

tschneidereit avatar Mar 08 '18 16:03 tschneidereit

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

littledan avatar Mar 08 '18 21:03 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?

littledan avatar Apr 15 '18 11:04 littledan

@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.

binji avatar Jul 18 '18 13:07 binji

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 avatar Sep 07 '18 20:09 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 avatar Oct 31 '18 11:10 littledan

I've pushed a patch to make the change as described in https://github.com/WebAssembly/spec/pull/745#issuecomment-434645244 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

littledan avatar Mar 01 '19 17:03 littledan

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

littledan avatar Mar 15 '19 22:03 littledan

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

Ms2ger avatar Mar 20 '19 16:03 Ms2ger

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.

lukewagner avatar Mar 25 '19 18:03 lukewagner

@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 https://github.com/whatwg/html/issues/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).

littledan avatar Mar 28 '19 17:03 littledan

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.

rossberg avatar Mar 29 '19 00:03 rossberg

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.

littledan avatar Mar 29 '19 00:03 littledan

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.).

smaug---- avatar Mar 29 '19 11:03 smaug----

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.

littledan avatar Mar 29 '19 14:03 littledan

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.

lukewagner avatar Mar 29 '19 18:03 lukewagner

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.

smaug---- avatar Mar 29 '19 20:03 smaug----

@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.

littledan avatar May 17 '19 09:05 littledan

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

lukewagner avatar May 17 '19 21:05 lukewagner

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.)

smaug---- avatar May 17 '19 23:05 smaug----

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.

littledan avatar May 17 '19 23:05 littledan

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?

smaug---- avatar May 17 '19 23:05 smaug----

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

rossberg avatar May 06 '22 11:05 rossberg

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.

Ms2ger avatar May 06 '22 12:05 Ms2ger

@Ms2ger, ping on this one.

rossberg avatar Aug 04 '22 09:08 rossberg

Any updates on this PR?

rossberg avatar Dec 15 '22 15:12 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.

rossberg avatar Feb 16 '23 11:02 rossberg