Last Comment Bug 760352 - Treeify "Other Measurements" in about:memory
: Treeify "Other Measurements" in about:memory
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: about:memory (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Nicholas Nethercote [:njn] (on vacation until July 11)
:
Mentors:
Depends on:
Blocks: 711130
  Show dependency treegraph
 
Reported: 2012-05-31 21:08 PDT by Nicholas Nethercote [:njn] (on vacation until July 11)
Modified: 2012-06-13 05:57 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch 1: allow OTHER reports to be in trees (70.54 KB, patch)
2012-06-03 22:05 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
justin.lebar+bug: review+
Details | Diff | Review
Patch 2: treeify non-JS-compartment OTHER reports where appropriate (9.05 KB, patch)
2012-06-03 22:08 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
justin.lebar+bug: review+
Details | Diff | Review
Patch 3: preserve alignment on 100% values (3.83 KB, patch)
2012-06-03 22:10 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
justin.lebar+bug: review+
Details | Diff | Review

Description Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-31 21:08:08 PDT
The "Other Measurements" section in about:memory is currently a list.  It would be better if it was a list of trees.
Comment 1 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-31 21:24:56 PDT
I have a draft patch that does this.  Below is some sample output.  (Ignore the bogus js-main-runtime tree which sums values that overlap.)  Things to note...

- Trees come before single entries.

- Trees are in alphabetical order of the root node names.

- There's a blank line after each tree.

- Single entries are in alphabetical order of the entry names.

- Single entries have padding on their left to keep them aligned.

At first I had trees and single entries intermingled, with no blank lines, and it was hard to read.  I think this looks good, but I'd be happy to hear suggestions.


Other Measurements
120 (100.0%) -- js-compartments
├──119 (99.17%) ── system
└────1 (00.83%) ── user

58.77 MB (100.0%) -- js-main-runtime
├──43.60 MB (74.19%) -- gc-heap
│  ├──15.89 MB (27.05%) ── committed
│  ├───8.19 MB (13.93%) ── allocated
│  ├───7.71 MB (13.12%) ── committed-unused
│  ├───5.58 MB (09.49%) ── arena-unused
│  ├───4.11 MB (06.99%) ── decommitted
│  └───2.13 MB (03.63%) -- chunk
│      ├──2.13 MB (03.63%) ── dirty-unused
│      └──0.00 MB (00.00%) ── clean-unused
├───3.99 MB (06.79%) ── shapes
├───3.96 MB (06.74%) ── scripts
├───3.10 MB (05.27%) ── objects
├───1.88 MB (03.19%) ── analysis-temporary
├───1.75 MB (02.97%) ── strings
└───0.50 MB (00.85%) ++ (2 tiny)

143,710 (100.0%) -- page-faults
├──143,625 (99.94%) ── soft
└───────85 (00.06%) ── hard

2.10 MB (100.0%) -- window-objects
├──1.33 MB (63.43%) -- layout
│  ├──0.81 MB (38.57%) ── style-sets
│  ├──0.50 MB (23.92%) ── arenas
│  └──0.02 MB (00.93%) ++ (2 tiny)
├──0.46 MB (22.11%) ── style-sheets
└──0.30 MB (14.46%) ── dom

 0.00 MB ── gfx-surface-image
 0.18 MB ── gfx-surface-xlib
       0 ── ghost-windows
42.80 MB ── heap-allocated
50.90 MB ── heap-committed
 8.08 MB ── heap-committed-unused
  18.80% ── heap-committed-unused-ratio
 1.91 MB ── heap-dirty
24.19 MB ── heap-unused
 0.00 MB ── images-content-used-uncompressed
20.00 MB ── js-gc-heap
  94.15% ── js-main-runtime-gc-heap-committed-unused-ratio
 6.33 MB ── storage-sqlite
Comment 2 Justin Lebar (not reading bugmail) 2012-05-31 21:44:25 PDT
> (Ignore the bogus js-main-runtime tree which sums values that overlap.)

How do you plan to deal with this?
Comment 3 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-31 23:42:40 PDT
> > (Ignore the bogus js-main-runtime tree which sums values that overlap.)
> 
> How do you plan to deal with this?

Don't put overlapping entries in the same tree.  E.g. there may end up being one js-main-runtime tree and a few single entries for cross-cutting measurements.
Comment 4 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-06-03 22:05:04 PDT
Created attachment 629695 [details] [diff] [review]
Patch 1: allow OTHER reports to be in trees

This patch puts in place the infrastructure to allow trees in about:memory's
"Other Measurements" section, but it doesn't actually take advantage of it.
(That's for follow-up patches.)  The only difference in output is that some
things had to change name.  (Once the OTHER reports are treeified, the 
output will be very much as comment 1 shows.)

Details of the patch.

- I removed the mention of LOW_MEMORY_EVENTS_COMMIT_SPACE, because the
  "low-memory-events-commit-space" reporter doesn't exist.

- Now that KIND_OTHER reporters can have '/' in them, there's no need for
  KIND_SUMMARY.  So I removed it.  I move the report checking into the
  individual handleReport() functions to allow the compartments and
  ghost-window reports to have an empty description.

- While I was at it, I removed KIND_MAPPED, which has been deprecated since
  FF8.

- It's no longer possible to have an OTHER reporter with the same name as
  that of a tree.  To avoid conflicts with the "resident" and "vsize" single
  reporters, I renamed the "vsize" tree as "size" and "resident" as "rss".
  These match the names used in /proc/<pid>/smaps.

  I also renamed the "explicit" single reporter as "explicit2".  I tried to
  think of a better name, the best other I could think of was
  "explicit-alt", but "explicit2" really conveys its meaning well, IMO.

- OtherReport objects are no longer used in aboutMemory.js, because OTHER
  reports are just put in trees like non-OTHER reports.

- TreeNode objects now have their unit included.  Also, "degenerate" nodes
  (i.e. trees with a single entry) are marked as such, because they get
  slightly modified treatment during presentation.

Sorry the patch does so much.  It didn't want to split up into smaller
patches :/

One nice thing is that it's a net reduction of about 30 lines of code even
though the OTHER reports are now more powerful.  This is because OTHER
reports are now treated more like other reports, instead of being
special-cased.
Comment 5 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-06-03 22:08:27 PDT
Created attachment 629696 [details] [diff] [review]
Patch 2: treeify non-JS-compartment OTHER reports where appropriate

This patch converts most of the suitable OTHER reporters to tree form.  It
doesn't do the js-main-runtime ones, though;  I'll do them in bug 711130.
Comment 6 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-06-03 22:10:00 PDT
Created attachment 629697 [details] [diff] [review]
Patch 3: preserve alignment on 100% values

I noticed that tree percentages that round up to 100.00% have an extra char
in them;  use 100.0% instead to preserve alignment.  I prefer consistent
alignment to significant figure correctness in this case :)
Comment 7 Justin Lebar (not reading bugmail) 2012-06-10 22:59:48 PDT
I haven't looked at the aboutMemory.js changes yet, but I wanted to give you my thoughts so far.

Thanks for being patient with me here.  I'll look at the rest of the changes in the morning.

>diff --git a/xpcom/base/nsIMemoryReporter.idl b/xpcom/base/nsIMemoryReporter.idl
>--- a/xpcom/base/nsIMemoryReporter.idl
>+++ b/xpcom/base/nsIMemoryReporter.idl
>+   * The paths of all reporters form a list of trees.  Some trees can be
>+   * "degenerate", i.e. contain a single entry with no '/'.

Sorry to comment nit, but this is a set, not a list.  Also, the second sentence
sounds like you're saying that only some trees may be degenerate, which,
although true, is an about:memory restriction, which you discuss elsewhere.

>-   * - All other paths represent cross-cutting values and may overlap with any
>-   *   other reporter.  Reporters in this category must have paths that do not
>-   *   contain '/' separators, kind OTHER, and a description that is a
>+   * - The "compartments" and "ghost-windows" trees are optional.  They are
>+   *   used by about:compartments.  Reporters in these trees must have kind
>+   *   OTHER, units COUNT, an amount of 1, and a description that's an empty
>+   *   string.
>+   *
>+   * - All other reports must have kind OTHER, and a description that is a
>    *   sentence.
>    */
>   readonly attribute AUTF8String path;

I'm not sure it makes sense to put all of this exposition onto the "path"
attribute; perhaps you can move it around.

>-   * There are three categories of memory reporters:
>+   * There are three kinds of memory reporters.
>    *
>    *  - HEAP: memory allocated by the heap allocator, e.g. by calling malloc,
>    *    calloc, realloc, memalign, operator new, or operator new[].  Reporters
>-   *    in this category must have units UNITS_BYTES and must have a path
>-   *    starting with "explicit/".
>+   *    in this category must have units UNITS_BYTES.
>    *
>    *  - NONHEAP: memory which the program explicitly allocated, but does not
>    *    live on the heap.  Such memory is commonly allocated by calling one of
>    *    the OS's memory-mapping functions (e.g. mmap, VirtualAlloc, or
>-   *    vm_allocate).  Reporters in this category must have units UNITS_BYTES
>-   *    and must have a path starting with "explicit/" or "smaps/".
>+   *    vm_allocate).  Reporters in this category must have units UNITS_BYTES.
>    *
>-   *  - OTHER: reporters which don't fit into either of these categories. Such
>-   *    reporters must have a path that does not start with "explicit/" or
>-   *    "smaps/" and may have any units.
>-   *
>-   *  - SUMMARY: reporters which report data that's available in a more
>-   *    detailed form via other reporters.  These reporters are sometimes
>-   *    useful for efficiency purposes -- for example, a KIND_SUMMARY reporter
>-   *    might list all the JS compartments without the overhead of the full JS
>-   *    memory reporter, which walks the JS heap.
>-   *
>-   *    Unlike other reporters, SUMMARY reporters may have empty descriptions.
>-   *
>-   *    SUMMARY reporters must not have a path starting with "explicit/" or
>-   *    "smaps/".
>+   *  - OTHER: reporters which don't fit into either of these categories.
>+   *    They can have any units.

These descriptions aren't parallel.  It's "HEAP: memory ...", "NONHEAP: memory
...", "OTHER: reporters ...".  Seems like they should all be "X: reporters".

>diff --git a/xpcom/base/nsMemoryReporterManager.cpp b/xpcom/base/nsMemoryReporterManager.cpp
>--- a/xpcom/base/nsMemoryReporterManager.cpp
>+++ b/xpcom/base/nsMemoryReporterManager.cpp
>@@ -486,33 +486,33 @@ NS_MEMORY_REPORTER_IMPLEMENT(HeapAllocat
>     KIND_OTHER,
>     UNITS_BYTES,
>     GetHeapAllocated,
>     "Memory mapped by the heap allocator that is currently allocated to the "
>     "application.  This may exceed the amount of memory requested by the "
>     "application because the allocator regularly rounds up request sizes. (The "
>     "exact amount requested is not recorded.)")
> 
>-// The computation of "explicit" fails if "heap-allocated" isn't available,
>+// The computation of "explicit2" fails if "heap-allocated" isn't available,
> // which is why this is depends on HAVE_HEAP_ALLOCATED_AND_EXPLICIT_REPORTERS.
>-#define HAVE_EXPLICIT_REPORTER 1
>-static nsresult GetExplicit(PRInt64 *n)
>+#define HAVE_EXPLICIT2_REPORTER 1
>+static nsresult GetExplicit2(PRInt64 *n)
> {
>     nsCOMPtr<nsIMemoryReporterManager> mgr = do_GetService("@mozilla.org/memory-reporter-manager;1");
>     if (mgr == nsnull)
>         return NS_ERROR_FAILURE;
> 
>     return mgr->GetExplicit(n);
> }
> 
>-NS_FALLIBLE_MEMORY_REPORTER_IMPLEMENT(Explicit,
>-    "explicit",
>+NS_FALLIBLE_MEMORY_REPORTER_IMPLEMENT(Explicit2,
>+    "explicit2",
>     KIND_OTHER,
>     UNITS_BYTES,
>-    GetExplicit,
>+    GetExplicit2,
>     "This is the same measurement as the root of the 'explicit' tree.  "
>     "However, it is measured at a different time and so gives slightly "
>     "different results.")
> #endif  // HAVE_HEAP_ALLOCATED_REPORTERS

My initial reaction was to suggest calling this guy "explicit" and renaming the current "explicit" to "explicit-tree".  Then we'd be able to have "pss-tree", "vsize-tree", and "rss-tree", which would be nice, because I still don't like the "vsize" --> "size" change (*).

But that change would require touching the majority of memory reporters, which I don't think either one of us wants to do.

So instead, perhaps we name "explicit2" "explicit:notree" and have a rule that if a memory reporter's name ends with ":notree" (or whatever), we strip that part off before displaying it.  Then we can also have "vsize:notree".  (I also don't think "explicit2" is particularly descriptive.)

(*) It matches what the kernel uses, but I don't think that difference was a pain point.  I think we should use the names we think are right and figure out how to accommodate that in about:memory.

How are you handling the fact that there's a "ghost-windows" single reporter, and also a "ghost-windows/" tree?  Doesn't this have the same problem as the (v)size reporters, or does this just happen to work because about:memory ignores the latter and about:compartments ignores the former?
Comment 8 Justin Lebar (not reading bugmail) 2012-06-11 07:53:53 PDT
Comment on attachment 629695 [details] [diff] [review]
Patch 1: allow OTHER reports to be in trees

>@@ -520,91 +499,100 @@ function getTreesAndOthersByProcess(aMgr
>   function handleReport(aProcess, aUnsafePath, aKind, aUnits, aAmount,
>                         aDescription)
>   {
>+    // Tree report.  Get the tree for the process, creating it if necessary.
>+    // All the trees for each process ("explicit", "rss", etc) are stored
>+    // in a "tree-of-trees".  This makes things simple later.

Is anything not a tree report anymore?

>+    if (!aTreesByProcess[process]) {
>+      aTreesByProcess[process] = new TreeNode("tree-of-trees", undefined);
>+    }

Could you please comment |/* units = */ undefined|?

> //---------------------------------------------------------------------------
> 
> // There are two kinds of TreeNode.
> // - Leaf TreeNodes correspond to reports.
> // - Non-leaf TreeNodes are just scaffolding nodes for the tree;  their values
> //   are derived from their children.
>-function TreeNode(aUnsafeName)
>+// Some trees are "degenerate", i.e. they contain a single node, i.e. they
>+// correspond to a report whose path had no '/' separators.
>+function TreeNode(aUnsafeName, aUnits, aIsDegenerate)

Nit: s/had/has.

>+TreeNode.compareOtherRoots = function(a, b) {

Could we come up with a better name for this?  compareTreeRoots or compareRootNames, maybe?

>-function addHeapUnclassifiedNode(aT, aOthers, aHeapTotal)
>+function addHeapUnclassifiedNode(aT, aTreeOfTrees, aHeapTotal)

Nit: Couldn't you get the |explicit| tree from aTreeOfTrees?

>@@ -863,54 +857,97 @@ function appendWarningElements(aP, aHasK
>-function appendProcessAboutMemoryElements(aP, aProcess, aTreeOfTrees, aOthers,
>+function appendProcessAboutMemoryElements(aP, aProcess, aTreeOfTrees,
>                                           aHeapTotal, aHasMozMallocUsableSize)
> {
> [...]
>+  // Fill in and sort all the other trees, and also get the length of the
>+  // longest root value among them.

You're only getting the max length of the degenerate tree roots.

r+, but I would be very happy if we could figure out a way not to have "explicit2" and "size".  I guess "size" is not even so bad because the smaps trees are usually hidden, and "size" is under the heading "Virtual Size Breakdown".
Comment 9 Justin Lebar (not reading bugmail) 2012-06-11 08:01:10 PDT
Comment on attachment 629696 [details] [diff] [review]
Patch 2: treeify non-JS-compartment OTHER reports where appropriate

I'm not sure summing low-memory-events/{virtual,physical} and page-faults/{soft,hard} makes much sense.

The page faults sum should always be dominated by the soft number, so it's not particularly interesting IMO.  Maybe there's a better case for combining the low-memory events, but they're also different things.
Comment 10 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-06-11 22:47:22 PDT
> How are you handling the fact that there's a "ghost-windows" single
> reporter, and also a "ghost-windows/" tree?  ... does this just happen to
> work because about:memory ignores the latter and about:compartments
> ignores the former?

That's right.

I started implementing the :notree idea, but stopped when I became unhappy with the idea of a reporter's patch not matching what is shown in about:memory.  jlebar, how about we just use what's in the patch with the exception of renaming "explicit2" as "explicit-alt"?


> >+TreeNode.compareOtherRoots = function(a, b) {
>
> Could we come up with a better name for this?  compareTreeRoots or
> compareRootNames, maybe?

compareRootNames is wrong because it sorts on kids/no-kids first.  compareTreeRoots is too generic, because this sort is just for the trees in the "Other Measurements" section.  compareOtherTreeRoots is possible, but "TreeRoots" is redundant.  And compareOtherRoots matches nicely with otherTrees.  So I'll keep it as is.


> >-function addHeapUnclassifiedNode(aT, aOthers, aHeapTotal)
> >+function addHeapUnclassifiedNode(aT, aTreeOfTrees, aHeapTotal)
>
> Nit: Couldn't you get the |explicit| tree from aTreeOfTrees?

Yes, but the calling function has already extracted the explicit tree from aTreeOfTrees and checked it is defined, so getting it again just to avoid an argument doesn't seem worthwhile.


> I'm not sure summing low-memory-events/{virtual,physical} and
> page-faults/{soft,hard} makes much sense.
> 
> The page faults sum should always be dominated by the soft number, so it's
> not particularly interesting IMO.  Maybe there's a better case for combining
> the low-memory events, but they're also different things.

I agree about the page faults, I'll revert that change.  But I like summing the low memory events, so I'll keep that.
Comment 11 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-06-11 23:35:00 PDT
> jlebar, how about we just use what's in the patch with the
> exception of renaming "explicit2" as "explicit-alt"?

Another possibility is to call it "explicit-mgr" or "explicit-manager" or "mgr.explicit" or something else that reflects the fact that it is implemented as nsIMemoryReporterManager::explicit.
Comment 12 Justin Lebar (not reading bugmail) 2012-06-12 06:49:12 PDT
> And compareOtherRoots matches nicely with otherTrees.  So I'll keep it as is.

Oh, I see, it's just for comparing the "other" trees.  Okay.  I didn't get that.  Sounds good.

> Another possibility is to call it "explicit-mgr" or "explicit-manager" or "mgr.explicit" or 
> something else that reflects the fact that it is implemented as nsIMemoryReporterManager::explicit.

explicit-single-reporter?  Or even explicit-reporter?  explicit-root?

What I care about more than the name, though, is that we uniformly name the single-reporters which overlap with a tree root.  I think that's explicit, rss, vsize, pss, swap, and ghost-windows.

Alternatively, we could make about:memory handle the case when the single-reporter overlaps with the tree root.  I don't think that should be impossible; we could just have a list of reporters to special-case.
Comment 13 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-06-12 22:15:32 PDT
I ended up finding a way to allow a degenerate tree to share the same name as the root of a non-degenerate tree, and reverted the "explicit2" name to "explicit".  I kept the "size" and "rss" names for the smaps trees, though, to match the Linux kernel's names.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5896112ab18f
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6adf27298db
https://hg.mozilla.org/integration/mozilla-inbound/rev/484b7ec3f2ec

Note You need to log in before you can comment on or make changes to this bug.