1167409 - Enable off-main-thread parsing of blocking scripts

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

Sub-bug to enable off-main-thread parsing of scripts that block the main HTML parser.  These are "typical" scripts that are present in <script> tags which are not marked 'async'.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: move-offthread-token-into-script-load-request.patch

Current off-main-thread parsing of async scripts uses a token value that's passed back from the JS compiler to the browser.  This token is then used to look up the compiled JSScript when the pre-compiled script needs to be run.

This patch moves the off-thread token from being a parameter that's passed around to the various processing functions, to a field that lives on nsScriptLoadRequest.

Comment 2

[Kannan Vijayan [:djvj]]

Try run for above patch:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=38adfa30fe7e

Comment 3

[Kannan Vijayan [:djvj]]

Attachment: factor-off-thread-parsing-into-separate-function.patch

Currently, the attempt to off-thread parse a script is made within the body of 'ProcessRequest'.  The attempt method checks the request to only try off-thread parsing on async scripts.

This patch factors the call to AttemptAsyncScriptParse into a separate method: 'ParseOffThreadOrProcessRequest', which is called only from the async-script processing code.

This is to allow cleaner usage of attempting off-thread compilation in places where we don't necessarily want to process the script immediately.

Try run:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba652c20f6ea

Comment 4

[Kannan Vijayan [:djvj]]

Attachment: change-mloading-to-mprogress.patch

Currently, nsScriptLoadRequest has an 'mLoading' field that is true when the script data is being loaded, and set to false when it's done loading.

In the new code, scripts may be a few different states depending on how they are to be processed: loading, done loading, compiling, and done compiling.  Some scripts will go from loading -> done loading -> execute, some scripts will go from loading -> done loading -> compiling -> done compiling -> execute.

This patch changes 'bool mLoading' into 'Progress mProgress', where Progress is an enumeration of the states above.  Logic which currently checks 'mLoading' is changed to use better-named methods that read the underlying mProgress var.

Comment 5

[Kannan Vijayan [:djvj]]

Attachment: off-thread-compile-blocking-scripts.patch

This is the final patch, and I think it has some bugs still.

It changes logic in 'ProcessScriptElement' to attempt off-thread compilation of parser-blocking scripts.  It also changes the 'OnStreamComplete' handler to try off-thread parsing of scripts which are blocking the parser.

It has a number of oranges on try which need to be tracked down, but should structurally be pretty solid.

Basically, we change some places where we are currengly 'ProcessRequest'ing a request, and instead try an off-thread compile first.  If the off-thread compile is successfully initiated, then the parser-blocking script is kept around, and the callback run at completion-of-compile processes the request - either directly, or by issuing an async command on the event loop.

Try run:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=5246f5c7e4e5

Comment 6

[Kannan Vijayan [:djvj]]

Attachment: off-thread-compile-blocking-scripts.patch

Updated patch to appropriately call unblockOnload in the case where an off-thread compiled script is handled via a call to ProcessPendingRequests.

Latest try runs still show oranges, but far fewer (the bc1 and cgc and dt errors are separate): https://treeherder.mozilla.org/#/jobs?repo=try&revision=32ce366d80ef

I'm tracking down the bc2 errors now, and it seems that "bocking script execution" is not done quite correctly in the new logic.  Somehow, there is script execution that continues despite the main page being blocked on an off-thread compiling synchronous script.

Comment 7

[Kannan Vijayan [:djvj]]

Fixing the remaining batches of orange on try, I've been focusing on the |bc2| errors above: browser/components/uitour/test/browser_UITour_loop.js

The basic issue is this: one of the tests in the UITour deals with the "loop" or "Firefox Hello" feature.  Referring to firefox hello as "loop" for this discussion as that's how it's referred to in code.

The test clicks the 'loop button' (presumably the hello button at top), and then waits for the 'loopButton.open' field to become true, then does somethign with the ContentAPI, then checks to see if the 'getting started button' exists:

    Services.prefs.setBoolPref("loop.gettingStarted.seen", false);
    Services.prefs.setCharPref("loop.gettingStarted.url", "http://example.com");
    is(loopButton.open, false, "Menu should initially be closed");
    loopButton.click();

    yield waitForConditionPromise(() => {
      return loopButton.open;
    }, "Menu should be visible after showMenu()");

    gContentAPI.registerPageID("hello-tour_OpenPanel_testPage");
    yield new Promise(resolve => {
      gContentAPI.ping(() => resolve());
    });

    let loopDoc = document.getElementById("loop-notification-panel").children[0].contentDocument;
    let gettingStartedButton = loopDoc.getElementById("fte-button");
    ok(gettingStartedButton, "Getting Started button should be found");


The test fails on the last line, noting that |gettingStartedButton| is not found.  Internally comparing logs between my patches firefox and reference firefox, I can see that the reference firefox loads and executes MANY scripts before the |fte-button| check occurs.  In my patched firefox, the logs indicate that the |react.js| script is scheduled for asynchronous parsing, but the failing assertion is reached before that compile even completes.  Furthermore, |fte-button| is a button added using React APIs.

So it seems like pretty strong evidence that the test code is continuing execution before the necessary scripts are parsed and executed.  I'm not sure how or why this happens, as my patched firefox _should_ be blocking page parsing when it's off-thread compiling scripts (using the same mechanism that pages are blocked when loading js code from netwerk).

Yet somehow the test code is running ahead of the time it usually should.

Comment 8

[Kannan Vijayan [:djvj]]

Attachment: test-failure-1.patch

Patch for replicating test failure 1 (browser/devtools/markupview/test/browser_markupview_search_01.js)

Comment 9

[Kannan Vijayan [:djvj]]

Attachment: test-failure-2.patch

Patch for recreating test failure 2 (browser/components/uitour/test/browser_UITour_loop.js)

Comment 10

[Kannan Vijayan [:djvj]]

I spent the last week meticulously tracing between test code, browser frontend code, and browser DOM code to track these failures.  Eventually I ended up resorting to bisecting my own patches, and used the results of that to come up with these two reduced patches, which cause the test failures when applied to tip.

The second patch (test-failure-2.patch) is particularly interesting to me.  It simply comments out an "optimization" - where nsScriptLoader processes a script syncrhonously instead of putting it on the mParserBlockingScript field and running through the event loop once and processing it asynchronously.

The first patch (test-failure-1.patch) modifies the behaviour of '::OnStreamComplete'.  Typically, OnStreamComplete will process incoming mParserBlockingScript scripts immediately and synchronously.  This patch modifies OnStreamComplete to push script processing off by scheduling script processing for the next event loop.

It seems like neither of these should affect execution semantics, and should only affect timing or incidental script execution ordering.

Comment 11

[Kannan Vijayan [:djvj]]

Attachment: test-failure-1.patch

Updated test-failure-1 patch changed to BlockOnload when deferring script processing, and UnblockOnload after processing.  Still shows test failure.

Comment 12

[Kannan Vijayan [:djvj]]

Attachment: test-failure-2.patch

Updated test-failure-2 patch changed to BlockOnload when deferring script processing, and UnblockOnload after processing.  Still shows test failure.

Comment 13

[Kannan Vijayan [:djvj]]

After a bunch of investigation, I'm reasonably sure these patches are good. Going to go ahead and get review on them (after a final try run) and wait for test issues to clear up.

Comment 14

[Kannan Vijayan [:djvj]]

Attachment: 1-move-offthread-token-into-script-load-request.patch

Comment 15

[Kannan Vijayan [:djvj]]

Attachment: 2-factor-process-request-and-off-thread-parsing-into-separate-functions.patch

Comment 16

[Kannan Vijayan [:djvj]]

Attachment: 3-change-mloading-to-mprogress.patch

Comment 17

[Kannan Vijayan [:djvj]]

Attachment: 4-off-thread-compile-blocking-scripts.patch

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8620614 [details] [diff] [review]
1-move-offthread-token-into-script-load-request.patch


>+    JSRuntime *rt;
* goes with the type



> 
>+  void** OffThreadTokenPtr() {
Nit, { goes to the next line

>+    return mOffThreadToken ?  &mOffThreadToken : nullptr;
>+  }

Hmm, a tiny bit odd to have getter for &mOffThreadToken, but not for mOffThreadToken.
Perhaps add also 
void* OffThreadToken()
{
  return mOffThreadToken;
}
and use use and not aRequest->mOffThreadToken


>   using super::getNext;
>   using super::isInList;
> 
>   nsCOMPtr<nsIScriptElement> mElement;
>   bool mLoading;          // Are we still waiting for a load to complete?
>   bool mIsInline;         // Is the script inline or loaded?
>   bool mHasSourceMapURL;  // Does the HTTP header have a source map url?
>   bool mIsDefer;          // True if we live in mDeferRequests.
>   bool mIsAsync;          // True if we live in mLoadingAsyncRequests or mLoadedAsyncRequests.
>   bool mIsNonAsyncScriptInserted; // True if we live in mNonAsyncExternalScriptInsertedRequests
>   bool mIsXSLT;           // True if we live in mXSLTRequests.
>   bool mIsCanceled;       // True if we have been explicitly canceled.
>+  void* mOffThreadToken;  // Off-thread parsing token.
Perhaps worth to document when it is set

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8620616 [details] [diff] [review]
2-factor-process-request-and-off-thread-parsing-into-separate-functions.patch

> nsresult
>+nsScriptLoader::ParseOffThreadOrProcessRequest(nsScriptLoadRequest* aRequest)
>+{
>+  NS_ASSERTION(nsContentUtils::IsSafeToRunScript(),
>+               "Processing requests when running scripts is unsafe.");
>+  NS_ASSERTION(!aRequest->mOffThreadToken,
>+               "Candidate for off-thread parsing is already parsed off-thread");
>+
>+  nsresult rv = AttemptAsyncScriptParse(aRequest);
>+  if (rv != NS_ERROR_FAILURE)
>+    return rv;
always {} with if
(I know you just moved this code, but still)



>   while (mEnabled && !mLoadedAsyncRequests.isEmpty()) {
>     request = mLoadedAsyncRequests.StealFirst();
>-    ProcessRequest(request);
>+    ParseOffThreadOrProcessRequest(request);
It is not clear at all after this patch when one should call ProcessRequest and when ParseOffThreadOrProcessRequest
but I assume the next patch explains that. (it is about the async-ness)

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8620617 [details] [diff] [review]
3-change-mloading-to-mprogress.patch


>+  enum Progress {
>+    Progress_Loading,
>+    Progress_DoneLoading,
>+    Progress_Compiling,
>+    Progress_DoneCompiling
>+  };
>+  bool IsReadyToRun() {
>+    return mProgress == Progress_DoneLoading ||
>+           mProgress == Progress_DoneCompiling;
>+  }
>+  bool IsLoading() {
>+    return mProgress == Progress_Loading;
>+  }
>+  bool IsDoneLoading() {
>+    return mProgress == Progress_DoneLoading;
>+  }
>+  bool InLoadingStage() {
>+    return IsLoading() || IsDoneLoading();
>+  }
{ goes to its own line with methods.

IsLoadingStage() and IsReadyToRun() may both return true at the same time.
For a random (non-native-English) reader the difference between IsLoading() and InLoadingStage() is rather vague I'd say.
Given that you use InLoadingStage() in assertions (which probably should be MOZ_ASSERTs), perhaps make this
InCompilingStage() and then !InCompilingStage() in assertions and change the assertion text a bit.

Comment 21

[Kannan Vijayan [:djvj]]

(In reply to Olli Pettay [:smaug] (for generic DOM reviews, you may want to look for other reviewers too ;)) from comment #18)
> Hmm, a tiny bit odd to have getter for &mOffThreadToken, but not for
> mOffThreadToken.
> Perhaps add also 
> void* OffThreadToken()
> {
>   return mOffThreadToken;
> }
> and use use and not aRequest->mOffThreadToken

I think your comment got mangled when you were writing.  Could you restate?  I don't understand what you're suggesting here.

Comment 22

[Kannan Vijayan [:djvj]]

So the latest try push looks pretty good (the other oranges are all intermittent or from the base revision the patches were applied to), except for a consistent |g2| crash in the debugger when talos-testing devtools on opt builds.  Seems to happen pretty consistently across platforms:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=70baeefe36ff

I'm trying to replicate this locally but not having any on an opt 64-bit build on linux.  Trying an opt 32-bit build in a linux VM now.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Kannan Vijayan [:djvj] from comment #21)
> (In reply to Olli Pettay [:smaug] (for generic DOM reviews, you may want to
> look for other reviewers too ;)) from comment #18)
> > Hmm, a tiny bit odd to have getter for &mOffThreadToken, but not for
> > mOffThreadToken.
> > Perhaps add also 
> > void* OffThreadToken()
> > {
> >   return mOffThreadToken;
> > }
> > and use use and not aRequest->mOffThreadToken
> 
> I think your comment got mangled when you were writing.  Could you restate? 
> I don't understand what you're suggesting here.
s/use use/use that/

So, add a getter for mOffThreadToken

Comment 24

[Kannan Vijayan [:djvj]]

(In reply to Olli Pettay [:smaug] from comment #23)
> > I think your comment got mangled when you were writing.  Could you restate? 
> > I don't understand what you're suggesting here.
> s/use use/use that/
> 
> So, add a getter for mOffThreadToken

I don't need a getter for mOffThreadToken because it's a public var.  The reason I need an get-address-of is that the address of the field is passed into nsJsUtils::EvaluateString which doesn't know about the request.  It receives a pointer-to-void-pointer which represents the token, and if non-null, uses it.  If the token is consumed by EvaluateString, EvaluateString sets the token to nullptr.  So the field is passed in as an outvar to EvaluateString.

I can work around this by having the caller to EvaluateString cache the void-star in a local var, and pass in a pointer to the local var, and after return update mOffThreadToken, but it's effectively equivalent to this.

I can go either way on this depending on how strongly you feel about it, but I think the get-address-of method we have now is sufficient and mirrors pre-existing usage pattern of the off-thread-token (it's passed around currentely as a pointer-to-void-star).

Comment 25

[Kannan Vijayan [:djvj]]

(In reply to Kannan Vijayan [:djvj] from comment #22)
> So the latest try push looks pretty good (the other oranges are all
> intermittent or from the base revision the patches were applied to), except
> for a consistent |g2| crash in the debugger when talos-testing devtools on
> opt builds.  Seems to happen pretty consistently across platforms:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=70baeefe36ff
> 
> I'm trying to replicate this locally but not having any on an opt 64-bit
> build on linux.  Trying an opt 32-bit build in a linux VM now.

I can't replicate this g2 crash I'm seeing in that try build.  Tried a local opt 32-bit and 64-bit linux build, no luck.  Got a loaner talos machine and ran the same talos build with a download of the try build, and the crash doesn't replicate.

Going to try to reduce my patches again and try to get a simple set of changes that reproduces this error.  That might be a better avenue.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

The getter for mOffThreadToken would be just for consistency - nothing else.
So either have getter for the address of mOffThreadToken and mOffThreadToken itself or don't have
getter for neither.

Comment 27

[Kannan Vijayan [:djvj]]

Ok.  I'll remove the pointer-getter then.  We never need to access the bool directly, it gets passed in as a pointer and set in js-specific code that doesn't have access to the request.

Comment 28

[Kannan Vijayan [:djvj]]

Finally managed to replicate the g2 crash seen here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5090fa33cf9b

The crash itself is occurring within the getter-function for '.element' on  debugger-proxy for a script source.  It occurs inside |DebuggerSource_getElement|.  It turns out that the |element| slot on the ScriptSourceObject for the script is initialized to a poison Magic value.

Looking into the initialization of ScriptSourceObject, we run into this
(https://dxr.mozilla.org/mozilla-central/source/js/src/frontend/BytecodeCompiler.cpp?from=CreateScriptSourceObject&case=true#195):

    // Off-thread compilations do all their GC heap allocation, including the
    // SSO, in a temporary compartment. Hence, for the SSO to refer to the
    // gc-heap-allocated values in |options|, it would need cross-compartment
    // wrappers from the temporary compartment to the real compartment --- which
    // would then be inappropriate once we merged the temporary and real
    // compartments.
    //
    // Instead, we put off populating those SSO slots in off-thread compilations
    // until after we've merged compartments.
    if (cx->isJSContext()) {
        if (!ScriptSourceObject::initFromOptions(cx->asJSContext(), sso, options))
            return nullptr;
    }

the |initFromOptions| call is what initializes the |element| field in a ScriptSourceObject.  For off-thread compiles, this is delayed until compartment merge time.

Somehow, these partially-initialized ScriptSourceObjects are being made visible to chrome (specifically the debugger), which is using them before they are initialized.

The javascript stack at the time of the crash is:

#0          (nil)   resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/utils/TabSources.js:288 (0x7f21644d0830 @ 61)
#1          (nil)   resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/utils/TabSources.js:360 (0x7f21644d0c18 @ 13)
#2          (nil)   resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922 (0x7f21644d3e70 @ 100)
#3          (nil)   resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:801 (0x7f21644d3a88 @ 72)
#4          (nil)   resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:740 (0x7f21644d38f8 @ 8)

So it's some event triggering a promise resolution that calls into some code that lists script source objects.  This code gets its hands on not-fully-initialized script source objects.

Next step is to figure out how and where ScriptSourceObjects are escaping.  There may be an assumption somewhere else in the code that emits events for particular scripts getting compiled, before the compartment merge happens and the ScriptSourceObject gets fully initialized.

Comment 29

[Kannan Vijayan [:djvj]]

Shu pointed to this as https://dxr.mozilla.org/mozilla-central/source/js/src/vm/Debugger.cpp#3688

The findScripts code figures out which scripts to expose to debugger.  Might need to fix some of the conditionals there.

Comment 30

[Kannan Vijayan [:djvj]]

Hey guys, I need some insight into this issue.

To give some context to this patchset (so you don't have to read all the comments), I'm trying to move most script parsing off the main thread and onto a background thread.  The patches for this are written and mostly green, but opt build crash on Talos G2 builds.

I discovered that this is due to the issues described on comment 28 - a ScriptSourceObject that has not been fully initialized is leaking out to the debugger frontend code.  This happens because off-main-thread-parsed scripts are parsed in a separate compartment from the main compartment.  This means that memory allocations during parsing (such as the allocation of ScriptSourceObject) happen in a separate compartment.  To handle this, the full initialization of a ScriptSourceObject is put off until an off-main-thread parse completes and the parse compartment is merged with the main compartment.

The issue is that before this happens, the SSO is getting leaked to the debugger frontend, which tries to get the '.element' property off of it, which has not been initialized, and thus crashes.

I need to figure out what control flow paths the SSO object takes from creation to escaping-to-debugger-frontend.  For all of those paths, I need to identify SSO objects created on off-main-thread parsed scripts, and ensure that they are "held back" until the parse completes.

Insight and advice would be greatly appreciated.

Comment 31

[Kannan Vijayan [:djvj]]

After more digging, and printf instrumentation, I've learned some more about what's going on:

1. The SSO we are crashing on is created as part of an off-main-thread compile.
2. The reason the SSO is never ititialized is because the compile (fronted::CompileScript) fails, returning null to HelperThread::handleParseWorkload.  This causes the ParseTask's script to get set to null.
3. I printf-instrumented the Debugger::ScriptQuery::consider function to print all JSScripts.  The script in question doesn't show up in it.
4. Which means that the SSO is somehow escaping into chrome js, and it's not going through Debugger::findScripts.

Comment 32

[Kannan Vijayan [:djvj]]

Figured out the problem.  This is what happens:

1. Off-main-thread compile is started, and creates an incomplete SSO object as part of the compile.  The expectation is that the SSO object will be fully initialized once the OMTC (off-main-thread compile) compartment is merged.

2. During the compile, the top-level JSScript is allocated, pointing to the SSO.  The top-level JSScript's code_ field is null (it's not set until the end of compilation), so this is ok, because the script can't be used.

3. However, inner JSScripts that are allocated can complete their compilation fully.  These scripts reference the same SSO, and their code_ field is not null.

4. The top-level compile fails (for whatever reason).  This sets the OMTC ParseTask->script to null.

5. On the main thread, after compartment merge, the code tries to fully initialize the SSO, but it can't because it gets the SSO from the ParseTask->script, which is now null.  The assumed logic here is "the script failed to compile and is not valid anyway, so it doesn't matter if the SSO for it is initialized or not".

6. Post-merge, the JSScript, and all of the inner JSScripts are in the new compartment.  The top-level JSScript is invalid (null code_ pointer), but the inner JSScripts are perfectly valid (valid code_ pointer).  The inner JSScripts point to a partially initialized SSO.

7. findScripts discovers the "successfully compiled" inner JSScripts, and accesses the partially initialized SSO from it, leading to the crash.

Comment 33

[Kannan Vijayan [:djvj]]

Ok, fixed the crash.  Try runs look clean (couple of them):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb349bc1f1a3
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b71b149efd99

Also, talos shows a significant perf IMPROVEMENT (yay) of 30% or more on V8 on windows: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=8a9734a9d97d&newProject=try&newRevision=bb349bc1f1a3

The improvement is not a fluke.  Holds between different try runs.  I am not sure _exactly_ why, but I'll take a look at the horse's mouth later.

Comment 34

[Kannan Vijayan [:djvj]]

Attachment: 0-move-offthread-token-into-script-load-request.patch

Comment 35

[Kannan Vijayan [:djvj]]

Attachment: 1-factor-process-request-and-off-thread-parsing-into-separate-functions.patch

Comment 36

[Kannan Vijayan [:djvj]]

Attachment: 2-change-mloading-to-mprogress.patch

Comment 37

[Kannan Vijayan [:djvj]]

Attachment: 3-off-thread-compile-blocking-scripts.patch

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

I wonder what the v8 test is actually testing. Does it test js parsing/compiling time + some benchmark, when one would expect it to test only the benchmark part.

Comment 39

[Kannan Vijayan [:djvj]]

Attachment: 4-initialize-sourceobject-when-compilation-fails.patch

Comment 40

[Kannan Vijayan [:djvj]]

(In reply to Olli Pettay [:smaug] from comment #38)
> I wonder what the v8 test is actually testing. Does it test js
> parsing/compiling time + some benchmark, when one would expect it to test
> only the benchmark part.

I'm not sure what's going on here, specifically, but the improvements hold across multiple comparisons with builds off of different tips, so they seem reliable.

I tried to get a breakdown on which tests were doing better, but the perfherder breakdown page didn't load.  I was just looking for "no regressions", so I'm not too concerned about getting to the bottom of this just yet.

Comment 41

[Kannan Vijayan [:djvj]]

Comment on attachment 8639439 [details] [diff] [review]
4-initialize-sourceobject-when-compilation-fails.patch

So, I'm giving this review to you since you did the rest of the reviews.  It touches JS-land a bit more heavily, but nothing serious.  It just makes sure that ScriptSourceObject gets initialized properly even if script compilation failed.

Mostly it's ensuring that frontend::CompileScript emits a ScriptSourceObject independently of the returned JSScript*.

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8639439 [details] [diff] [review]
4-initialize-sourceobject-when-compilation-fails.patch

This is way too js-eng-y for me...


>+++ b/js/src/vm/HelperThreads.h
>@@ -475,16 +475,19 @@ struct ParseTask
>     JS::OffThreadCompileCallback callback;
>     void* callbackData;
> 
>     // Holds the final script between the invocation of the callback and the
>     // point where FinishOffThreadScript is called, which will destroy the
>     // ParseTask.
>     JSScript* script;
> 
>+    // Holds the ScriptSourceObject generated for the script compilation.
>+    ScriptSourceObject* sourceObject;
>+

...especially since I have no idea what the lifetime management is here.
Any Foo* member variable looks scary to me, and I treat them as a security critical bug unless proved otherwise.

Comment 43

[Kannan Vijayan [:djvj]]

(In reply to Olli Pettay [:smaug] from comment #42)
> ...especially since I have no idea what the lifetime management is here.
> Any Foo* member variable looks scary to me, and I treat them as a security
> critical bug unless proved otherwise.

I realize it's in Jan's hands now, but just to explain that part: It's OK for the same reason the bare-pointer |JSScript* script| hanging off of that struct is ok.

The only thing that happens after compilation finishes is the ParseTask getting queued back up for finishing-up in the main thread.  The finishing-up is just merging the compartment.  No more allocations or any other activity should be happening on that compartment.

If it were, the |JSScript*| on the ParseTask would also be unsafe.

Comment 44

[Kannan Vijayan [:djvj]]

Comment on attachment 8639439 [details] [diff] [review]
4-initialize-sourceobject-when-compilation-fails.patch

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

::: js/src/jsscript.cpp
@@ +2782,5 @@
>      return global();
>  }
>  
>  void
> +JSScript::makeInvalid()

I just realized this method is unused.  Please ignore it.  It was for an earlier attempt at the fix that didn't work out.

Comment 45

[Jan de Mooij [:jandem]]

Comment on attachment 8639439 [details] [diff] [review]
4-initialize-sourceobject-when-compilation-fails.patch

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

Ugh, nice detective work tracking this down.

::: js/src/frontend/BytecodeCompiler.cpp
@@ +783,5 @@
>      compiler.setEnclosingStaticScope(enclosingStaticScope);
> +    JSScript* script = compiler.compileScript(scopeChain, evalCaller, staticLevel);
> +
> +    if (sourceObjectOut)
> +        *sourceObjectOut = compiler.sourceObjectPtr();

To avoid accidentally reintroducing the bug, this could use a comment explaining we need to return the sourceObject even if the compilation failed, because inner scripts may be referencing it.

::: js/src/jsscript.h
@@ +1838,5 @@
>  #ifdef DEBUG
>      uint32_t stepModeCount() { return hasDebugScript_ ? debugScript()->stepMode : 0; }
>  #endif
>  
> +    void makeInvalid();

(as you said, don't forget to remove this one)

::: js/src/vm/HelperThreads.cpp
@@ +232,5 @@
>  
>  bool
>  ParseTask::finish(JSContext* cx)
>  {
> +    if (sourceObject) {

Nit: it seems we could MOZ_ASSERT(sourceObject) ?

Comment 46

[Kannan Vijayan [:djvj]]

(In reply to Jan de Mooij [:jandem] from comment #45)
> ::: js/src/vm/HelperThreads.cpp
> @@ +232,5 @@
> >  
> >  bool
> >  ParseTask::finish(JSContext* cx)
> >  {
> > +    if (sourceObject) {
> 
> Nit: it seems we could MOZ_ASSERT(sourceObject) ?

It's possible for the compilation to fail early enough that the SSO itself is not created.  We need to allow for a null sourceObject after the compilation attempt.

Comment 47

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b88cb51c1f77
https://hg.mozilla.org/integration/mozilla-inbound/rev/95a7f1251dd4
https://hg.mozilla.org/integration/mozilla-inbound/rev/4264d30f4b03
https://hg.mozilla.org/integration/mozilla-inbound/rev/211b839cfcda
https://hg.mozilla.org/integration/mozilla-inbound/rev/08653d62e6c0

Comment 48

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for mochitest-chrome failures in https://hg.mozilla.org/integration/mozilla-inbound/rev/3d882ef61333

https://treeherder.mozilla.org/logviewer.html#?job_id=12222096&repo=mozilla-inbound

Comment 49

[Kannan Vijayan [:djvj]]

Trying to build b2g emulator to replicate this.  Running into build problems (the B2G build instructions don't work).  Installing ubuntu on a VM to try again and see if that works.

The B2G Emulator Mochitest chrome crash is a shutdown crash, in:

std::auto_ptr<std::basic_streambuf<char, std::char_traits<char> > >::~auto_ptr()

This destructor is called on one of the static global streambuf instances that stl uses to keep the underlying buffers for its cin/cout/cerr streams.  Something in the destructor ends up jumping to null (pc == 0x00000000 in crash).

I have no clue why this might be happening.  Investigating.

Comment 50

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to Kannan Vijayan [:djvj] from comment #33)
> Also, talos shows a significant perf IMPROVEMENT (yay) of 30% or more on V8
> on windows:
> https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-
> inbound&originalRevision=8a9734a9d97d&newProject=try&newRevision=bb349bc1f1a3

(In reply to Kannan Vijayan [:djvj] from comment #40)
> I tried to get a breakdown on which tests were doing better, but the
> perfherder breakdown page didn't load.  I was just looking for "no
> regressions", so I'm not too concerned about getting to the bottom of this
> just yet.

The subtest view loads now, but the scores make no sense! For one thing, why are Earley and Boyer, and Decrypt and Encrypt separate? Even ignoring those, why does RegExp have the highest score when it should have the lowest? It looks like Talos is displaying the *times*, but treating them as scores! I don't know how this could be, though, since the score at [1] does seem to be going up over time (as you'd expect from our gradually improving score).

[1] http://graphs.mozilla.org/graph.html#tests=[[230,131,35]]&sel=none&displayrange=365&datatype=geo

Comment 51

[Emanuel Hoogeveen [:ehoogeveen]]

Joel, could you shed some light on this? Which view is correct? It goes without saying that if the scores are actually times, then the large improvement is actually a large regression.

Comment 52

[Joel Maher ( :jmaher ) (UTC -8)]

ah, sorry for the confusion, and thanks for looking at the perf on your own!

V8- while this is a few years old, it still has a lot of useful data.  V8 has the 8 subtests and they are then summarized into the final v8 score.  This is done inside of Talos and uploaded to graph server, so the graph server score for v8 is the number that v8 intended to report.

Keep in mind that we have each of these subtests which are run (e.g. Earley, Boyer, RegExp) and we harvest the numbers from them.  In fact all of the raw replicates of them are posted to perfherder which we take all of the data and just report a geometric mean, not a benchmark score.  This is mentioned in bug 1184966.  We are fixing this and have already done 1 of 2 parts where talos now sends the computed benchmark scores for everything in the data blog to perfherder.  What remains is that perfherder needs to ingest the new data in the json blob so we report it properly.

Does that shed light and make sense?

Comment 53

[Emanuel Hoogeveen [:ehoogeveen]]

I think so, but this means that the perfherder results are currently wrong, correct? They are interpreting an increase in raw runtime as an increase in scores, and thus reporting an improvement?

Kannan, the affected tests are DeltaBlue, RayTrace and Richards, with RayTrace regressing particularly badly (if I'm interpreting this right). See e.g. [1] and [2] for the linux32 and linux64 results.

Comment 54

[Emanuel Hoogeveen [:ehoogeveen]]

Er, where [1] and [2] are as below.

[1] https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-inbound&originalRevision=8a9734a9d97d&newProject=try&newRevision=bb349bc1f1a3&originalSignature=f3cdc5746374e146c7cc7c679e89f6dc5931f68e&newSignature=f3cdc5746374e146c7cc7c679e89f6dc5931f68e
[2] https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-inbound&originalRevision=8a9734a9d97d&newProject=try&newRevision=bb349bc1f1a3&originalSignature=e81533b82bb54362bdb9a24b08da2d09b79c8956&newSignature=e81533b82bb54362bdb9a24b08da2d09b79c8956

Comment 55

[Joel Maher ( :jmaher ) (UTC -8)]

the suite summary in perfherder will be incorrect, but the subtests like RayTrace and Richards should be useful.  Right now graph server doesn't have an easy way to get those subtest results.

Comment 56

[Kannan Vijayan [:djvj]]

I want to go over what I've discovered so far about this bug:

1. No luck in reproducing locally yet.  I took a while to get a build of B2G ICS Emulator going.  Then took a while more to get mochitests running on it.  I'm still not sure if it's runnign fine because the screen is black due to a known bug in B2G that is not fixed yet.

2. I bisected my patchset.  The problem gets introduced at patch  3-off-thread-compile-blocking-scripts.patch  .  This patch is what "turns on" the OMTC (off-main-thread compilation) machinery.  Within the patch, the specific logic that directs scripts to off-thread parsing is what induces the crash.

3. Interestingly, there are 2 places where the patch directs previously synchronous script compilation to an off-main-thread compilation.  The first is when we find a preloaded (speculatively loaded) script in the preload cache.  The second is when we get a script through a regular network request.  If either one of these is commented out, the crash goes away.  It's only when both mechanisms are used that the crash shows up.

I'm requesting a try machine loan again and am going to try to replicate it there.

Here is a failing try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=213da61817be

(In reply to Kannan Vijayan [:djvj] from comment #49)
> Trying to build b2g emulator to replicate this.  Running into build problems
> (the B2G build instructions don't work).  Installing ubuntu on a VM to try
> again and see if that works.
> 
> The B2G Emulator Mochitest chrome crash is a shutdown crash, in:
> 
> std::auto_ptr<std::basic_streambuf<char, std::char_traits<char> >
> >::~auto_ptr()
> 
> This destructor is called on one of the static global streambuf instances
> that stl uses to keep the underlying buffers for its cin/cout/cerr streams. 
> Something in the destructor ends up jumping to null (pc == 0x00000000 in
> crash).
> 
> I have no clue why this might be happening.  Investigating.

Comment 57

[Kannan Vijayan [:djvj]]

In the interests of making this easier to track, I've landed all the lead-up infrastructure to off-main-thread parsing, but left off the actual usage of that parsing.

Comment 58

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7251438cac7
https://hg.mozilla.org/integration/mozilla-inbound/rev/5afb8edd05f7
https://hg.mozilla.org/integration/mozilla-inbound/rev/c467f1b2bc71
https://hg.mozilla.org/integration/mozilla-inbound/rev/49dbd2c38b9e

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e7251438cac7
https://hg.mozilla.org/mozilla-central/rev/5afb8edd05f7
https://hg.mozilla.org/mozilla-central/rev/c467f1b2bc71
https://hg.mozilla.org/mozilla-central/rev/49dbd2c38b9e

Comment 60

[Olli Pettay [:smaug][[e-mail]]]

Would it make sense to close this one and move the rest of the work to happen in a different bug.
That way patches from one bug would land all in one release.

Comment 61

[Kannan Vijayan [:djvj]]

Sure, I'm ok for that.  The infrastructure is in now.  It's just the actually-using-it part that's blocked on crash.

Comment 62

[Kannan Vijayan [:djvj]]

Resolving this, moving remaining work to bug 1084009.

Comment 63

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.