Last Comment Bug 906371 - Use off thread JS parsing when loading <script async> scripts from HTML documents
: Use off thread JS parsing when loading <script async> scripts from HTML docum...
Status: RESOLVED FIXED
[games]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
-- normal (vote)
: mozilla26
Assigned To: Brian Hackett (:bhackett)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 916541 897655 906060 908294 908301 908699 912719 915485 915625 915687 916531 942984
Blocks: gecko-games 931852 916255
  Show dependency treegraph
 
Reported: 2013-08-17 15:51 PDT by Brian Hackett (:bhackett)
Modified: 2015-04-14 08:03 PDT (History)
22 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
bug 908699 leftovers (6.58 KB, patch)
2013-08-29 16:45 PDT, Brian Hackett (:bhackett)
wmccloskey: review+
Details | Diff | Splinter Review
bug 908301 leftovers (4.67 KB, patch)
2013-08-29 16:46 PDT, Brian Hackett (:bhackett)
wmccloskey: review+
Details | Diff | Splinter Review
patch (21.67 KB, patch)
2013-08-29 16:47 PDT, Brian Hackett (:bhackett)
bzbarsky: review+
Details | Diff | Splinter Review

Description User image Brian Hackett (:bhackett) 2013-08-17 15:51:42 PDT
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 User image Luke Wagner [:luke] 2013-08-19 14:01:28 PDT
\o/
Comment 2 User image Brian Hackett (:bhackett) 2013-08-29 16:45:12 PDT
Created attachment 797562 [details] [diff] [review]
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 User image Brian Hackett (:bhackett) 2013-08-29 16:46:48 PDT
Created attachment 797563 [details] [diff] [review]
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 User image Brian Hackett (:bhackett) 2013-08-29 16:47:36 PDT
Created attachment 797564 [details] [diff] [review]
patch

DOM changes (mainly nsScriptLoader) to allow <script async> scripts to be parsed off the main thread.
Comment 5 User image Brian Hackett (:bhackett) 2013-08-29 16:48:19 PDT
Morphing the title, this bug will just be about handling <script async> asynchronously.
Comment 6 User image Bill McCloskey (:billm) 2013-08-30 18:24:24 PDT
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 User image Boris Zbarsky [:bz] (still a bit busy) 2013-09-04 23:00:42 PDT
I'm sorry this is taking so long...  I'm off tomorrow, but I plan to finish this review on Friday.
Comment 8 User image Dave Camp (:dcamp) 2013-09-05 16:41:49 PDT
I don't think this should land while Debugger can't set breakpoints in these scripts.
Comment 9 User image Boris Zbarsky [:bz] (still a bit busy) 2013-09-10 22:04:51 PDT
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 User image Luke Wagner [:luke] 2013-09-11 07:33:53 PDT
Before landing, can we give this all a round of fuzzing specifically using finishOffThreadScript from bug 912321?
Comment 11 User image Brian Hackett (:bhackett) 2013-09-11 10:43:22 PDT
Gary and Christian, do you mind fuzzing this per comment 10?
Comment 12 User image Luke Wagner [:luke] 2013-09-11 11:08:02 PDT
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 User image Christian Holler (:decoder) 2013-09-11 11:30:30 PDT
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 User image Luke Wagner [:luke] 2013-09-11 12:49:41 PDT
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 User image Brian Hackett (:bhackett) 2013-09-11 16:47:55 PDT
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 User image Boris Zbarsky [:bz] (still a bit busy) 2013-09-11 17:56:33 PDT
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 User image Ed Morley [:emorley] 2013-09-12 04:12:51 PDT
https://hg.mozilla.org/mozilla-central/rev/3ca22e239a1d
Comment 18 User image Boris Zbarsky [:bz] (still a bit busy) 2013-09-12 18:23:36 PDT
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 User image Brian Hackett (:bhackett) 2013-09-13 12:38:12 PDT
(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 User image James Lal [:lightsofapollo] (inactive) 2013-09-14 22:24:55 PDT
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 User image Brian Hackett (:bhackett) 2013-09-15 06:46:02 PDT
(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 User image Brian Hackett (:bhackett) 2013-09-15 19:59:43 PDT
Patch moving UnblockOnload to the right place:

https://hg.mozilla.org/integration/mozilla-inbound/rev/26b48b6dbff4
Comment 23 User image Phil Ringnalda (:philor) 2013-09-16 23:32:15 PDT
https://hg.mozilla.org/mozilla-central/rev/26b48b6dbff4
Comment 24 User image Ting-Yu Chou [:ting] 2015-03-19 21:04:32 PDT
May I understand the reason why <script defer> is not parsed off main thread? What are the concerns? Thanks.
Comment 25 User image Brian Hackett (:bhackett) 2015-03-24 10:58:16 PDT
(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 User image David Bruant 2015-03-24 11:07:03 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.