Closed
Bug 755583
Opened 12 years ago
Closed 12 years ago
Streamline JS compartment reporters
Categories
(Toolkit :: about:memory, defect)
Toolkit
about:memory
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
(Whiteboard: [MemShrink:P1])
Attachments
(6 files)
17.41 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
1.74 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
937 bytes,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
16.93 KB,
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
22.07 KB,
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
8.27 KB,
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
Compartment-per-global means we have many more compartments than before. And there's scope for distinguishing many of the ones that are currently merged in about:memory (e.g. see bug 754771). But the generation of about:memory itself consumes memory, and it consumes more as the amount of text in about:memory increases. In other words, about:memory perturbs what it is measuring. More information is a good thing, so I'd like to reduce this perturbation, either by making about:memory's generation more efficient, or reducing the number of things reported per compartment, or both. But to really make a difference, we'll require better measurement of the perturbation, something we currently lack.
Assignee | ||
Comment 1•12 years ago
|
||
> But to really make a difference, we'll require better measurement of the > perturbation, something we currently lack. Bug 720595 does this.
Depends on: 720595
Assignee | ||
Comment 2•12 years ago
|
||
We have 30+ measurements per compartment. With CPG landing, and bug 754771 promising to distinguish the many System Principle compartments (i.e. so they won't be aggregated in about:memory), about:memory is bloated with lots of small, uninteresting measurements. This also perturbs what's being measured. This patch modifies the js multi-reporters to merge all measurements for a single compartment that are below 4KB into a "sundries" entry. Actually, there is a "gc-heap/sundries" entry (which is KIND_NONHEAP) and a "other-sundries" (which is KIND_HEAP); the two kinds must be kept separate otherwise bad things happen. With patch two from bug 754771 applied, this reduces the size of the verbose "explicit" tree at start-up from ~3000 lines to ~1500 lines, and roughly halves memory consumed during generation of about:memory. Here's an example of a very small compartment after this change: │ ├──────22,304 B (00.03%) -- compartment([System Principal], chrome://global/content/bindings/splitter.xml) │ │ ├──20,480 B (00.03%) -- gc-heap │ │ │ ├──19,688 B (00.03%) -- arena │ │ │ │ └──19,688 B (00.03%) ── unused │ │ │ └─────792 B (00.00%) ── sundries │ │ └───1,824 B (00.00%) ── other-sundries And a mid-sized compartment: │ ├──────89,736 B (00.13%) -- compartment([System Principal], resource://services-common/utils.js) │ │ ├──69,632 B (00.10%) -- gc-heap │ │ │ ├──38,264 B (00.06%) -- arena │ │ │ │ └──38,264 B (00.06%) ── unused │ │ │ ├──13,592 B (00.02%) ── sundries │ │ │ ├───9,216 B (00.01%) -- objects │ │ │ │ └──9,216 B (00.01%) ── function │ │ │ └───8,560 B (00.01%) -- shapes │ │ │ └──8,560 B (00.01%) ── tree │ │ ├───7,496 B (00.01%) ── script-data │ │ ├───6,656 B (00.01%) -- shapes-extra │ │ │ └──6,656 B (00.01%) ── compartment-tables │ │ └───5,952 B (00.01%) ── other-sundries Sub-trees with a single child are now common; I'm going to investigate whether I can compact them next.
Attachment #624647 -
Flags: review?(luke)
Assignee | ||
Comment 3•12 years ago
|
||
RegExp.test is lighterweight than String.match. This patch was from Nils, r=me.
Attachment #624648 -
Flags: review+
Assignee | ||
Comment 4•12 years ago
|
||
This avoids creating a JS-side Node wrapper. This reduces cum-GC-bytes by about 10%. Patch was from Nils, r=me.
Attachment #624649 -
Flags: review+
Assignee | ||
Comment 5•12 years ago
|
||
This patch splits the "smaps/" tree into four: "resident", "pss", "vsize", and "swap". I like it better this way, but more importantly, it makes the next patch much simpler.
Attachment #624651 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 6•12 years ago
|
||
This patch avoids unnecessarily showing of tree nesting. E.g. this: ├────499.00 MB (49.90%) -- a │ └──499.00 MB (49.90%) -- b │ └──499.00 MB (49.90%) ── c [3] becomes this: ├────499.00 MB (49.90%) ── a/b/c [3] This reduces the number of entries in about:memory by about 20--25% at start-up with Nils patch from bug 754771 applied.
Attachment #624653 -
Flags: review?(justin.lebar+bug)
Comment 7•12 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #6) > This reduces the number of entries in about:memory by about 20--25% at > start-up with Nils patch from bug 754771 applied. Nice! It also makes about:memory easier to read. ;)
Assignee | ||
Comment 8•12 years ago
|
||
Currently for nodes that have children we always generate both a '++' and a '--', and then we hide the one that's not necessary, and toggle visiblity of both when it's clicked. This patch changes things so we generate only '++' or '--', and we change the text when it's toggled. This doesn't affect the cumulative-GC-bytes noticeably, but it's one less <span> per line so it's gotta reduce memory consumption on the C++ side. The code is also a bit nicer now, esp. since I could get rid of the kid state constants.
Attachment #624662 -
Flags: review?(justin.lebar+bug)
Updated•12 years ago
|
Attachment #624647 -
Flags: review?(luke) → review+
Comment 9•12 years ago
|
||
Comment on attachment 624651 [details] [diff] [review] Patch 4: split up the smaps/ tree >-const kMapTreePaths = >- ['smaps/resident', 'smaps/pss', 'smaps/vsize', 'smaps/swap']; >+const kSmapsTreePrefixes = ['resident/', 'pss/', 'vsize/', 'swap/']; >+ >+function isSmapsTreePrefix(s) "resident/foo/bar" isn't really an "smaps tree prefix". "isSmapsPath" or something? >@@ -650,46 +652,35 @@ TreeNode.compare = function(a, b) { > }; > > /** > * From a table of Reports, builds a tree that mirrors the tree structure that > * will be shown as output. > * > * @param aReports > * The table of Reports, indexed by _unsafePath. >- * @param aTreeName >- * The name of the tree being built. >+ * @param aTreePrefix >+ * The prefix (name) of the tree being built. Must have '/' on the end. > * @return The built tree. > */ >-function buildTree(aReports, aTreeName) >+function buildTree(aReports, aTreePrefix) > { >- // We want to process all reports that begin with |aTreeName|. First we >+ assert(aTreePrefix.indexOf('/') == aTreePrefix.length - 1); I don't particularly care, but it looks like most assertions in this file have descriptions.
Attachment #624651 -
Flags: review?(justin.lebar+bug) → review+
Comment 10•12 years ago
|
||
Comment on attachment 624653 [details] [diff] [review] Patch 5: merge each single child node into its parent >+ function fillInNonLeafNodes(aT, aCannotMerge) > { > if (aT._kids.length === 0) { > // Leaf node. Has already been filled in. > assert(aT._kind !== undefined, "aT._kind is undefined for leaf node"); >+ >+ } else if (aT._kids.length === 1 && !aCannotMerge) { >+ // Non-leaf node with one child. Merge the child with the node to avoid >+ // redundant entries. >+ assert(aT._kind === undefined, "aT._kind is defined for non-leaf node"); >+ let kid = aT._kids[0]; >+ let kidBytes = fillInNonLeafNodes(kid); >+ aT._unsafeName += '/' + kid._unsafeName; >+ aT._kids = kid._kids; >+ aT._amount = kid._amount; >+ aT._description = kid._description; >+ aT._kind = kid._kind; >+ if (kid._nMerged) { >+ aT._nMerged = kid._nMerged >+ } >+ > } else { /me grumbles about cuddle-else with a linebreak before the else. uncuddle-else with no extra linebreak is clearly superior. Anyway, nice patch. :)
Attachment #624653 -
Flags: review?(justin.lebar+bug) → review+
Updated•12 years ago
|
Attachment #624662 -
Flags: review?(justin.lebar+bug) → review+
Assignee | ||
Comment 11•12 years ago
|
||
Patch 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/7e12d6d329af
Whiteboard: [MemShrink:P1] → [MemShrink:P1][leave open]
Target Milestone: --- → mozilla15
Assignee | ||
Comment 12•12 years ago
|
||
> /me grumbles about cuddle-else with a linebreak before the else.
Don't look at the rest of the file, then! :P
Assignee | ||
Comment 13•12 years ago
|
||
Patches 2--6: https://hg.mozilla.org/integration/mozilla-inbound/rev/3c851d643ea5 https://hg.mozilla.org/integration/mozilla-inbound/rev/d74787facbf4 https://hg.mozilla.org/integration/mozilla-inbound/rev/7c0e76bd2c8d https://hg.mozilla.org/integration/mozilla-inbound/rev/95da608768e5 https://hg.mozilla.org/integration/mozilla-inbound/rev/d6ce3744f217
Whiteboard: [MemShrink:P1][leave open] → [MemShrink:P1]
Comment 14•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7e12d6d329af https://hg.mozilla.org/mozilla-central/rev/3c851d643ea5 https://hg.mozilla.org/mozilla-central/rev/d74787facbf4 https://hg.mozilla.org/mozilla-central/rev/7c0e76bd2c8d https://hg.mozilla.org/mozilla-central/rev/95da608768e5 https://hg.mozilla.org/mozilla-central/rev/d6ce3744f217
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•