906371 - Use off thread JS parsing when loading <script async> scripts from HTML documents

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

This should be doable using the machinery that was added in bug 897655.  I'm not sure yet whether we should do this for all <script> tags, or just <script async> and/or <script defer>.

Comment 1

[Luke Wagner [:luke]]

\o/

Comment 2

[Brian Hackett [Laid off!]]

Attachment: bug 908699 leftovers

Fix some leftover stuff from bug 908699.  AtomToPrintableString would just return NULL on an ExclusiveContext, which caused error reporting to crash in some cases.

Comment 3

[Brian Hackett [Laid off!]]

Attachment: bug 908301 leftovers

With bug 908301, we can deadlock when a worker thread tries to wait on another worker compressing source and that other thread is paused, as the first thread itself will never pause and no progress will be made.

Comment 4

[Brian Hackett [Laid off!]]

Attachment: patch

DOM changes (mainly nsScriptLoader) to allow <script async> scripts to be parsed off the main thread.

Comment 5

[Brian Hackett [Laid off!]]

Morphing the title, this bug will just be about handling <script async> asynchronously.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 797563 [details] [diff] [review]
bug 908301 leftovers

Review of attachment 797563 [details] [diff] [review]:
-----------------------------------------------------------------

Man, this is getting complicated. Can you remind me why we pause workers during GC? It seems like we might be able to get away without it (although doing so might be more work than this patch).

::: js/src/jsworkers.cpp
@@ +781,5 @@
> +        // loop in AutoPauseWorkersForGC.
> +        if (cx->workerThread()) {
> +            state.numPaused++;
> +            if (state.numPaused == state.numThreads)
> +                state.notify(WorkerThreadState::MAIN);

Can we rename MAIN and WORKER to something like CONSUMER and PRODUCER? Otherwise, this code is pretty confusing to read.

@@ +788,2 @@
>          while (state.compressionInProgress(this))
>              state.wait(WorkerThreadState::MAIN);

It seems like multiple threads now can be waiting simultaneously on MAIN. Therefore, it seems like we should be using notifyAll(MAIN) in all the places where we're using notify(MAIN) now. Alternatively, we could use more condition variables or something.

@@ +790,5 @@
>  
> +        if (cx->workerThread()) {
> +            state.numPaused--;
> +            if (state.shouldPause)
> +                cx->workerThread()->pause();

Is the needed for correctness, or is it just an extra maybePause sort of check?

Comment 7

[Boris Zbarsky [:bzbarsky]]

I'm sorry this is taking so long...  I'm off tomorrow, but I plan to finish this review on Friday.

Comment 8

[Dave Camp (:dcamp)]

I don't think this should land while Debugger can't set breakpoints in these scripts.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 797564 [details] [diff] [review]
patch

>+  nsAutoCString mURL;

Please document that this is used to keep the "filename" used by the JS engine alive across a background compilation?

>+class NotifyOffThreadScriptLoadCompletedRunnable : public nsRunnable

Please put this in an anonymous namespace.

>+      JS::FinishOffThreadScript(NULL, rt, mToken);

nullptr, please.

>+nsScriptLoader::AttemptAsyncScriptParse(nsScriptLoadRequest* aRequest)

>+  if (!aRequest->mElement->GetScriptAsync() || aRequest->mIsInline)
>+    return NS_ERROR_FAILURE;

Curly braces around all blocks, please, here and elsewhere.

>+  if (!JS::CompileOffThread(cx, global, options,

Does this require cx to stay alive until OffThreadScriptLoaderCallback happens?

>+  AutoPushJSContext cx(context->GetNativeContext());

I _think_ this can GC, so unrootedGlobal is a gc hazard.  :(

You want to put the Rooted before this line.

Same thing in nsScriptLoader::EvaluateScript.

It may be worth a comment that which JSContext we use here matters, for error reporting.

>+  if (aRequest->mIsInline) {

Given the first line of this function, this will always test false.  Is it vestigial?  If so, please remove.

>+  mOffThreadScriptRequest = aRequest;

Why do we need this?  In particular, why only allow one off-thread compilation at a time?

You do need onload blocking here.  Right now, the unblock happens at the end of nsScriptLoader::ProcessPendingRequests if there is nothing pending.  So either that needs to not unblock if there are off-thread compilations going (but then you need to call back into that code after the off-thread thing is done or something) or the off-thread compilation needs to explicitly block/unblock onload.

>+++ b/dom/base/nsJSUtils.cpp
>+      *aOffThreadToken = NULL; // Mark the token as having been finished.

nullptr.

r=me with the above fixed.  Sorry for the lag.  :(

Comment 10

[Luke Wagner [:luke]]

Before landing, can we give this all a round of fuzzing specifically using finishOffThreadScript from bug 912321?

Comment 11

[Brian Hackett [Laid off!]]

Gary and Christian, do you mind fuzzing this per comment 10?

Comment 12

[Luke Wagner [:luke]]

In particular, it would be valuable if the fuzzers were able to pound on the usage of offThreadCompileScript+finishOffThreadScript in a threadsafe build with --ion-parallel-compile=on (generating the input analogous to what would be passed to eval/evaluate).

Comment 13

[Christian Holler (:decoder)]

Do you mind quickly describing here how these two functions should be used? Is it just that you pass code to offThreadCompileScript and then just call finishOffThreadScript without any arguments?

Comment 14

[Luke Wagner [:luke]]

IIUC,
  offThreadCompileScript(str);var result = finishOffThreadScript();
is basically the same as
  var result = evaluate(str);
(offThreadCompileScript doesn't accept any fancy options like evaluate, though.)

The two main interesting vectors to fuzz I see are (1) feeding a bunch of stuff into the string argument to offThreadCompileScript and (2) putting a bunch of JS between the calls to offThreadCompileScript and finishOffThreadScript.  Note: calling offThreadCompileScript() when there is already a compilation job pending appears to throw a runtime error.

Comment 15

[Brian Hackett [Laid off!]]

mOffThreadScriptRequest is there so that a reference is maintained on the nsScriptLoadRequest for the off thread parse, and so that it can be identified when the off thread parse finishes.  It would be nice, yeah, if this was improved so that a single script loader could have multiple off thread parses in flight at once.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ca22e239a1d

Comment 16

[Boris Zbarsky [:bzbarsky]]

I see.  Mind following a followup to have the event hold references to both the script load request and the nsScriptLoader or something?  Then we'd be able to have more than one of these in flight at once.

Might also be worth adding telemetry to see how often we avoid going off-thread because mOffThreadScriptRequest is set.

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3ca22e239a1d

Comment 18

[Boris Zbarsky [:bzbarsky]]

I think we want to unblock onload _after_ we execute the script.  Otherwise we'll possibly fire the onload event before the script runs.

Comment 19

[Brian Hackett [Laid off!]]

(In reply to Boris Zbarsky [:bz] from comment #18)
> I think we want to unblock onload _after_ we execute the script.  Otherwise
> we'll possibly fire the onload event before the script runs.

OK, I added a fix for this to the patch in bug 916255.

Comment 20

[James Lal [:lightsofapollo]]

Per comment #18 (and see https://bugzilla.mozilla.org/show_bug.cgi?id=916255) this basically breaks some assumptions about how the script tag loading works... Can we back this out and reland with the fix (and hopefully a test) we saw some serious breakage in gaia because of this change in behavior.

Comment 21

[Brian Hackett [Laid off!]]

(In reply to James Lal [:lightsofapollo] from comment #20)
> Per comment #18 (and see
> https://bugzilla.mozilla.org/show_bug.cgi?id=916255) this basically breaks
> some assumptions about how the script tag loading works... Can we back this
> out and reland with the fix (and hopefully a test) we saw some serious
> breakage in gaia because of this change in behavior.

I'll try to land a fix with just the OnLoad movement later today.  It's going through try right now (https://tbpl.mozilla.org/?tree=Try&rev=1784cd49c8d0 as I don't know if it caused the tp5 timeouts in bug 916255 or not.  Also note that parallel script parsing can be disabled via the javascript.options.parallel_parsing config option.

Comment 22

[Brian Hackett [Laid off!]]

Patch moving UnblockOnload to the right place:

https://hg.mozilla.org/integration/mozilla-inbound/rev/26b48b6dbff4

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/26b48b6dbff4

Comment 24

[Ting-Yu Chou [:ting] (away)]

May I understand the reason why <script defer> is not parsed off main thread? What are the concerns? Thanks.

Comment 25

[Brian Hackett [Laid off!]]

(In reply to Ting-Yu Chou [:ting] from comment #24)
> May I understand the reason why <script defer> is not parsed off main
> thread? What are the concerns? Thanks.

I don't think there's any fundamental reason, it just hasn't been implemented.  The semantics of <script async> and <script defer> are not the same.

Comment 26

[David Bruant]

(In reply to Brian Hackett (:bhackett) from comment #25)
> (In reply to Ting-Yu Chou [:ting] from comment #24)
> > May I understand the reason why <script defer> is not parsed off main
> > thread? What are the concerns? Thanks.
> 
> I don't think there's any fundamental reason, it just hasn't been
> implemented.  The semantics of <script async> and <script defer> are not the
> same.
I took the liberty to file bug 1147048 then. I use @defer much more often than I use @async for the applications I deploy.