Closed
Bug 757299
Opened 12 years ago
Closed 12 years ago
Streamline about:memory yet again
Categories
(Toolkit :: about:memory, defect)
Toolkit
about:memory
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: n.nethercote, Assigned: n.nethercote)
Details
Attachments
(10 files)
7.29 KB,
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
1.01 KB,
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
1.07 KB,
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
4.67 KB,
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
21.86 KB,
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
4.95 KB,
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
12.50 KB,
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
1.59 KB,
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
8.21 KB,
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
2.75 KB,
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
Following on from 755583. Here are 10 patches, most of which reduce about:memory's memory consumption, and a couple which are just code clean-ups. The followings stats show the memory reductions; "combined" is the most important one. cum-GC string object heap-unc combined original 10.5 4.7 7.9 12.2 24.7 optional-kids 10.1 4.7 7.5 11.9 24.1 better-flipBack 9.6 4.7 7.0 11.9 23.5 "below-this-one" 9.6 4.5 7.0 11.9 23.4 merge-newline 9.5 4.7 6.8 11.5 23.0 rm-Report-objects 8.2 4.8 5.3 11.4 21.5 better-path-concat 7.9 4.7 5.4 11.4 21.5 rm-_kind 7.9 4.7 5.4 11.4 21.5 tweak-perc 7.8 4.4 5.6 11.4 21.4 better-treeLine 8.0 5.6 3.9 11.4 20.8 rm-kind-from-desc 8.0 4.7 3.9 11.4 20.0
Assignee | ||
Comment 1•12 years ago
|
||
This patch removes the empty |kids| array from leaf TreeNodes. It also slightly improves the merging of single child nodes into their parent.
Attachment #625835 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 2•12 years ago
|
||
This patch avoids creating a new string in the common case where the given string doesn't contain a '\'.
Attachment #625836 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 3•12 years ago
|
||
This makes the description on non-leaf nodes more generic. No loss, IMO.
Attachment #625837 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 4•12 years ago
|
||
This patch puts the newline at the end of each line in the last <span> on the line, instead of in a separate text node. Saves one text node per line.
Attachment #625838 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 5•12 years ago
|
||
Currently we (1) create a Report object for each memory report, (2) build the tree from the Report objects, and (3) build the DOM from the tree. This patch merges steps (1) and (2), so Reporter objects are no longer necessary. It's a 40 line reduction in code. And it saves jlebar from having to worry whether I'm using "report" or "Report" appropriately in comments :) (BTW, merging step (1+2) with step (3) is theoretically possible but would be awful, so I have no intention of doing it.)
Attachment #625839 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 6•12 years ago
|
||
This makes the creation of paths from the tree a bit nicer. Not really a memory win, but it will make it easier to remove the safe/unsafe path distinction later.
Attachment #625840 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 7•12 years ago
|
||
This removes the TreeNode._kids field. It had two uses: - For adding "(Heap)" or "(Non-heap)" to descriptions. This is now done right at the start. - For measuring HEAP usage in the explicit tree. This is now done earlier. Again, not a memory win, but I like that it allows some assertions to be removed.
Attachment #625841 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 8•12 years ago
|
||
This is a slightly more efficient way to construct the percentage strings.
Attachment #625842 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 9•12 years ago
|
||
This is a more efficient way to generate the treelines. Easier to understand, too.
Attachment #625845 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 10•12 years ago
|
||
This removes the prepending of "(Heap)" or "(Non-heap)" from explicit tree descriptions. I don't think it adds much; if it's really useful in certain descriptions we could put it in the description itself.
Attachment #625847 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Updated•12 years ago
|
Attachment #625841 -
Attachment description: Patch 7: rm-_kids → Patch 7: rm-_kind
Assignee | ||
Comment 11•12 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #5) > Created attachment 625839 [details] [diff] [review] > Patch 5: remove-Report-objects > > This patch merges steps (1) and (2), so Reporter objects are no longer > necessary. I forgot to say: like patch 6, this will make removing the safe/unsafe distinction easier.
Comment 12•12 years ago
|
||
Comment on attachment 625835 [details] [diff] [review] Patch 1: optional-kids I've seen JS strict mode warnings about "referencing undefined property", but now I can't re-create them. It may be preferable to do |if ('kids' in a)| instead of |if (a.kids === undefined)|. But we should also be consistent; I'd be OK with picking either of those (or simply |if (a.kids)|), so long as whatever we pick doesn't warn. > TreeNode.prototype = { > findKid: function(aUnsafeName) { >- for (let i = 0; i < this._kids.length; i++) { >- if (this._kids[i]._unsafeName === aUnsafeName) { >- return this._kids[i]; >+ if (this._kids) { >+ for (let i = 0; i < this._kids.length; i++) { >+ if (this._kids[i]._unsafeName === aUnsafeName) { >+ return this._kids[i]; >+ } > } > } > return undefined; > }, > > toString: function() { > return formatBytes(this._amount); > } >@@ -700,16 +702,19 @@ function buildTree(aReports, aTreePrefix > let u = t; > for (let i = 0; i < unsafeNames.length; i++) { > let unsafeName = unsafeNames[i]; > let uMatch = u.findKid(unsafeName); > if (uMatch) { > u = uMatch; > } else { > let v = new TreeNode(unsafeName); >+ if (!u._kids) { >+ u._kids = []; >+ } > u._kids.push(v); > u = v; > } > } > // Fill in extra details in the leaf node from the Report object. > u._amount = r._amount; > u._description = r._description; > u._kind = r._kind; >@@ -730,34 +735,41 @@ function buildTree(aReports, aTreePrefix > > // Using falseRoot makes the above code simpler. Now discard it, leaving > // aTreePrefix at the root. > t = t._kids[0]; > > // Next, fill in the remaining properties bottom-up. > function fillInNonLeafNodes(aT, aCannotMerge) > { >- if (aT._kids.length === 0) { >+ if (!aT._kids) { > // 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; >+ if (kid._kids !== undefined) { >+ aT._kids = kid._kids; >+ } else { >+ delete aT._kids; >+ } > aT._amount = kid._amount; > aT._description = kid._description; >- aT._kind = kid._kind; >- if (kid._nMerged) { >+ if (kid._kind !== undefined) { >+ aT._kind = kid._kind; >+ } >+ if (kid._nMerged !== undefined) { This one needs to still be !== undefined, so I guess the other ones are OK.
Attachment #625835 -
Flags: review?(justin.lebar+bug) → review+
Updated•12 years ago
|
Attachment #625836 -
Flags: review?(justin.lebar+bug) → review+
Updated•12 years ago
|
Attachment #625837 -
Flags: review?(justin.lebar+bug) → review+
Comment 13•12 years ago
|
||
Comment on attachment 625838 [details] [diff] [review] Patch 4: merge-newline >-function appendMrNameSpan(aP, aKind, aSep, aDescription, aUnsafeName, >+function appendMrNameSpan(aP, aKind, aDescription, aUnsafeName, > aIsInvalid, aNMerged) > { >- appendElementWithText(aP, "span", "mrSep", aSep); >- >- let nameSpan = appendElementWithText(aP, "span", "mrName", >- flipBackslashes(aUnsafeName)); >+ let safeName = flipBackslashes(aUnsafeName); >+ if (!aIsInvalid && !aNMerged) { >+ safeName += "\n"; >+ } >+ let nameSpan = appendElementWithText(aP, "span", "mrName", safeName); > nameSpan.title = kindToString(aKind) + aDescription; > > if (aIsInvalid) { >- let noteSpan = appendElementWithText(aP, "span", "mrNote", " [?!]"); >+ let noteText = " [?!]"; >+ if (!aNMerged) { >+ noteText += "\n"; >+ } Save a string op here by doing let noteText = aNMerged ? " [?!]\n" : " [?!]"; ? Although I guess it doesn't matter because we don't hit this often.
Attachment #625838 -
Flags: review?(justin.lebar+bug) → review+
Assignee | ||
Comment 14•12 years ago
|
||
> It may be preferable to do |if ('kids' in a)| instead of |if (a.kids === undefined)|. That is nicer. But there are lots of places where I test for |undefined|; I'll rip them out in a final patch. > >+ if (kid._nMerged !== undefined) { > > This one needs to still be !== undefined Nope; |"_nMerged" in kid| would be fine.
Comment 15•12 years ago
|
||
> > This one needs to still be !== undefined
>
> Nope; |"_nMerged" in kid| would be fine.
Yeah, I'm not sure why I thought otherwise. I think I may have thought that _nMerged could be 0, but it looks like you avoid that.
Comment 16•12 years ago
|
||
Comment on attachment 625839 [details] [diff] [review] Patch 5: remove-Report-objects Cool. >diff --git a/toolkit/components/aboutmemory/content/aboutMemory.js b/toolkit/components/aboutmemory/content/aboutMemory.js >--- a/toolkit/components/aboutmemory/content/aboutMemory.js >+++ b/toolkit/components/aboutmemory/content/aboutMemory.js >@@ -459,24 +459,28 @@ function updateAboutMemory() > > // First, clear the page contents. Necessary because updateAboutMemory() > // might be called more than once due to the "child-memory-reporter-update" > // observer. > let body = clearBody(); > > // Generate output for one process at a time. Always start with the > // Main process. >- let reportsByProcess = getReportsByProcess(mgr); >+ let treesByProcess = {}, othersByProcess = {}; >+ getTreesAndOthersByProcess(mgr, treesByProcess, othersByProcess); >+ > let hasMozMallocUsableSize = mgr.hasMozMallocUsableSize; >- appendProcessReportsElements(body, "Main", reportsByProcess["Main"], >- hasMozMallocUsableSize); >- for (let process in reportsByProcess) { I feel like the comment should go right here. >+ appendProcessAboutMemoryElements(body, "Main", treesByProcess["Main"], >+ othersByProcess["Main"], >+ hasMozMallocUsableSize); >+ for (let process in treesByProcess) { > if (process !== "Main") { >- appendProcessReportsElements(body, process, reportsByProcess[process], >- hasMozMallocUsableSize); >+ appendProcessAboutMemoryElements(body, process, treesByProcess[process], >+ othersByProcess[process], >+ hasMozMallocUsableSize); > } > } > function handleReport(aProcess, aUnsafePath, aKind, aUnits, aAmount, > aDescription) > { > ... >+ if (aUnsafePath.indexOf('/') !== -1) { >+ // Tree report. Get the tree for the process, creating it if necessary. >+ // All the trees for each process ("explicit", "vsize", etc") are stored etc" >-function buildTree(aReports, aTreePrefix) >+function fillInTree(aTreeOfTrees, aTreePrefix) > ... >+ // There should always be an "explicit/" tree. But smaps trees may not be >+ // present; if that happens, bail. >+ let t = aTreeOfTrees.findKid(aTreePrefix.replace(/\//g, '')); >+ if (!t) { >+ assert(aTreePrefix !== 'explicit/', "missing explicit tree"); > return null; > } We're bailing if there's no explicit tree, but that's not what the comment says. Sorry for the wait, Nick.
Attachment #625839 -
Flags: review?(justin.lebar+bug) → review+
Updated•12 years ago
|
Attachment #625840 -
Flags: review?(justin.lebar+bug) → review+
Comment 17•12 years ago
|
||
Comment on attachment 625841 [details] [diff] [review] Patch 7: rm-_kind >@@ -635,18 +642,30 @@ function getTreesAndOthersByProcess(aMgr > > if (u._amount) { > // Duplicate! Sum the values and mark it as a dup. > u._amount += aAmount; > u._nMerged = u._nMerged ? u._nMerged + 1 : 2; > } else { > // New leaf node. Fill in extra details node from the report. > u._amount = aAmount; >- u._description = aDescription; >- u._kind = aKind; >+ if (unsafeNames[0] === "explicit") { >+ u._description = kindToString(aKind) + aDescription; >+ } else { >+ // We don't want to show '(Non-heap)' on an smaps tree because >+ // the whole tree is non-heap. >+ u._description = aDescription; >+ } >+ } >+ >+ if (unsafeNames[0] === "explicit" && aKind == KIND_HEAP) { >+ if (!aHeapTotalByProcess[process]) { >+ aHeapTotalByProcess[process] = 0; >+ } >+ aHeapTotalByProcess[process] += aAmount; > } Can we write a JS equivalent of Python's defaultdict yet? http://docs.python.org/library/collections.html#collections.defaultdict >-function fixUpExplicitTree(aT, aOthers) >+function fixUpExplicitTree(aT, aOthers, aHeapTotal) > { > // A special case: compute the derived "heap-unclassified" value. Don't > // mark "heap-allocated" when we get its size because we want it to appear > // in the "Other Measurements" list. > let heapAllocatedReport = aOthers["heap-allocated"]; > if (heapAllocatedReport === undefined) > return false; > > let heapAllocatedBytes = heapAllocatedReport._amount; > let heapUnclassifiedT = new TreeNode("heap-unclassified"); >+ heapUnclassifiedT._amount = heapAllocatedBytes - aHeapTotal; > gHeapUnc = heapUnclassifiedT._amount; >+ heapUnclassifiedT._description = kindToString(KIND_HEAP) + > "Memory not classified by a more specific reporter. This includes " + > "slop bytes due to internal fragmentation in the heap allocator " + > "(caused when the allocator rounds up request sizes)."; > aT._kids.push(heapUnclassifiedT); > aT._amount += heapUnclassifiedT._amount; > return true; > } Now that I can easily see this whole function, it occurs to me that we're not really "fixing up" anything here anymore; we're just adding the heap-unclassified node.
Attachment #625841 -
Flags: review?(justin.lebar+bug) → review+
Updated•12 years ago
|
Attachment #625842 -
Flags: review?(justin.lebar+bug) → review+
Comment 18•12 years ago
|
||
Comment on attachment 625845 [details] [diff] [review] Patch 9: better-treelines >-const kHorizontal = "\u2500", >- kVertical = "\u2502", >- kUpAndRight = "\u2514", >- kVerticalAndRight = "\u251c", >- kNoKidsSep = " \u2500\u2500 ", >- kHideKidsSep = " ++ ", >- kShowKidsSep = " -- "; >+const kHorizontal = "\u2500", >+ kVertical = "\u2502", >+ kUpAndRight = "\u2514", >+ kUpAndRight_Right_Right = "\u2514\u2500\u2500", >+ kVerticalAndRight = "\u251c", >+ kVerticalAndRight_Right_Right = "\u251c\u2500\u2500", >+ kVerticalAndSpace_Space_Space = "\u2502 ", >+ kNoKidsSep = " \u2500\u2500 ", >+ kHideKidsSep = " ++ ", >+ kShowKidsSep = " -- "; I think you're forgetting kUpUpDownDownLeftRightLeftRightBA. >@@ -1278,52 +1281,50 @@ function appendTreeElements(aPOuter, aT, > * Appends the elements for a particular tree, without a heading. > * > * @param aP > * The parent DOM node. > * @param aUnsafeNames > * An array of the names forming the path to aT. > * @param aT > * The tree. >- * @param aBaseIndentText >- * The base text of the indent, which may be augmented within the >- * function. >- * @param aIndentGuide >- * Records what indentation is required for this tree. It has one >- * entry per level of indentation. For each entry, ._isLastKid >- * records whether the node in question is the last child, and >- * ._depth records how many chars of indentation are required. >+ * @param aTreelineText1 >+ * The first part of the treeline for this entry and this entry's >+ * children. >+ * @param aTreelineText2a >+ * The second part of the treeline for this entry. >+ * @param aTreelineForChildrenText2 >+ * The second part of the treeline for this entry's children. Wrong param name. > * @param aParentStringLength > * The length of the formatted byte count of the top node in the tree. > */ >- function appendTreeElements2(aP, aUnsafeNames, aT, aIndentGuide, >- aBaseIndentText, aParentStringLength) >+ function appendTreeElements2(aP, aUnsafeNames, aT, aTreelineText1, >+ aTreelineText2a, aTreelineText2b, >+ aParentStringLength) > { >- function repeatStr(aA, aC, aN) >+ function appendN(aS, aC, aN) > { > for (let i = 0; i < aN; i++) { >- aA.push(aC); >+ aS += aC; > } >+ return aS; > } It might be a premature optimization, but I imagine this could use less memory. (e.g. we could memoize the string-to-append.)
Attachment #625845 -
Flags: review?(justin.lebar+bug) → review+
Comment 19•12 years ago
|
||
Comment on attachment 625847 [details] [diff] [review] Patch 10: rm-kind-from-desc I agree that "(Heap)" and "(Non-heap)" aren't adding much.
Attachment #625847 -
Flags: review?(justin.lebar+bug) → review+
Assignee | ||
Comment 20•12 years ago
|
||
> Can we write a JS equivalent of Python's defaultdict yet? > http://docs.python.org/library/collections.html#collections.defaultdict Assuming I've understood your point correctly... ECMAScript 6 will have a new "Map" type, but it's only just been implemented in SpiderMonkey and subject to change, so I don't want to use it yet: > >- function appendTreeElements2(aP, aUnsafeNames, aT, aIndentGuide, > >- aBaseIndentText, aParentStringLength) > >+ function appendTreeElements2(aP, aUnsafeNames, aT, aTreelineText1, > >+ aTreelineText2a, aTreelineText2b, > >+ aParentStringLength) > > { > >- function repeatStr(aA, aC, aN) > >+ function appendN(aS, aC, aN) > > { > > for (let i = 0; i < aN; i++) { > >- aA.push(aC); > >+ aS += aC; > > } > >+ return aS; > > } > > It might be a premature optimization, but I imagine this could use less > memory. > (e.g. we could memoize the string-to-append.) I measured, the two versions were basically the same.
Comment 21•12 years ago
|
||
> ECMAScript 6 will have a new "Map" type,
From a brief perusal, I don't think Map is what I mean here. The key feature of Map seems to be that you can use any object as a key.
What I mean is a normal dictionary with the caveat that when you do
dict["foo"]
and "foo" is not in the dictionary, instead of getting undefined, you get some value you specified when you created the map.
But anyway, it's not a specific comment to your patches; we don't need to change anything here. If I'm feeling ambitious, I could change it as a followup.
Assignee | ||
Comment 22•12 years ago
|
||
> What I mean is a normal dictionary with the caveat that when you do
>
> dict["foo"]
>
> and "foo" is not in the dictionary, instead of getting undefined, you get
> some value you specified when you created the map.
Oh, so that's what the "default" in "defaultdict" means. Yes, I don't think JS has that.
Assignee | ||
Comment 23•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7daeedb89d5 https://hg.mozilla.org/integration/mozilla-inbound/rev/b58b537b36b3 https://hg.mozilla.org/integration/mozilla-inbound/rev/16dbfe2ae98b https://hg.mozilla.org/integration/mozilla-inbound/rev/f7894fb36f72 https://hg.mozilla.org/integration/mozilla-inbound/rev/eff14a3bd0da https://hg.mozilla.org/integration/mozilla-inbound/rev/2f11a3566554 https://hg.mozilla.org/integration/mozilla-inbound/rev/1d98ab2314e0 https://hg.mozilla.org/integration/mozilla-inbound/rev/cd59583cf7d4 https://hg.mozilla.org/integration/mozilla-inbound/rev/a9a778e471d1 https://hg.mozilla.org/integration/mozilla-inbound/rev/5859f7d3fb25
Comment 24•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c7daeedb89d5 https://hg.mozilla.org/mozilla-central/rev/b58b537b36b3 https://hg.mozilla.org/mozilla-central/rev/16dbfe2ae98b https://hg.mozilla.org/mozilla-central/rev/f7894fb36f72 https://hg.mozilla.org/mozilla-central/rev/eff14a3bd0da https://hg.mozilla.org/mozilla-central/rev/2f11a3566554 https://hg.mozilla.org/mozilla-central/rev/1d98ab2314e0 https://hg.mozilla.org/mozilla-central/rev/cd59583cf7d4 https://hg.mozilla.org/mozilla-central/rev/a9a778e471d1 https://hg.mozilla.org/mozilla-central/rev/5859f7d3fb25
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in
before you can comment on or make changes to this bug.
Description
•