1442737 - Remove the compilation scope

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement, P3)

Description

[Bobby Holley (:bholley)]

I don't think we have a really good reason to have a special global for compilation. Seems like we could just use one of the junk scopes.

I'd imagine this would save at least 15k per content process, possibly more.

Comment 1

[Kris Maglione [:kmag]]

At this point, we should probably just use the shared JSM global, and use that in place of the privileged junk scope too. That would also have the side-effect of removing script cloning and wrapper overhead as more things get moved to that global.

Comment 2

[Bobby Holley (:bholley)]

(In reply to Kris Maglione [:kmag] from comment #1)
> At this point, we should probably just use the shared JSM global, and use
> that in place of the privileged junk scope too. That would also have the
> side-effect of removing script cloning and wrapper overhead as more things
> get moved to that global.

+1. Should be a trivial patch.

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1442737: Use shared JSM global for compillation and privileged junk scopes.

Review commit: https://reviewboard.mozilla.org/r/252656/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/252656/

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8987408 [details]
Bug 1442737: Use shared JSM global for compillation and privileged junk scopes.

https://reviewboard.mozilla.org/r/252656/#review259292

r=me with those issues addressed.

::: js/xpconnect/src/XPCJSRuntime.cpp
(Diff revision 1)
>      mUnprivilegedJunkScope = js::UncheckedUnwrap(&v.toObject());
> -
> -    // Create the Privileged Junk Scope.
> -    SandboxOptions privilegedJunkScopeOptions;
> -    privilegedJunkScopeOptions.sandboxName.AssignLiteral("XPConnect Privileged Junk Compartment");
> -    privilegedJunkScopeOptions.invisibleToDebugger = true;

Hm, seems like we're losing debugger invisibility, right? Are we at risk of the same issues that arose in bug 1052089 comment 9?

::: js/xpconnect/src/XPCJSRuntime.cpp:3090
(Diff revision 1)
> -    sandbox->ReleaseWrapper(sandbox);
> -    mPrivilegedJunkScope = nullptr;
> -    sandbox = SandboxPrivate::GetPrivate(mCompilationScope);
> -    sandbox->ReleaseWrapper(sandbox);
> -    mCompilationScope = nullptr;
> +}
> +
> +JSObject*
> +XPCJSRuntime::LoaderGlobal()
> +{
> +    if (!mLoaderGlobal) {

I'm concerned about the laziness here, I generally prefer explicitly-ordered instantiation of singletons. So I'd rather we just have XPCJSRuntime initialization start up the component loader at the appropriate moment, and then grab the loader global then.

I won't insist if it seems hard, or if you're short on time. If you have time and it's easy to do that please fix it, otherwise at least add a comment saying we should move in that direction and describing any known roadblocks.

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8987408 [details]
Bug 1442737: Use shared JSM global for compillation and privileged junk scopes.

https://reviewboard.mozilla.org/r/252656/#review259292

> Hm, seems like we're losing debugger invisibility, right? Are we at risk of the same issues that arose in bug 1052089 comment 9?

I don't think so, since we're creating this scope early during startup already.

> I'm concerned about the laziness here, I generally prefer explicitly-ordered instantiation of singletons. So I'd rather we just have XPCJSRuntime initialization start up the component loader at the appropriate moment, and then grab the loader global then.
> 
> I won't insist if it seems hard, or if you're short on time. If you have time and it's easy to do that please fix it, otherwise at least add a comment saying we should move in that direction and describing any known roadblocks.

I tried that at first, but trying to initialize the loader when we create the scope objects causes crashes.

I guess I could make the scope creation logic static, have XPCJSRuntime create the LoaderGlobal itself, and have the component loader just call the getter. I'd rather do that as a follow-up, though. We're already dealing with lazy global creating for this scope as it is, and these changes don't make it happen any earlier or later.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Kris Maglione [:kmag] from comment #5)
> Comment on attachment 8987408 [details]
> Bug 1442737: Use shared JSM global for compillation and privileged junk
> scopes.
> 
> https://reviewboard.mozilla.org/r/252656/#review259292
> 
> > Hm, seems like we're losing debugger invisibility, right? Are we at risk of the same issues that arose in bug 1052089 comment 9?
> 
> I don't think so, since we're creating this scope early during startup
> already.

There's the newGlobal notifications, but also the new script notifications. I _think_ we only get those when scripts are executed rather than compiled, so we're probably fine for the compilation scope. In general, I'm guessing we'd see something break if this were going to cause problems, but hard to say. I wish I could remember what precise issues I ran into there...

> 
> > I'm concerned about the laziness here, I generally prefer explicitly-ordered instantiation of singletons. So I'd rather we just have XPCJSRuntime initialization start up the component loader at the appropriate moment, and then grab the loader global then.
> > 
> > I won't insist if it seems hard, or if you're short on time. If you have time and it's easy to do that please fix it, otherwise at least add a comment saying we should move in that direction and describing any known roadblocks.
> 
> I tried that at first, but trying to initialize the loader when we create
> the scope objects causes crashes.
> 
> I guess I could make the scope creation logic static, have XPCJSRuntime
> create the LoaderGlobal itself, and have the component loader just call the
> getter. I'd rather do that as a follow-up, though. We're already dealing
> with lazy global creating for this scope as it is, and these changes don't
> make it happen any earlier or later.

SGTM thanks.

Comment 7

[Kris Maglione [:kmag]]

(In reply to Bobby Holley (:bholley) from comment #6)
> (In reply to Kris Maglione [:kmag] from comment #5)
> > > Hm, seems like we're losing debugger invisibility, right? Are we at risk of the same issues that arose in bug 1052089 comment 9?
> > 
> > I don't think so, since we're creating this scope early during startup
> > already.
> 
> There's the newGlobal notifications, but also the new script notifications.
> I _think_ we only get those when scripts are executed rather than compiled,
> so we're probably fine for the compilation scope. In general, I'm guessing
> we'd see something break if this were going to cause problems, but hard to
> say. I wish I could remember what precise issues I ran into there...

Well we're compiling scripts for this global as early as possible already, too :) So I think we should be fine.

Comment 8

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8477472996e06d06a21d8e602e4a92d0ec130ea3
Bug 1442737: Use shared JSM global for compillation and privileged junk scopes. r=bholley

Comment 9

[Eliza Balazs [:ebalazs_]]

Backed out changeset 8477472996e0 (bug 1442737) for frequent mochitest failures e.g.: toolkit/components/alerts/test/test_principal.html

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8477472996e06d06a21d8e602e4a92d0ec130ea3&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-classifiedState=unclassified&filter-searchStr=linux%20x64%20asan%20mochitests%20with%20e10s%20test-linux64-asan%2Fopt-mochitest&selectedJob=184861316

Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c38b3264ab366874c4280038b5eb24ab4e3a9848&filter-classifiedState=unclassified&filter-searchStr=linux%20x64%20asan%20mochitests%20with%20e10s%20test-linux64-asan%2Fopt-mochitest

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=184861316&repo=mozilla-inbound&lineNumber=1637

Comment 10

[Kris Maglione [:kmag]]

These seem to be random ASAN-only shutdown hangs. Unfortunately, the test failures don't give me anything to go on, and so far I can't reproduce it locally...

Comment 11

[Kris Maglione [:kmag]]

OK, after having wasted a bunch of time on this, I've come to the conclusion that those hangs are almost certainly unrelated to this change. They happened relatively frequently before the change landed. They don't seem to happen any more frequently in the push where it landed. It looks like they *probably* do happen more frequently in the next push, but it's hard to say. All I can say is that the pushes where there dozens of retriggers have more of these failures.

Comment 12

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1db50f691f000a0261a57d39da75675592ada9c
Bug 1442737: Use shared JSM global for compilation and privileged junk scopes. r=bholley

Comment 13

[Cristina Coroiu [:ccoroiu]]

Backed out changeset a1db50f691f0 (bug 1442737) for frequent mochitest failures on e.g: dom/workers/test/browser_fileURL.js

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/78ddf47da425f6f6d14032017d783a80c8e0cadc

Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a1db50f691f000a0261a57d39da75675592ada9c

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=185084620&repo=mozilla-inbound&lineNumber=3326

Comment 14

[Kris Maglione [:kmag]]

As far as I can tell, this is just a bug in ASan. Once every thousand or so shutdowns, it freezes. It happened before this patch, too, just less frequently.

I managed to reproduce it on a loaner once after looping for several hours, and then couldn't attach to the frozen process because of yama settings, and couldn't kill -11 it for reasons I cannot figure out without being able to attach to it.

So I'm going to try to update the test runner to ignore shutdown hangs in ASan.

That said, I don't think a shutdown hang every several thousand test sessions on ASan is a good reason to backout a patch that saves us 100-200K per content process. Perhaps if it had continued for several days without a fix.

Comment 15

[Kris Maglione [:kmag]]

I wrote the patch to ignore the ASan timeouts, but according to those try runs, the issue seems to have gone away on its own, anyway. So I'm going to land again, but leave those patches up, since at least the first one fixes other issues that would have made this much easier to diagnose.

Comment 16

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/72189b6ec0f192d7c26abaa8d449af3b94a11327
Bug 1442737: Use shared JSM global for compilation and privileged junk scopes. r=bholley

Comment 17

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/72189b6ec0f1

Comment 18

[Ionuț Goldan [:igoldan]]

AWSY perf wins!

== Change summary for alert #14050 (as of Tue, 26 Jun 2018 18:34:49 GMT) ==

Improvements:

  3%  Base Content JS linux64 opt stylo               7,678,729.85 -> 7,437,400.67

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=14050