Closed Bug 993772 Opened 6 years ago Closed 6 years ago

Just use one compilation scope for Gecko

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
blocking-b2g 1.3T+
Tracking Status
b2g-v1.3T --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

(Whiteboard: [MemShrink:P2])

Attachments

(7 files, 1 obsolete file)

We have a plethora of compilation scopes, generally one per XULPrototypeDocument and one per XBL document. No code ever runs in these scopes, so I don't know of any reason why we can't just have one compilation scope (with a null principal) that hangs off of XPConnect.

This might save a sizable chunk of memory.
Blocks: 737012
Whiteboard: [MemShrink]
Until my patches in bug 990290, XBL compilation scopes would sometimes define things on their global. My patches there fix that. Setting the dep.
Depends on: 990290
On a simple test run (opening google.com, mozilla.com, and about:memory), this reduces the number of compartments from 259 to 225, a 13% reduction. Each of these globals was also cycle-collected, so this probably reduces our CC overhead as well. It also removes ~500 lines of very non-trivial code.

\o/
Woo! Do it again, do it again :)
Can we go to 1/20 of a compilation scope?  ;)
The principal of a script is determined by the compartment these days, so this
field is useless.
Attachment #8403735 - Flags: review?(mrbkap)
Attachment #8403738 - Flags: review?(mrbkap)
Attachment #8403742 - Flags: review?(mrbkap)
Comment on attachment 8403735 [details] [diff] [review]
Part 1 - Stop serializing principals along with scripts. v1

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

::: js/xpconnect/src/nsXPConnect.cpp
@@ +1361,5 @@
>      if (!script) {
>          RootedFunction fun(cx, JS_GetObjectFunction(functionObj));
>          script.set(JS_GetFunctionScript(cx, fun));
>      }
> + 

Added ws
Comment on attachment 8403736 [details] [diff] [review]
Part 2 - Introduce a singleton compilation scope. v1

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +3482,5 @@
> +{
> +    if (!mCompilationScope) {
> +        AutoSafeJSContext cx;
> +        SandboxOptions options;
> +        options.sandboxName.AssignASCII("XPConnect Compilation Compartment");

AssignLiteral? Or = NS_LITERAL_STRING()?

@@ +3503,2 @@
>      AutoSafeJSContext cx;
> +    if(mJunkScope) {

Space after if (twice)

::: js/xpconnect/src/xpcprivate.h
@@ +598,5 @@
>      nsTArray<xpcContextCallback> extraContextCallbacks;
>      nsRefPtr<WatchdogManager> mWatchdogManager;
>      JS::GCSliceCallback mPrevGCSliceCallback;
>      JSObject* mJunkScope;
> +    JSObject* mCompilationScope;

(Should these use some kind of magic GC type?)
Comment on attachment 8403740 [details] [diff] [review]
Part 5 - Switch to the singleton compilation scope for XUL prototype compilation. v1

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

::: content/xul/content/src/nsXULElement.cpp
@@ +2416,5 @@
>      if (NS_FAILED(rv)) return rv;
>  
>      // Calling fromMarkedLocation() is safe because we trace mScriptObject in
>      // TraceScriptObject() and because its value is never changed after it has
>      // been set.

(Ugh. Is this so hot?)
Blocks: 990729
Whiteboard: [MemShrink] → [MemShrink:P2]
Comment on attachment 8403735 [details] [diff] [review]
Part 1 - Stop serializing principals along with scripts. v1

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

::: js/xpconnect/src/nsXPConnect.cpp
@@ -1352,1 @@
>  static const uint8_t HAS_ORIGIN_PRINCIPALS_FLAG        = 2;

Add a comment that if someone wants to re-use the '1' value, they'll have to bump the XDR version number?
Attachment #8403735 - Flags: review?(mrbkap) → review+
Attachment #8403736 - Flags: review?(mrbkap) → review+
Attachment #8403737 - Flags: review?(mrbkap) → review+
Attachment #8403738 - Flags: review?(mrbkap) → review+
Attachment #8403740 - Flags: review?(mrbkap) → review+
Attachment #8403742 - Flags: review?(mrbkap) → review+
Bobby believes we could get memory wins on b2g also, so I'll test on tarako.
blocking-b2g: --- → 1.3T?
Green modulo rooting hazards and rebasing. Trying again:

https://tbpl.mozilla.org/?tree=Try&rev=9e4bea86df74
Duplicate of this bug: 737012
ni? Fabrice, wonder if you have further information after testing this out? thanks
Flags: needinfo?(fabrice)
Bobby, rebasing on 1.3t has a lot of conflicts. Can you do it properly?
Flags: needinfo?(fabrice) → needinfo?(bobbyholley)
Last we discussed this, you were going to test if it has a noticeable affect on a trunk b2g build, and then we'd decide whether to backport. Did you get any results there?
Flags: needinfo?(fabrice)
Flags: needinfo?(bobbyholley)
I tested on trunk with/without these patches, this gives us 160kB reduction of memory usage in the parent process. Bobby, can you rebase?
Flags: needinfo?(fabrice)
Attached file compilationglobal-tarako.tar.bz2 (obsolete) —
I generated these on a linux VM where I could build tarako.

It was a 2-hour backport, so I hope it does the trick. Can you give it a test on a tarako device and let me know how it looks? Measure js-main-runtime-compartments  to check what the reduction in compartments is.
Attachment #8410711 - Flags: feedback?(fabrice)
The last patch set was busted by a single assertion, due to bug 980180 not being on the tarako branch. I've fixed it, and the patches now pass desktop mochitests.
Attachment #8410711 - Attachment is obsolete: true
Attachment #8410711 - Flags: feedback?(fabrice)
Attachment #8410763 - Flags: feedback?(fabrice)
(In reply to Bobby Holley (:bholley) from comment #26)
> Created attachment 8410711 [details]
> compilationglobal-tarako.tar.bz2
> 
> I generated these on a linux VM where I could build tarako.
> 
> It was a 2-hour backport, so I hope it does the trick. Can you give it a
> test on a tarako device and let me know how it looks? Measure
> js-main-runtime-compartments  to check what the reduction in compartments is.

We end up with a reduction of two compartments in the parent process, no changes in the content process.
Attachment #8410763 - Flags: feedback?(fabrice) → feedback+
blocking-b2g: 1.3T? → 1.3T+
Looks like this causes mochitest to leak.  See bug 1018544.
(In reply to Doug Turner (:dougt) from comment #30)
> Looks like this causes mochitest to leak.  See bug 1018544.

To clarify, the backport to 1.3T causes mochitest to leak on linux desktop, which isn't known to be a supported configuration. Still worth investigating though.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.