Closed
Bug 993772
Opened 10 years ago
Closed 10 years ago
Just use one compilation scope for Gecko
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
Tracking | Status | |
---|---|---|
b2g-v1.3T | --- | fixed |
People
(Reporter: bholley, Assigned: bholley)
References
Details
(Whiteboard: [MemShrink:P2])
Attachments
(7 files, 1 obsolete file)
12.22 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
6.05 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
3.55 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
10.33 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
4.82 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
12.85 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
10.44 KB,
application/x-bzip2
|
fabrice
:
feedback+
|
Details |
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.
Updated•10 years ago
|
Whiteboard: [MemShrink]
Assignee | ||
Comment 1•10 years ago
|
||
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
Assignee | ||
Comment 2•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=428970a30e22
Assignee | ||
Comment 3•10 years ago
|
||
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/
Comment 4•10 years ago
|
||
Woo! Do it again, do it again :)
Comment 5•10 years ago
|
||
Can we go to 1/20 of a compilation scope? ;)
Assignee | ||
Comment 6•10 years ago
|
||
The principal of a script is determined by the compartment these days, so this field is useless.
Attachment #8403735 -
Flags: review?(mrbkap)
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8403736 -
Flags: review?(mrbkap)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8403737 -
Flags: review?(mrbkap)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8403738 -
Flags: review?(mrbkap)
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8403740 -
Flags: review?(mrbkap)
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8403742 -
Flags: review?(mrbkap)
Comment 12•10 years ago
|
||
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 13•10 years ago
|
||
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 14•10 years ago
|
||
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?)
Updated•10 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P2]
Comment 15•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8403736 -
Flags: review?(mrbkap) → review+
Updated•10 years ago
|
Attachment #8403737 -
Flags: review?(mrbkap) → review+
Updated•10 years ago
|
Attachment #8403738 -
Flags: review?(mrbkap) → review+
Updated•10 years ago
|
Attachment #8403740 -
Flags: review?(mrbkap) → review+
Updated•10 years ago
|
Attachment #8403742 -
Flags: review?(mrbkap) → review+
Comment 16•10 years ago
|
||
Bobby believes we could get memory wins on b2g also, so I'll test on tarako.
blocking-b2g: --- → 1.3T?
Assignee | ||
Comment 17•10 years ago
|
||
Final all-platform try push: https://tbpl.mozilla.org/?tree=Try&rev=107cdf474a1c
Assignee | ||
Comment 18•10 years ago
|
||
Green modulo rooting hazards and rebasing. Trying again: https://tbpl.mozilla.org/?tree=Try&rev=9e4bea86df74
Assignee | ||
Comment 19•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/64e9e6c601b7 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d8be02486b86 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/3c13321cab20 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/63818195fa63 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/10d5e3c5b0e4 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9dde6cd3739b
Comment 20•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/64e9e6c601b7 https://hg.mozilla.org/mozilla-central/rev/d8be02486b86 https://hg.mozilla.org/mozilla-central/rev/3c13321cab20 https://hg.mozilla.org/mozilla-central/rev/63818195fa63 https://hg.mozilla.org/mozilla-central/rev/10d5e3c5b0e4 https://hg.mozilla.org/mozilla-central/rev/9dde6cd3739b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment 22•10 years ago
|
||
ni? Fabrice, wonder if you have further information after testing this out? thanks
Flags: needinfo?(fabrice)
Comment 23•10 years ago
|
||
Bobby, rebasing on 1.3t has a lot of conflicts. Can you do it properly?
Flags: needinfo?(fabrice) → needinfo?(bobbyholley)
Assignee | ||
Comment 24•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(bobbyholley)
Comment 25•10 years ago
|
||
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)
Assignee | ||
Comment 26•10 years ago
|
||
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)
Assignee | ||
Comment 27•10 years ago
|
||
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)
Comment 28•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8410763 -
Flags: feedback?(fabrice) → feedback+
Updated•10 years ago
|
blocking-b2g: 1.3T? → 1.3T+
Comment 29•10 years ago
|
||
remote: https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/e04dc43cf5f9 remote: https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/53f27850e693 remote: https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/b28aa40c62b2 remote: https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/1adf691f94f9 remote: https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/4b60784f269f remote: https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/8329ca6bb655 remote: https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/080d169a8ddd
status-b2g-v1.3T:
--- → fixed
Comment 30•10 years ago
|
||
Looks like this causes mochitest to leak. See bug 1018544.
Assignee | ||
Comment 31•10 years ago
|
||
(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.
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•