Always show negative numbers in about:memory, and highlight them

RESOLVED FIXED in mozilla12

Status

()

Toolkit
about:memory
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: njn)

Tracking

Trunk
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink], URL)

Attachments

(4 attachments, 5 obsolete attachments)

(Reporter)

Description

6 years ago
When heap-unclassified is negative, for example, it should be shown despite being below the cutoff.  When it's buggy it should be visible.
OS: Mac OS X → All
Hardware: x86 → All
Summary: Always show negative numbers → Always show negative numbers in about:memory
Let's also highlight them while we're at it, so they're easier to see visually.
Summary: Always show negative numbers in about:memory → Always show negative numbers in about:memory, and highlight them
Wow, I said "see visually".  I'm sorry, Mrs. Janoff.

Updated

6 years ago
Duplicate of this bug: 701502
This is tricker than I thought it would be at first.  But I'm working on it...
Assignee: nobody → justin.lebar+bug
Created attachment 576065 [details] [diff] [review]
Patch v1
Attachment #576065 - Flags: review?(nnethercote)
(Assignee)

Comment 6

6 years ago
Comment on attachment 576065 [details] [diff] [review]
Patch v1

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

On the right track, but I'd like to see another revision.

::: toolkit/components/aboutmemory/content/aboutMemory.css
@@ +43,2 @@
>    color: #400;
>  }

Why did you split .mrValue's rule in two?

@@ +79,5 @@
> +.bad-value:after {
> +  color: #b45f04;
> +  content: " (?!)";
> +  -moz-user-select: text;
> +}

We already use "[*]" for unknown values (with the corresponding style |mStar|), generated by aboutMemory.js instead of via CSS.  It shows up in the test.  Can you do something like that?  Maybe "[?]" as the symbol?

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +612,5 @@
> +    for (var i = 0; i < aNode._kids.length; i++) {
> +      markMayOmit(aNode._kids[i]);
> +      omit = omit && aNode._kids[i]._mayOmit;
> +    }
> +    aNode._mayOmit = omit;

You should mention _mayOmit in the comment within TreeNode's constructor... but as we discussed on IRC, trying to do tree value arithmetic with negative numbers is kinda bogus.  Just showing them seperately would avoid complicating the tree handling code.

@@ +740,5 @@
>    } else {
> +    // We have to handle negative values of aBytes here, rather than let
> +    // formatInt handle it for us.  We pass only the number of megabytes to
> +    // formatInt, but "-0" == 0, so formatInt won't properly handle negative
> +    // values of absolute value less than 1mb!

Handling this in formatInt would be better.  If x equals 0 or -0, in JS you can determine which it is with 1/x -- it either gives Infinity or -Infinity.
Attachment #576065 - Flags: review?(nnethercote) → review-
(Assignee)

Comment 7

6 years ago
Thinking about this some more...

Sequestering negative values when the memory reporters are accessed won't detect a negative heap-unclassified, because it's derived from other reporters.  Furthermore, heap-unclassified is by far the most likely value to be negative precisely because it is the only non-trivial derived value:

- Reports from reporters are unlikely to be negative, because that'd be a pretty weird bug.

- The non-leaf nodes in the trees are also derived, but they only involve addition and so won't be negative if none of the reporters are negative.

In comparison, heap-unclassified's derivation involves a subtraction, and so any over-counting by a reporter can lead to it being negative.

With all that in mind, I wonder if it's worth only checking if heap-unclassified is negative, and if so, displaying it as "0 B (00.00%) -- heap-unclassified [?]".  That would be simple and catch the case we've actually seen in practice multiple times.
That sounds like a reasonable compromise to me, to simplify things.  Not to bikeshed, but I think the failure should be a little more emphatic than a question mark.  Would it be possible to do something like "?? B (??.??%) -- heap-unclassified"?
(Reporter)

Comment 9

6 years ago
"??" sounds like "I don't know". It would be clearer to show the negative number (which is somewhat useful for figuring out which reporter is buggy) and add "WTF??" after it.
There was that canvas memory reporter which went negative occasionally (not at my awesomebar, don't have the bug).  Any memory reporter which uses a counter, instead of walking the graph of live objects, could go negative.
(Assignee)

Updated

6 years ago
Assignee: justin.lebar+bug → n.nethercote
(Assignee)

Comment 11

6 years ago
Created attachment 589402 [details] [diff] [review]
Patch 1

This patch takes a slightly different approach.  It highlights bogus values,
but doesn't do anything to ensure they aren't omitted in non-verbose mode.
However, it does print a warning at the top of the process's info (much like
is done in bug 715453) to indicate that something went wrong.  I think this
is good enough, and it's easier than the other approaches.

I added some testing, and I also beefed up the testing of |kUnknown| in 
test_aboutmemory.xul.
Attachment #576065 - Attachment is obsolete: true
Attachment #589402 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 12

6 years ago
Created attachment 589403 [details] [diff] [review]
Patch 2

This patch does some clean-ups:

- Renames |asString| as |_asString|, for consistency with other data
  properties.

- Renames |_hasProblem| as |_isUnknown| (and similar cases), for consistency
  with |_isInvalid| and |kUnknown|.

- Removes a dead |hasProblem| local variable.
Attachment #589403 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 13

6 years ago
Created attachment 589404 [details]
screenshot of a highlighted invalid value
I don't like "hey, there was an error, but we're probably hiding it.  Be skeptical."

How about we

 * switch to verbose mode upon detecting an erroneous value, or
 * display the name(s) of the erroneous reporter(s) in the error message?
Attachment #589402 - Flags: review?(justin.lebar+bug) → review-
Attachment #589403 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Updated

6 years ago
Depends on: 719335
(Assignee)

Updated

6 years ago
Blocks: 698928
(Assignee)

Comment 15

6 years ago
Created attachment 592017 [details] [diff] [review]
patch 1, v2: highlight bogus values

This version reports the names of memory reporters that give bogus values.  
And because we have expandable trees now, if the entry isn't visible you can
click-to-expand and find it.
Attachment #589402 - Attachment is obsolete: true
Attachment #589403 - Attachment is obsolete: true
Attachment #589404 - Attachment is obsolete: true
Attachment #592017 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 16

6 years ago
Created attachment 592018 [details]
new screenshot

New screenshot from patch 1, v2.
(Assignee)

Comment 17

6 years ago
Created attachment 592019 [details] [diff] [review]
patch 2, v2

This patch is basically unchanged from v1.  Carrying over the r+.
Attachment #592019 - Flags: review+
(Assignee)

Comment 18

6 years ago
Created attachment 592021 [details] [diff] [review]
patch 3: make unsanitized things more obvious

Names, paths and descriptions need to be sanitized before putting them into
HTML.  The way this was done was a bit ad hoc -- it was sometimes hard to
tell if a thing had been sanitized or not, and it was making me nervous.

This patch makes things much clearer by using "safe"/"unsafe" prefixes for 
names everywhere that is appropriate.  If JS had static types I'd use them
to enforce the unsafe/safe distinction, but this is still much better than
what we had.  For example, while doing this I discovered that several paths 
in assertion failure messages weren't being sanitized -- not a big deal but
still nice to fix.

I also discovered that the description for "heap-unclassified" was 
truncated...  I missed a '+' operator and JS's stupid semi-colon insertion
slapped me in the face, sigh.
Attachment #592021 - Flags: review?(justin.lebar+bug)
(Assignee)

Updated

6 years ago
Whiteboard: [MemShrink]
(Assignee)

Comment 19

6 years ago
Created attachment 592023 [details] [diff] [review]
patch 3b: make unsanitized things more obvious

Attach the right patch this time.
Attachment #592021 - Attachment is obsolete: true
Attachment #592021 - Flags: review?(justin.lebar+bug)
Attachment #592023 - Flags: review?(justin.lebar+bug)
> This version reports the names of memory reporters that give bogus values.  
> And because we have expandable trees now, if the entry isn't visible you can
> click-to-expand and find it.

Now that we have click-to-expand, can we automatically expand the path to any invalid reporter?  Presumably this should be simple to do, right?
Comment on attachment 592023 [details] [diff] [review]
patch 3b: make unsanitized things more obvious

This is pretty ugly, but I understand the need and don't have a better idea.  r=me.
Attachment #592023 - Flags: review?(justin.lebar+bug) → review+
Comment on attachment 592017 [details] [diff] [review]
patch 1, v2: highlight bogus values

>@@ -804,7 +843,7 @@ function formatBytes(aBytes)
>   } else {
>     var mbytes = (aBytes / (1024 * 1024)).toFixed(2);
>     var a = String(mbytes).split(".");
>-    s = formatInt(a[0]) + "." + a[1] + " " + unit;
>+    s = formatInt(Number(a[0])) + "." + a[1] + " " + unit;

Can you please add a comment explaining that formatInt hanadles negative
Number(a[0]), even -0?

>@@ -1053,6 +1100,15 @@ function genTreeText(aT, aProcess)
>       return s;
>     }
> 
>+    // Determine if we should show the sub-tree below this entry;  this
>+    // involves reinstate any previous toggling of the sub-tree.

reinstating.

r=me, but if you decide to expand the path down to invalid reporters (I'd like it if we did!), I'd like to take a look at that change.
Attachment #592017 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Comment 23

6 years ago
> This is pretty ugly, but I understand the need and don't have a better idea.
> r=me.

Yep :/


> Now that we have click-to-expand, can we automatically expand the path to
> any invalid reporter?  Presumably this should be simple to do, right?

With the current page generation strategy -- generate a big HTML string and then set innerHTML -- it's a bit difficult.  By the time you realize you should expand parent nodes you've already generated the HTML code for them.

I'm planning to investigate instead building the DOM directly with things like document.createElement.  The main motivation is to hopefully reduce the amount of memory used when generating the page, but it would also make this expansion a lot easier because we'd have a live data structure to fiddle with instead of a half-formed string.
> By the time you realize you should expand parent nodes you've already generated the HTML code for 
> them.

But you could still toggle() all the relevant nodes, no?
(Assignee)

Comment 25

6 years ago
I'd have to record them somewhere and do it after setting innerHTML.  I'd rather wait and be able to do it on the spot.
(In reply to Nicholas Nethercote [:njn] from comment #25)
> I'd have to record them somewhere and do it after setting innerHTML.  I'd
> rather wait and be able to do it on the spot.

Okay, I think I understand what the issue is.  If you have a plan to change it, that's fine.
(Assignee)

Comment 27

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cba1d921a6d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/f989793c20bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/50080b09ae04
(Assignee)

Comment 28

6 years ago
(In reply to Nicholas Nethercote [:njn] from comment #23)
> > This is pretty ugly, but I understand the need and don't have a better idea.
> > r=me.
> 
> Yep :/

I just learnt that if you don't generate HTML strings but instead build the page via DOM methods and properties you don't need to escape all those characters.  Still, I'll probably keep the safe/unsafe names as long as the backslash flipping is needed.  (I do hope to get rid of that one day.)
https://hg.mozilla.org/mozilla-central/rev/cba1d921a6d5
https://hg.mozilla.org/mozilla-central/rev/f989793c20bd
https://hg.mozilla.org/mozilla-central/rev/50080b09ae04
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
(Assignee)

Updated

6 years ago
Blocks: 723402
You need to log in before you can comment on or make changes to this bug.