Don't include large negative values in the "tiny" category in about:memory

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mccr8, Assigned: njn)

Tracking

unspecified
mozilla32
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment)

In bug 1005442, Bobby hit a case with a memory reporter diff that was hard to analyze because a -1MB size was included in a "tiny" category.  The decision to include something in "tiny" should probably use the absolute value.
This happens when you have a large negative and large positive children of a node, like this:

* 0.01 MiB
  * 1.01 MiB
  * -1.00 MiB

The "is this tiny?" test operates on the 0.01 MiB parent node. It's like this because that makes sense when you don't have negative values, and the diff stuff was implemented much later. I don't know off the top of my head how hard it'll be to change this behaviour.
Whiteboard: [MemShrink] → [MemShrink:P2]
Assignee: nobody → n.nethercote
There are even trickier cases. Consider this:

> 1,001,000 B (100.0%) -- a
> ├──1,000,000 B (99.90%) ── b
> ├──────1,000 B (00.10%) ── g
> └──────────0 B (00.00%) -- c
>            ├──-999,000 B (-99.80%) ── e
>            ├──998,000 B (99.70%) ── d
>            └──1,000 B (00.10%) ── f

Which nodes are insignificant and should be hidden? Definitely 'g', because it
only accounts for 0.1% of the tree.

But what about 'c'? It accounts for 0% of the tree, but has children that
account for large (positive and negative) fractions of the tree.

So here's an idea: in diff mode, when considering a node for sorting and significance purposes, don't look just at its value. Instead, get the maximum absolute value of it and all its descendents. In the example above, that would result in 'c' having the value 999,000 for sorting and significant purposes, which would (a) make it get sorted ahead of 'g', and (b) make it be considered significant.

I have a draft patch implementing this which seems to work, but polishing it will have to wait until Monday.
This patch implements the idea from comment 2.
Attachment #8428441 - Flags: review?(continuation)
Comment on attachment 8428441 [details] [diff] [review]
Implement better sub-tree sorting and significance detection in about:memory's diff mode

Review of attachment 8428441 [details] [diff] [review]:
-----------------------------------------------------------------

Yay for tests!

(I didn't get to this until today due to the 3 day weekend in the US.)

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +1082,5 @@
>    },
>  
> +  // When gIsDiff is false, tree operations -- sorting and determining if a
> +  // sub-tree is significant -- are straightforward. But when gIsDiff is true,
> +  // the combination of positive and negative values within a tree complicate

nit: complicates.  Unless combination is plural in Australian English or something.  I vaguely recall there's some difference in the way American English handles collective nouns, or something...

@@ +1084,5 @@
> +  // When gIsDiff is false, tree operations -- sorting and determining if a
> +  // sub-tree is significant -- are straightforward. But when gIsDiff is true,
> +  // the combination of positive and negative values within a tree complicate
> +  // things. So for a non-leaf node, instead of just looking at _amount, we
> +  // instead look at the maximum absolute value of the node and all its

nit: "all its" -> "all of its" maybe?

@@ +1087,5 @@
> +  // things. So for a non-leaf node, instead of just looking at _amount, we
> +  // instead look at the maximum absolute value of the node and all its
> +  // descendents.
> +  maxAbsDescendent: function() {
> +    if (!this._kids) {

I guess this way you avoid caching the max in leaf nodes, which are the most common?

@@ +1099,5 @@
> +    }
> +
> +    // Compute the maximum absolute value of all descendents.
> +    let max = Math.abs(this._amount);
> +    if (this._kids) {

nit: this check is redundant, because we checked !this._kids above.
Attachment #8428441 - Flags: review?(continuation) → review+
> > +  // When gIsDiff is false, tree operations -- sorting and determining if a
> > +  // sub-tree is significant -- are straightforward. But when gIsDiff is true,
> > +  // the combination of positive and negative values within a tree complicate
> 
> nit: complicates.  Unless combination is plural in Australian English or
> something.  I vaguely recall there's some difference in the way American
> English handles collective nouns, or something...

Nope, I just got it wrong. I probably unconsciously made it agree with "values" instead of "combination".

> > +  maxAbsDescendent: function() {
> > +    if (!this._kids) {
> 
> I guess this way you avoid caching the max in leaf nodes, which are the most
> common?

Yep.

BTW, I looked up "descendent" vs "descendant". Both are acceptable, but the latter is more common both in general and within the Mozilla codebase, so I switched to using it.
> BTW, I looked up "descendent" vs "descendant". Both are acceptable, but the latter is more common both in general and within the Mozilla codebase, so I switched to using it.
Yeah, it looked odd to me, so I looked it up, and it seemed correct so I didn't say anything. ;)
https://hg.mozilla.org/mozilla-central/rev/a6d5e1bfc6ac
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.