Closed Bug 719335 Opened 12 years ago Closed 12 years ago

Allow arbitrary collapsing/expanding of sub-trees in about:memory

Categories

(Toolkit :: about:memory, defect)

Product:
Toolkit
Component:
about:memory
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla12

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

(Whiteboard: [MemShrink])

Attachments

(1 file, 3 obsolete files)

patch v3
43.65 KB, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review
Attached patch patch (obsolete) — — Splinter Review 
This patch allows any sub-tree to be expanded/collapsed just by clicking on
it.  Specifics:

- You can click anywhere on the entry -- e.g. the value, the percentage, the
  name.  This is subtly indicated by using the |cursor: pointer| style.

- Collapsed nodes have a '+' in front of their name to indicate that they
  are collapsible.  It can't be at the end of the name because long names
  can be truncated or run off the right edge of the screen.

- about:memory still uses thresholding to hide small nodes by default, and
  about:memory?verbose shows everything by default.  To facilitate the
  aggregation of tiny nodes, I've (on jlebar's suggestion) introduced "tiny"
  nodes.  E.g. if you have this tree

  - foo
    - a
    - b
    - c

  And b and c were tiny you'd change it to this:

  - foo
    - a
    - tiny
      - b
      - c

  This modifies paths, which is a bit weird, but without doing this you have
  to show every child in a significant sub-tree, and you can end up showing
  lots of insignificant children, which sucks.

- I removed the full tree-hiding feature that we used to hide the
  virtual/RSS/PSS/trees.  Now each of these trees is collapsed to the root
  node by default.

- If you hit the GC/CC/MMU buttons, it remembers any sub-trees that you've
  collapsed/expanded.  If you reload it doesn't, of course.  So I added an
  "Update" button just in case you want to update without doing GC/GC/MMU
  but keep your tree state.  If any new sub-trees appear, they'll just get
  the default state.

- I haven't updated test_aboutmemory.xul yet, I'm waiting to nail down the
  thresholding behaviour before I do that.

jlebar, I recommend applying the patch and playing with it!
Attachment #589785 - Flags: feedback?(justin.lebar+bug)
Attached image screenshot (obsolete) — 
Here's a screenshot showing everything below the level-1 nodes collapsed.  Note the "+tiny" node in particular.
Dreams do come true!
I haven't built with the patch yet, but some off-the-cuff suggestions:

 * Can we only underline-on-hover nodes which can be expanded/collapsed?
 * How does it look if you indicate nodes which are collapsible with a minus sign?
 * Can we line up the +/- in their own column?  (i.e., if a node doesn't have + or -, it has a space there instead.)
 * Maybe we could use Unicode SQUARED_{PLUS,MINUS} (U0229E, U0229F) instead of +/-?

 * Can we make it so that compartments are never put into a "tiny" node?  Compartments are too important to ignore, I think.

 * Are you planning to add "expand all" / "collapse to default" buttons?

 * I'm OK with getting rid of "tiny" and doing what you had earlier, if you think that's better.  That would obviate the need for an exception to implement "never hide any compartment."  But yeah, it's lame seeing "0 bytes: XML" everywhere.  :-/
(In reply to Justin Lebar [:jlebar] from comment #3)
> I haven't built with the patch yet, but some off-the-cuff suggestions:
> 
>  * Can we only underline-on-hover nodes which can be expanded/collapsed?

Underline-on-hover is meant to indicate there's a tooltip.  The '+' (and '-' if it's added) indicate a node can expand/collapse, as does the |cursor: pointer| style.

>  * How does it look if you indicate nodes which are collapsible with a minus
> sign?
>  * Can we line up the +/- in their own column?  (i.e., if a node doesn't
> have + or -, it has a space there instead.)
>  * Maybe we could use Unicode SQUARED_{PLUS,MINUS} (U0229E, U0229F) instead
> of +/-?

A '-' is redundant, unlike the '+' -- you can tell if a node is collapsible because it has children.  But I'll try it and all the other suggestions.

>  * Can we make it so that compartments are never put into a "tiny" node? 
> Compartments are too important to ignore, I think.

>  * Are you planning to add "expand all" / "collapse to default" buttons?

Not sure, I'll think about it.
 
>  * I'm OK with getting rid of "tiny" and doing what you had earlier, if you
> think that's better.  That would obviate the need for an exception to
> implement "never hide any compartment."  But yeah, it's lame seeing "0
> bytes: XML" everywhere.  :-/

I didn't want the tiny nodes, but it's pretty awful without them.  Particular in the vsize/RSS trees you can have a big node that has 100s of insignificant children and it looks terrible in non-verbose mode.
> Underline-on-hover is meant to indicate there's a tooltip.

Yes, but this is un-idiomatic.  Underline plus |cursor: pointer| clearly means "do something".

> A '-' is redundant, unlike the '+' -- you can tell if a node is collapsible because it has 
> children.

Right; it's just a question of idiom.
Comment on attachment 589785 [details] [diff] [review]
patch

Clearing f?, unless there's more feedback you'd like?
Attachment #589785 - Flags: feedback?(justin.lebar+bug)
Attached patch patch v2 (obsolete) — — Splinter Review 
Changes in this version:

- Now using a '++' separator instead of '--' to indicate expandable nodes,
  after some discussion and experimentation with jlebar on IRC.  (The
  Unicode chars looked awful.)

- Expandable entries are underlined when hovered on (as well as the existing
  |cursor: pointer|).  Non-expandable entries aren't underlined.

- I upped the significance in non-verbose mode from 0.5% to 1%.  Now that we
  can expand trees, showing a bit less to begin with seems reasonable, and
  1% is a nice round number.

- Aggregate nodes now look like "(3 tiny)".  Also, it now only inserts
  aggregate nodes where it's helpful -- no aggregate node will end up with a
  single child, and no node will end up with a single child that's an
  aggregate node.

- I updated the legend at the bottom.

Compartments can still be collapsed.  I think that's the right thing to do.
If we want a "show all compartments" view, that should be something
different, e.g. an about:compartments page.

I haven't added "expand all" or "collapse to default" buttons.  The
former can be achieved by just loading about:memory?verbose and the latter
can be achieved by hitting refresh.  In both cases that causes the
measurements to be re-taken, but I don't think avoiding that is important
enough to bother with explicit buttons for these operations.

I still haven't updated test_aboutmemory.xul.  And I need to add a test for
the collapsing/expanding;  that'll be fun.
Attachment #590067 - Flags: feedback?(justin.lebar+bug)
* The tooltips should appear when you hover anywhere over the entry.

* Maybe we don't need the gray bars on the section headers anymore?  I put them there to make the clickability more obvious, but now they're not clickable.  If you get rid of the bars, you should also probably move the headers a bit closer to the trees (the headees?).

* Update seems to take a long time (in a debug build, admittedly).  I'm spinning a release build to check how fast it is there.

* Clicking update (debug build) adds 10-20mb of RSS.  Much of it goes away after minimize memory usage, but that's still a lot of noise added by opening about:memory.  In my vanilla release build, the difference is much smaller.  I'll check once my build finishes how much memory it uses.

* I'm still confused visually by which nodes can and can't be contracted.  The underline doesn't help a lot, because it only appears on rollover.  I'll see if I can come up with a solution which I like.
A patched release build:

Resident Set Size (RSS) Breakdown
  90.61 MB (100.0%) ++ resident

Other Measurements
  113.17 MB -- resident

Ouch.  I don't see a difference anywhere this large in my nightly build.
I think I could get used to using \U00B7 (·) for the leaf (or even expanded non-leaf) reporters, with the other retaining "--".  It's still noisier than I'd like, though...
Or we could make the leaves black and leave the non-leaves navy.  That looks marginally better than "··" to my eyes. We should check with someone who's colorblind to see whether there's enough contrast between those two colors.
Comment on attachment 590067 [details] [diff] [review]
patch v2

I haven't looked at the code, but this sure looks like we're going in the right direction.
Attachment #590067 - Flags: feedback?(justin.lebar+bug) → feedback+
You could try getting some UX team input here, assuming the ASCII programmer art won't cause them migraines. ;)
> We should check with someone who's colorblind to see whether there's enough contrast between those 
> two colors.

My colorblind friend is OK with the navy/black difference, although of course everyone is different.
I like the gray section header bars!

I also noticed that update seemed slower in debug builds, though I haven't investigated further.  That plus the memory usage worries me.  I have some measurements/ideas that I'll try on Monday.

Leaf nodes can't be contracted, non-leaf nodes can.  Simple :)  I'm not keen on further visual distinction between the two cases (e.g. with colour) because it makes the page look very noisy.

When I first overhauled about:memory I got exactly zero feedback from the UX guys despite asking several times, so I don't expect that to change for a much more minor change.
> Leaf nodes can't be contracted, non-leaf nodes can.  Simple :)

Understood.  :)  I think this is unsettling to me because you can't tell by looking at a line whether it can be contracted; you have to look at the line below and judge the indentation level.
I think using two \u2500 (horizontal box-drawing lines) for leaf nodes doesn't look too bad.

│  │  │   ├──1.95 MB (02.98%) -- shapes
│  │  │   │  ├──1.15 MB (01.76%) ── tree
│  │  │   │  └──0.80 MB (01.22%) ++ (2 tiny)
│  │  │   ├──1.03 MB (01.58%) ── scripts
│  │  │   ├──0.96 MB (01.46%) -- arena
(In reply to Justin Lebar [:jlebar] from comment #9)
> A patched release build:
> 
> Resident Set Size (RSS) Breakdown
>   90.61 MB (100.0%) ++ resident
> 
> Other Measurements
>   113.17 MB -- resident
> 
> Ouch.  I don't see a difference anywhere this large in my nightly build.

Is this repeatable?  I find the two are normally pretty close.  I wonder if the nightly builds are equally capable of occasional large differences but we don't normally notice it because the RSS tree is currently hidden by default.
(In reply to Justin Lebar [:jlebar] from comment #18)
> I think using two \u2500 (horizontal box-drawing lines) for leaf nodes
> doesn't look too bad.

│   ├──11,577,960 B (07.58%) -- compartment([System Principal], 0x7f1ae1c50000)
│   │  ├───6,623,232 B (04.34%) -- gc-heap
│   │  │   ├──2,080,160 B (01.36%) -- objects
│   │  │   │  ├──1,514,464 B (00.99%) ── function
│   │  │   │  └────565,696 B (00.37%) ── non-function
│   │  │   ├──1,937,904 B (01.27%) -- shapes
│   │  │   │  ├──1,154,800 B (00.76%) ── tree
│   │  │   │  ├────442,624 B (00.29%) ── base
│   │  │   │  └────340,480 B (00.22%) ── dict
│   │  │   ├──1,293,472 B (00.85%) -- arena
│   │  │   │  ├──1,173,632 B (00.77%) ── unused
│   │  │   │  ├─────68,096 B (00.04%) ── padding
│   │  │   │  └─────51,744 B (00.03%) ── headers
│   │  │   ├──1,040,832 B (00.68%) ── scripts
│   │  │   ├────220,000 B (00.14%) ── strings
│   │  │   ├─────50,544 B (00.03%) ── type-objects
│   │  │   └────────320 B (00.00%) ── xml

Yeah, not bad.  I'm happy with that.
> Is this repeatable?  I find the two are normally pretty close.  I wonder if the nightly builds are 
> equally capable of occasional large differences but we don't normally notice it because the RSS tree 
> is currently hidden by default.

Quite possibly!  I've been unable to reproduce this result.

On the other hand, I still see a ~20mb jump in RSS when I refresh the page.  Most of that memory goes away when I GC, but the increase is much greater than with vanilla about:memory.

Can we measure RSS at the beginning and end of rendering about:memory and report the difference?  We can do that as a separate bug, but it seems like that work should block this bug.
How does that compare to verbose about:memory?  Maybe this is a dumb question, but I'm just wondering if the memory usage is because we're keeping everything around all the time, or from the new fancy opening closing doo-dads.
With about:memory?verbose currently, I see roughly 10mb change.  I'd expect the two numbers to be much closer than that, so maybe I'm measuring badly.
Testing with about:memory?verbose I don't see much difference, though it's hard to get good measurements.  Not much extra HTML was needed vs. the old verbose HTML, maybe 10--15% if you measure it as a string, and that could be compacted easily by e.g. using shorter class names.

jlebar, if you're measuring about:memory (non-verbose) I'm not surprised you're seeing bigger memory usage, because we're not truncating the trees at all.  In particular, if you're on Linux, the vsize/rss/pss trees are huge.

I did some experiments to reduce memory usage yesterday, didn't get conclusive results, other than removing the vsize/rss/pss trees would be the easiest thing :/  There is a lot of JS string-chars and JS objects.  I want to try to measure the amount of JS memory allocated while rendering, that'll be better than measuring the RSS before/after.
> jlebar, if you're measuring about:memory (non-verbose) I'm not surprised you're seeing bigger memory 
> usage, because we're not truncating the trees at all.  In particular, if you're on Linux, the 
> vsize/rss/pss trees are huge.

Right; this is my theory.

> other than removing the vsize/rss/pss trees would be the easiest thing :/

They're not as useful as the other trees, and if they're messing up the results, we should either get rid of them, move them to another page, or calculate them lazily.

But to make a good decision, we need to know how large the difference is.

> I want to try to measure the amount of JS memory allocated while rendering, that'll be better than 
> measuring the RSS before/after.

I think that's a good thing to measure too.  But the proof is in the resident set.  If RSS changes but JS memory doesn't change, I'll be just as curious why!
I measured the length of the HTML string that gets put into the top-level <div> with just about:memory loaded:

                              non-verbose         verbose
  with vsize/rss/pss/swap     50K                 452K 
  without:                    31K                 69K

I'm fairly confident that the HTML string length correlates strongly with the total memory usage, based on looking at how resident, explicit, js-total-strings and js-total-objects correlate.

I experimented also with using DOM methods like createElement instead of writing HTML fragments and setting innerHTML, but that didn't seem to make much difference.  Another possibility is to build up an array of string fragments and join them at the end (the JS engine uses ropes but this would still avoid lots of object creations.

But those will have much less effect than making vsize/rss/pss/swap lazy.


> > I want to try to measure the amount of JS memory allocated while rendering, that'll be better than 
> > measuring the RSS before/after.
> 
> I think that's a good thing to measure too.  But the proof is in the
> resident set.  If RSS changes but JS memory doesn't change, I'll be just as
> curious why!

The trouble with measuring RSS is that GCs make things confusing.  I want to just measure the amount of memory allocated, not the amount freed, which would give a clearer picture.  Evaluating the effectiveness of these modifications is really hard without something like that.
Attached patch patch v3 — — Splinter Review 
This version is ready for review.  Changes since the last patch:

- Leaf nodes have "──" as their separator.
  
- The vsize/rss/pss/swap trees are only shown in verbose mode.  This is 
  because generating them takes lots of memory.  Bug 720595 will help
  get that memory consumption down.
  
- I added test_aboutmemory2.xul, which tests the expanding/collapsing.

- I updated test_aboutmemory.xul for the changes, and tweaked it so if a 
  test fails the EXPECTED/ACTUAL output is only shown once.
Attachment #589785 - Attachment is obsolete: true
Attachment #589786 - Attachment is obsolete: true
Attachment #590067 - Attachment is obsolete: true
Attachment #590986 - Flags: review?(justin.lebar+bug)
Sorry for sitting on this for so long, Nick.

> .mrPerc {
> }
> 
>+.mrSep {
>+}

Do you mean to have these empty rules?  (I don't much care either way.)

>+  text += "<div>" +
>           "<span class='legend'>Hover the pointer over the name of a memory " +
>-          "reporter to see a detailed description of what it measures. Click a " +
>-          "heading to expand or collapse its tree.</span>" +
>-          "</div>";
>+          "reporter to see a detailed description of what it measures.</span>";

Nit: s/detailed // ?

>-  for (var i = 0; i < aT._kids.length; i++) {
>-    if (shouldOmit(aT._kids[i]._amount)) {
>-      // This sub-tree is below the significance threshold
>-      // Remove it and all remaining (smaller) sub-trees, and
>-      // replace them with a single aggregate node.
>+  // If the first child is insignificant, they all are, and there's no point
>+  // creating an aggregate node that lacks siblings.  Just set the parent's
>+  // _hideKids property and process all children.
>+  if (isInsignificant(aT._kids[0])) {
>+    aT._hideKids = true; 

Nit: Trailing whitespace.

>+    for (var i = 0; i < aT._kids.length; i++) {
>+      filterTree(aTotalBytes, aT._kids[i]);
>+    }
>+    return;
>+  } 

Also here.

>+  

And here.  And elsewhere.  r=me for getting rid of all trailing whitespace in aboutmemory.js; otherwise, would you mind running s/^(\+.*) $/\1/ over the patch a few times?

(Not to be That Git Person, but this is git rebase --whitespace=fix.)

>+      // Process the moved children.
>+      for (i = 0; i < aggT._kids.length; i++) {
>+        filterTree(aTotalBytes, aggT._kids[i]);
>+      }

Is this necessary?  We're not going to hide any of the already-hidden descendants.  Is this just a sort call?  (It looks like they're added to the subtree in sorted order.)

>+// This is used to record which sub-trees have been toggled, so the
>+// collapsed/expanded state can be replicated when the page is regenerated.
>+// It can end up holding IDs of nodes that no longer exist, e.g. for
>+// compartments that have been closed.  This doesn't seem like a big deal,
>+// because the number is limited by the number of entries the user has changed
>+// from their original state.
>+var gToggles = {};
>+
>+function toggle(aEvent)
>+{
>+  // This relies on each line being a span that contains at least five spans:
>+  // mrValue, mrPerc, mrSep ('++'), mrSep ('--'), mrName, and then zero or more
>+  // mrStars.  All whitespace must be within one of these spans for this
>+  // function to find the right nodes.  And the span containing the children of
>+  // this line must immediately follow.  Assertions check this.
>+
>+  function assertClassName(span, className) {
>+    assert(span, "undefined " + className);
>+    assert(span.nodeName === "span", "non-span " + className);
>+    assert(String.search(span.className, className) >= 0,
>+           "bad " + className);
>+  }

You could use span.classList.contains(className).
Attachment #590986 - Flags: review?(justin.lebar+bug) → review+
> Sorry for sitting on this for so long, Nick.

No problem.  I wish more people were consistently fast enough with their reviews that three days was unusually slow!
 

> > .mrPerc {
> > }
> > 
> >+.mrSep {
> >+}
> 
> Do you mean to have these empty rules?  (I don't much care either way.)

mrSep is needed to sanity check that the right node is selected in toggle().  mrPerc is not needed but having it means that each entry consists of five <span> elements, which is nice for consistency.


> >+      // Process the moved children.
> >+      for (i = 0; i < aggT._kids.length; i++) {
> >+        filterTree(aTotalBytes, aggT._kids[i]);
> >+      }
> 
> Is this necessary?  We're not going to hide any of the already-hidden
> descendants.  Is this just a sort call?  (It looks like they're added to the
> subtree in sorted order.)

It's needed to sort and insert aggregate nodes into sub-trees beneath the children.


> >+    assert(String.search(span.className, className) >= 0,
> >+           "bad " + className);
> >+  }
> 
> You could use span.classList.contains(className).

True!  I am a terrible webdev.  And my version would normally be written |span.className.search(className)|... I'm not even sure how the |String.search()| version works, but it does.
> mrSep is needed to sanity check that the right node is selected in toggle().  mrPerc is not needed 
> but having it means that each entry consists of five <span> elements, which is nice for consistency.

Sure, but you can have the classes in your xhtml without having empty rules present in the CSS file.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3116d5b83687


> Sure, but you can have the classes in your xhtml without having empty rules
> present in the CSS file.

True.  But that might cause some head-scratching later on... "Why doesn't this class name have any rules associated with it?" :)
I am not sure that the current situation clarifies anything in that regard!  :D
Whiteboard: [MemShrink]
Comment on attachment 590986 [details] [diff] [review]
patch v3

>+      text += "<span class='mrSep hidden'>++ </span>";
>+      text += "<span class='mrSep'>-- </span>";
>+    } else {
>+      text += "<span class='mrSep'>++ </span>";
>+      text += "<span class='mrSep hidden'>-- </span>";

>+  plusSpan .classList.toggle("hidden");
>+  minusSpan.classList.toggle("hidden");
I might be overlooking something but it's certainly confusing to have both a "hidden" attribute and a "hidden" class...
Where's the "hidden" attribute?  In comment 33, they all look like "hidden" classes to me.
https://hg.mozilla.org/mozilla-central/rev/3116d5b83687
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: