657327 - about:memory merge the "mapped" and "heap used" trees, and make the resulting tree flatter

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch

This is a follow-up to 655642 comment 3.

Currently about:memory has a "mapped" tree and a "heap-used" tree.  The root of the "mapped" tree is what's called "VSIZE" on Linux and Mac.  There are some problems with this:

- You can't measure VSIZE on Windows (see bug 655642).

- The VSIZE values on Mac are ridiculously large, because huge amounts of shared memory gets included.

- Even on Linux, where the values are reasonable, the measurement isn't that useful because it includes lots of implicitly mapped memory (eg. code and data segments) as well as explicitly requested mapped memory (eg. calls to mmap() and the heap) which is what we're most interested in.  Indeed, that's why I put the heap in a separate tree, to try to avoid people focussing too much on the "mapped" value.

Given these facts, I think it's better to stop putting VSIZE at the root of the tree.  Instead, the root of the tree should just be the sum of all the things we measure with memory reporters.  The heap is the lion's share, and we can measure that accurately and easily.  Beyond that, there are a few places where we explicitly map memory (with mmap/VirtualAlloc/whatever) and we either already have reporters for them (eg. the JS JITs), or should soon (eg. bug 546477).

A second issue is that about:memory currently treats mapped memory and heap memory separately.  This means that the memory allocated by various subsystems is split in two parts, and makes for a more deeply-nested tree than necessary.  (Jesse pointed out that this split is undesirable.)

The attached patch fixes both these issues.  Specifically:

- Merges the two trees, and calls the new tree "Requested Memory".  The root node is now called "total".

- The VSIZE measurement is still made on Mac and Linux, but it's now in the "Other Measurements" list, where it's less prominent.

- The heap vs. mapped distinction is gone, and the tree is flatter as a result.  This required adding the 'kind' field to nsIMemoryReporter, and doing some special-casing in aboutMemory.js to find the "heap-unclassified" value.

- The heap-used and heap-unused reporters are now in the "Other Measurements" list.  The heap-unused one in particular wasn't a good fit for the main tree, because it's memory that Firefox code has deallocated.

- The threshold of significance in the tree is now 0.5%.  This tends to give smaller trees in the default (non-verbose) view.

- The "Other Measurements" list is now sorted from largest to smallest.

- I haven't updated the test yet, I'm waiting in case there are further tweaks to the presentation.

Jesse, I'm asking you for review because you've given me by far the most detailed critiques of the structure of about:memory.  Let me know if you're not an appropriate reviewer.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: screenshot

This shows:

- The single tree, with it's new name.

- The flatness of the single tree, and the lack of heap vs. mapped distinctions.

- The smallness of the single tree, thanks to the higher significance threshold (0.5%).

- The new figures in "Other Measurements": vsize, heap-used, heap-unused.

Comment 2

[Jesse Ruderman]

Awesomesauce!

I have comments on some parts of this patch, but I haven't reviewed the C++ and JavaScript code in enough detail to understand it all.  Please get real review from a peer ;)

It's too bad we can't use an enum for "kind" in the IDL (bug 626493).

I'm saddened by the way memory reporters use strings. Many copies of mostly-static strings are made, and there are raw pointers all over the place.

https://developer.mozilla.org/En/Mozilla_internal_string_guide makes it sound like you have to use nsXPIDLCString when you use getter_Copies (e.g. in dom/ipc/ContentChild.cpp). Is the doc wrong?

Maybe the tooltips should show whether things are considered "heap" or "non-heap", since it's no longer captured in the path/name. (Or "mixed" for the combined JS, I guess.)

Should "heap-unclassified" be called "heap-used-unclassified" or "heap-used/unclassified", to make it clear that it doesn't include fragmentation and overhead?

buildTree is getting long. Would it be sensible to split getKnownHeapUsedBytes and creation of heapUnclassified out?

buildTree might no longer need the aTreeName parameter.

> const MR_OTHER = 2;
> 
> const kUnknown = -1; // indicates that a memory reporter failed

It was unclear to me that kUnknown was being used for fields other than "kind".  There might be less special-casing if kind==kUnknown implied bytes==0 rather than bytes==kUnknown.  But either way, an explanatory comment would be nice.

> * _hasProblem: boolean; (might not be defined)

In what cases is it expected to be defined?

> // Nb: aDesc may have single-quotes in it.

Please escape attribute values properly instead of relying on them to not contain characters such as " and &.

> Paths starting with "total" represent non-overlapping regions of memory.

This is a little confusing. "Total" really refers to non-overlapping regions of *explicitly allocated* memory, right?  Maybe "total" is not the best name.

> may overlap arbitrarily with regions in the "mapped" and "heap-used" trees.

This should be updated to refer to the new single tree.

> Each one can be viewed as representing a path in a tree from
> the root node ("total") to a node lower in the tree; 

I found this description a little confusing.  "More general" and "more specific" might be better than "higher" and "lower".

> this lower node may be a non-leaf or a leaf node.

What is this sentence trying to tell me?  Of course a tree has leaves and non-leaves.

The more interesting things about this tree are (1) some intermediate nodes have reporters and some do not, and (2) in the final tree, each node's value will be equal to sum of the values of its children.  Maybe add something like:

"If an intermediate node does not have a reporter, a value will be shown based on the sum of its children. If an intermediate node does have a reporter, an 'other' child will be inserted with the difference from the sum of its children."

> eg. by calling malloc

"e.g." here and in several other places

> "This is the vsize figure as reported by 'top' or 'ps'; on Mac the amount "

Could use ifdefs to show different, detailed messages for each OS.

> // These must match nsIMemoryReporter.
> const MR_MAPPED = 0;

How about:
const MR_MAPPED = Components.interfaces.nsIMemoryReporter.MR_MAPPED;

> "This tree covers explicit memory allocations by the application, " +
> "both at the operating system level (via calls to functions such as " +
> "VirtualAlloc, vm_allocate, and mmap), and at the heap allocation level " +
> "(via functions such as malloc, calloc, realloc, memalign, operator " +
> "new, and operator new[]). It excludes memory mapped implicitly such as " +
> "code and data segments, and thread stacks. It also excludes heap " +
> "memory that has been freed by the application but is still being held " +
> "onto by the heap allocator. Although it may not cover every explicit " +
> "allocation, it does cover most and is the single best number to focus " +
> "on when trying to reduce memory usage.";

It would be good to explain that it includes all heap allocations (annotated or not) and most other explicit allocations (because most of these are annotated). If you can figure out a way to do that without making the tooltip unreasonably long ;)

Comment 3

[Nicholas Nethercote [inactive]]

(In reply to comment #2)
> 
> I'm saddened by the way memory reporters use strings. Many copies of
> mostly-static strings are made, and there are raw pointers all over the
> place.
> 
> https://developer.mozilla.org/En/Mozilla_internal_string_guide makes it
> sound like you have to use nsXPIDLCString when you use getter_Copies (e.g.
> in dom/ipc/ContentChild.cpp). Is the doc wrong?

I think bug 653627 would fix these problems?


> Maybe the tooltips should show whether things are considered "heap" or
> "non-heap", since it's no longer captured in the path/name. (Or "mixed" for
> the combined JS, I guess.)

I added "(Heap) " and "(Mapped) " prefixes for MR_HEAP and MR_MAPPED reporters.


> Should "heap-unclassified" be called "heap-used-unclassified" or
> "heap-used/unclassified", to make it clear that it doesn't include
> fragmentation and overhead?

But it does include them... at least, it includes excess bytes allocated by the heap allocator due to rounding up of allocation sizes.  I changed the tool-tip to explain this.


> buildTree is getting long. Would it be sensible to split
> getKnownHeapUsedBytes and creation of heapUnclassified out?
> 
> buildTree might no longer need the aTreeName parameter.

Done.  I removed the aOmitThresholdPerc param as well.


> > const MR_OTHER = 2;
> > 
> > const kUnknown = -1; // indicates that a memory reporter failed
> 
> It was unclear to me that kUnknown was being used for fields other than
> "kind".  There might be less special-casing if kind==kUnknown implied
> bytes==0 rather than bytes==kUnknown.  But either way, an explanatory
> comment would be nice.

'kUnknown' is only used for the "memoryUsed" property.


> > * _hasProblem: boolean; (might not be defined)
> 
> In what cases is it expected to be defined?

Only if it's true.  I clarified the comment.


> > // Nb: aDesc may have single-quotes in it.
> 
> Please escape attribute values properly instead of relying on them to not
> contain characters such as " and &.

Done.


> > Paths starting with "total" represent non-overlapping regions of memory.
> 
> This is a little confusing. "Total" really refers to non-overlapping regions
> of *explicitly allocated* memory, right?  Maybe "total" is not the best name.

I discussed with Jesse on IRC and we agreed leaving it as "total" was ok.


> > may overlap arbitrarily with regions in the "mapped" and "heap-used" trees.
> 
> This should be updated to refer to the new single tree.

Done.

 
> > Each one can be viewed as representing a path in a tree from
> > the root node ("total") to a node lower in the tree; 
> 
> I found this description a little confusing.  "More general" and "more
> specific" might be better than "higher" and "lower".
> 
> > this lower node may be a non-leaf or a leaf node.
> 
> What is this sentence trying to tell me?  Of course a tree has leaves and
> non-leaves.

It indicates that the path doesn't have to go all the way to a leaf node.  Eg. you can have "total/a" and "total/a/b".  I've clarified the comment.


> The more interesting things about this tree are (1) some intermediate nodes
> have reporters and some do not, and (2) in the final tree, each node's value
> will be equal to sum of the values of its children.  Maybe add something
> like:
> 
> "If an intermediate node does not have a reporter, a value will be shown
> based on the sum of its children. If an intermediate node does have a
> reporter, an 'other' child will be inserted with the difference from the sum
> of its children."

Strictly speaking, that's what about:memory does, but about:memory isn't the only user of these reporters.  Eg. some add-ons use them.  So I'll avoid discussing about:memory-specific things as much as possible here.


> > eg. by calling malloc
> 
> "e.g." here and in several other places

"eg." is a valid informal variant, eg. see http://en.wiktionary.org/wiki/e.g.


> > "This is the vsize figure as reported by 'top' or 'ps'; on Mac the amount "
> 
> Could use ifdefs to show different, detailed messages for each OS.
> 
> > // These must match nsIMemoryReporter.
> > const MR_MAPPED = 0;
> 
> How about:
> const MR_MAPPED = Components.interfaces.nsIMemoryReporter.MR_MAPPED;

I knew there had to be a way to do that :)  Fixed.


> > "This tree covers explicit memory allocations by the application, " +
> > "both at the operating system level (via calls to functions such as " +
> > "VirtualAlloc, vm_allocate, and mmap), and at the heap allocation level " +
> > "(via functions such as malloc, calloc, realloc, memalign, operator " +
> > "new, and operator new[]). It excludes memory mapped implicitly such as " +
> > "code and data segments, and thread stacks. It also excludes heap " +
> > "memory that has been freed by the application but is still being held " +
> > "onto by the heap allocator. Although it may not cover every explicit " +
> > "allocation, it does cover most and is the single best number to focus " +
> > "on when trying to reduce memory usage.";
> 
> It would be good to explain that it includes all heap allocations (annotated
> or not) and most other explicit allocations (because most of these are
> annotated). If you can figure out a way to do that without making the
> tooltip unreasonably long ;)

Looks like I posted a patch that was very slightly out of date.  I subsequently added "(including the entire heap)" after "it does cover most".

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: patch v2, addressing Jesse's comments

bsmedberg, the super-review issues here are:

- Most of the memory reporters have had their path changed (which you approved previously in bug 633653).

- nsIMemoryReporter has a new 'kind' field.

Comment 5

[Jesse Ruderman]

>   function escapeQuotes(aStr)
>   {
>     return aStr.replace(/'/g, ''');
>   }

You need to escape & (first), since it's the escape character.

Comment 6

[Nicholas Nethercote [inactive]]

(In reply to comment #5)
> >   function escapeQuotes(aStr)
> >   {
> >     return aStr.replace(/'/g, ''');
> >   }
> 
> You need to escape & (first), since it's the escape character.

You sure?  AFAICT it's working fine.

Comment 7

[Shawn Wilsher :sdwilsh]

Comment on attachment 532862 [details] [diff] [review]
patch v2, addressing Jesse's comments

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

r=sdwilsh

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: patch v3

I changed the name of the top-level reporter in the tree from "total" to "explicit", which is much better.  Jesse agreed with this improvement.  Little else changed, so I'm carrying forward sdwilsh's r+.

Super-review is still needed;  the super-review issues in comment 4 haven't changed.

Comment 9

[Nicholas Nethercote [inactive]]

bsmedberg: I'd really appreciate a super-review before the aurora merge on Tuesday.  This patch simplifies the output of about:memory and makes it more useful, so it would be great to get it into Firefox 6.  Thanks!

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 533477 [details] [diff] [review]
patch v3

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

Comment 11

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/mozilla-central/rev/a6b3a22fbca7

Comment 13

[[e-mail]]

Comment on attachment 533477 [details] [diff] [review]
patch v3

>-#define NS_MEMORY_REPORTER_IMPLEMENT(_classname,_path,_desc,_usageFunction,_dataptr) \
>+#define NS_MEMORY_REPORTER_IMPLEMENT(_classname,_path,_kind,_desc,_usageFunction,_dataptr) \
>     class MemoryReporter_##_classname : public nsIMemoryReporter {      \
>     public:                                                             \
>       NS_DECL_ISUPPORTS                                                 \
>       NS_IMETHOD GetPath(char **memoryPath) { *memoryPath = strdup(_path); return NS_OK; } \
>+      NS_IMETHOD GetKind(int *kind) { *kind = _kind; return NS_OK; } \
...
>+NS_IMETHODIMP nsMemoryReporter::GetKind(PRInt32 *aKind)
>+{
>+    *aKind = mKind;
>+    return NS_OK;
>+}
I think they should both be PRInt32*, otherwise at some point compilers are going to complain.