Closed
Bug 1442737
Opened 7 years ago
Closed 7 years ago
Remove the compilation scope
Categories
(Core :: XPConnect, enhancement, P3)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: bholley, Assigned: kmag)
References
(Blocks 1 open bug)
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.
Updated•7 years ago
|
Priority: -- → P3
Updated•7 years ago
|
Whiteboard: [overhead:15k]
Assignee | ||
Comment 1•7 years ago
|
||
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.
Reporter | ||
Comment 2•7 years ago
|
||
(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 hidden (mozreview-request) |
Reporter | ||
Comment 4•7 years ago
|
||
mozreview-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
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+
Assignee | ||
Comment 5•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → kmaglione+bmo
Reporter | ||
Comment 6•7 years ago
|
||
(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.
Assignee | ||
Comment 7•7 years ago
|
||
(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.
Assignee | ||
Comment 8•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8477472996e06d06a21d8e602e4a92d0ec130ea3
Bug 1442737: Use shared JSM global for compillation and privileged junk scopes. r=bholley
Assignee | ||
Updated•7 years ago
|
Whiteboard: [overhead:15k] → [overhead:100k]
Comment 9•7 years ago
|
||
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
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 10•7 years ago
|
||
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)
Assignee | ||
Comment 11•7 years ago
|
||
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.
Assignee | ||
Comment 12•7 years ago
|
||
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•7 years ago
|
||
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
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 14•7 years ago
|
||
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)
Assignee | ||
Comment 15•7 years ago
|
||
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.
Assignee | ||
Comment 16•7 years ago
|
||
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•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment 18•7 years ago
|
||
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.
Description
•