Closed Bug 1442737 Opened 4 years ago Closed 3 years ago

Remove the compilation scope

Categories

(Core :: XPConnect, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: bholley, Assigned: kmag)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Whiteboard: [overhead:100k])

Attachments

(1 file)

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.
Priority: -- → P3
Whiteboard: [overhead:15k]
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.
(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 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.
Attachment #8987408 - Flags: review?(bobbyholley) → review+
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.
Blocks: 1470969
Assignee: nobody → kmaglione+bmo
(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.
(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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8477472996e06d06a21d8e602e4a92d0ec130ea3
Bug 1442737: Use shared JSM global for compillation and privileged junk scopes. r=bholley
Whiteboard: [overhead:15k] → [overhead:100k]
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...
Flags: needinfo?(kmaglione+bmo)
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1db50f691f000a0261a57d39da75675592ada9c
Bug 1442737: Use shared JSM global for compilation and privileged junk scopes. r=bholley
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.
Flags: needinfo?(kmaglione+bmo)
Depends on: 1471724
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/72189b6ec0f192d7c26abaa8d449af3b94a11327
Bug 1442737: Use shared JSM global for compilation and privileged junk scopes. r=bholley
https://hg.mozilla.org/mozilla-central/rev/72189b6ec0f1
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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
You need to log in before you can comment on or make changes to this bug.