Closed Bug 674290 Opened 13 years ago Closed 13 years ago

Expose contents of /proc/self/maps and smaps in about:memory

Categories

(Toolkit :: about:memory, defect)

All
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

Details

(Whiteboard: [MemShrink:P2])

Attachments

(5 files, 9 obsolete files)

2.34 KB, text/plain
Details
16.85 KB, text/plain
Details
6.25 KB, text/plain
Details
52.08 KB, patch
n.nethercote
: review+
Details | Diff | Splinter Review
13.43 KB, patch
Details | Diff | Splinter Review
On newer Linux kernels, we can read /proc/self/maps and /proc/self/smaps to get a breakdown of every mapping the process has made, and how much is resident in memory.

This would be really helpful for accounting for the part of RSS that's in excess of explicit allocations, among other things.
Whiteboard: [MemShrink]
Are you proposing dumping the whole contents into about:memory, or parsing and presenting some summary info?  The latter seems much more reasonable;  if I want the former I can just view the file directly.  I've thought before about writing a script to summarize /proc/self/maps but never got around to it.
Definitely not |cat smaps >> about:memory| -- I agree that if you want a raw dump, you can just look at the file.  But I think there's lots of interesting information we could aggregate and display.
Here's a Perl script that parses /proc/<pid>/maps and prints out some summary info.  Some terminology:  I use "file" for lines with a pathname in them, like this:

    08048000-08056000 r-xp 00000000 03:0c 64593   /usr/sbin/gpm

I use "anon" for lines with no pathname in them, like this:

    08058000-0805b000 rwxp 00000000 00:00 0

And I use "other" for weird cases like this:

    7fffd449a000-7fffd44bb000 rw-p 00000000 00:00 0       [stack]

Here's the output for a running Firefox:

1,273,008,128 B (100.00%): vsize ()
----
 755,769,344 B (59.37%): anon  (rw-p) (heap, mmaps)
 291,147,776 B (22.87%): file  (---p) (padding)
  68,702,208 B (05.40%): file  (r-xp) (code)
  68,005,888 B (05.34%): file  (rw-s) (shared data)
  60,395,520 B (04.74%): file  (r--p) (rodata)
  26,718,208 B (02.10%): anon  (rwxp) (JIT code)
   1,294,336 B (00.10%): file  (rw-p) (data)
     528,384 B (00.04%): file  (r--s) (shared rodata)
     319,488 B (00.03%): other (rw-p) (other)
     118,784 B (00.01%): anon  (---p) (???)
       8,192 B (00.00%): other (r-xp) (other)

I'll attach about:memory shortly.

The classification on the RHS of each line is what I came up with, please tell me what I've got wrong.  Interesting that so much appears to be padding.
Things to note:

- The mjit-code and tjit-code entries add up to 26,456,064, which is close to the 26,718,208 "JIT code" line.

- 'explicit' is 500MB, well short of the 755MB under "heap, mmaps".  I'm not sure what that means.
(In reply to comment #3)
> Created attachment 549019 [details]
> Perl script to parse /proc/pid/maps
> 
> Here's a Perl script that parses /proc/<pid>/maps and prints out some
> summary info.  Some terminology:  I use "file" for lines with a pathname in
> them, like this:
> 
>     08048000-08056000 r-xp 00000000 03:0c 64593   /usr/sbin/gpm
> 
> I use "anon" for lines with no pathname in them, like this:
> 
>     08058000-0805b000 rwxp 00000000 00:00 0
> 
> And I use "other" for weird cases like this:
> 
>     7fffd449a000-7fffd44bb000 rw-p 00000000 00:00 0       [stack]
> 
> Here's the output for a running Firefox:
> 
> 1,273,008,128 B (100.00%): vsize ()
> ----
>  755,769,344 B (59.37%): anon  (rw-p) (heap, mmaps)
>  291,147,776 B (22.87%): file  (---p) (padding)
>   68,702,208 B (05.40%): file  (r-xp) (code)
>   68,005,888 B (05.34%): file  (rw-s) (shared data)
>   60,395,520 B (04.74%): file  (r--p) (rodata)
>   26,718,208 B (02.10%): anon  (rwxp) (JIT code)
>    1,294,336 B (00.10%): file  (rw-p) (data)
>      528,384 B (00.04%): file  (r--s) (shared rodata)
>      319,488 B (00.03%): other (rw-p) (other)
>      118,784 B (00.01%): anon  (---p) (???)
>        8,192 B (00.00%): other (r-xp) (other)
> 
> I'll attach about:memory shortly.
> 
> The classification on the RHS of each line is what I came up with, please
> tell me what I've got wrong.  Interesting that so much appears to be padding.

Really not surprising. When you build an ELF binary, it comes with two segments separated from each other. When the ELF binary is mapped in memory, to avoid any conflict with other mappings, the whole range, from the start of the lower segment to the end of the upper segment is mapped, and then each segment is mapped. The gap remains, but it's uncommitted memory. The gap is usually smaller on x86-64, btw.

Parsing smaps instead of maps could allow to distinguish committed vs. uncommitted memory, and could also allow to know how much of the private mappings are actually private (in practice, pages that you don't modify are shared).
(In reply to comment #4)
> - 'explicit' is 500MB, well short of the 755MB under "heap, mmaps".  I'm not
> sure what that means.

It likely means there's a whole lot of jemalloc arenas that are not quite full. We really need to instrument jemalloc for that...
> It likely means there's a whole lot of jemalloc arenas that are not quite 
> full. We really need to instrument jemalloc for that...

jemalloc tells us how much we have allocated and how much it has mapped.  I don't know if that's *right*...

In this case, it says mapped but unallocated is 34mb.  Some of this is committed, and some isn't.

So now we've accounted for 480mb of explicit (the 500 figure is mib) plus 34mb of jemalloc's mappings, or 514mb overall.  That leaves a lot to go...

Offhand, I wonder how much extra the js stack takes up.  According to the tooltip, js/stack is only the committed portion.
Oh, and there are probably the thread stacks in there, too.
(In reply to comment #7)
> So now we've accounted for 480mb of explicit (the 500 figure is mib)
Sorry to add noise to this bug, but is that a typo? MiB (MebiBytes) represent 1024^2 Bytes, where MB should represent 1000^2 Bytes, but is traditionally used to represent 1024^2 Bytes. So if the 500 figure is in MiB, that would be equivalent to ~524 MB in the SI sense, but I don't think that's what you mean.
Yes, I got my bibytes backwards; sorry for the confusion!
(In reply to comment #7)
> 
> Offhand, I wonder how much extra the js stack takes up.  According to the
> tooltip, js/stack is only the committed portion.

On Linux and Mac, the whole stack is committed.  Only on Windows can the stack be partially uncommitted.
> On Linux and Mac, the whole stack is committed.

It's committed in the sense that you can write to it without segfaulting, but it could be backed by copy-on-write zero pages, right?
Whiteboard: [MemShrink] → [MemShrink:P2]
Blocks: 636220
I'm working on getting this into about:memory.
Assignee: nobody → justin.lebar+bug
How will you do it?  You could parse it on the C++ side and report the numbers via the nsIMemoryMultiReporter interface, which would give you uniformity with the existing code.  Or you could parse it on the JS side with ad hoc code in aboutMemory.js, which would give you greater flexibility.  The latter is probably better.
I was planning to parse on the C++ side, since we might want to use the data collected there for other things, eventually, so it would be good to be able to query these reporters.
Would they be KIND_OTHER reports, thus going in the "other measurements" list?  You won't get percentages that way, which is a shame.
(In reply to Nicholas Nethercote [:njn] from comment #16)
> Would they be KIND_OTHER reports, thus going in the "other measurements"
> list?  You won't get percentages that way, which is a shame.

My week wouldn't be complete without rewriting about:memory, so I was going to make their path start with "map/" and add handling for that just like we now handle "explicit/".
Here's the non-verbose output of my WIP patch.  (I haven't fixed the negative numbers resulting from overcounting yet.)

700.97 MB (100.0%) -- map
├──343.73 MB (49.04%) -- [Anonymous] [97]
│  ├──343.73 MB (49.04%) -- Size [97]
│  ├───60.38 MB (08.61%) -- Rss [97]
│  ├───60.38 MB (08.61%) -- Pss [97]
│  ├───60.38 MB (08.61%) -- Private_Dirty [97]
│  ├───60.38 MB (08.61%) -- Referenced [97]
│  ├───60.38 MB (08.61%) -- Anonymous [97]
│  └──-301.88 MB (-43.07%) -- (8 omitted)
├──226.16 MB (32.26%) -- (164 omitted)
├───53.75 MB (07.67%) -- libxul.so [3]
│   ├──53.75 MB (07.67%) -- Size [3]
│   ├──33.86 MB (04.83%) -- Rss [3]
│   ├──33.86 MB (04.83%) -- Pss [3]
│   ├──33.86 MB (04.83%) -- Referenced [3]
│   ├──31.16 MB (04.44%) -- Private_Clean [3]
│   └──-132.73 MB (-18.94%) -- (9 omitted)
├───40.34 MB (05.75%) -- icon-theme.cache [2]
│   ├──40.34 MB (05.75%) -- Size [2]
│   └───0.00 MB (00.00%) -- (13 omitted)
├───20.65 MB (02.95%) -- [heap]
│   ├──20.65 MB (02.95%) -- Size
│   ├──20.11 MB (02.87%) -- Rss
│   ├──20.11 MB (02.87%) -- Pss
│   ├──20.11 MB (02.87%) -- Private_Dirty
│   ├──20.11 MB (02.87%) -- Referenced
│   ├──20.11 MB (02.87%) -- Anonymous
│   └──-100.53 MB (-14.34%) -- (8 omitted)
├────6.54 MB (00.93%) -- locale-archive
│    ├──6.54 MB (00.93%) -- Size
│    └──0.00 MB (00.00%) -- (13 omitted)
├────6.25 MB (00.89%) -- libgtk-x11-2.0.so.0.2400.4 [4]
│    ├──6.25 MB (00.89%) -- Size [4]
│    └──0.00 MB (00.00%) -- (13 omitted)
└────3.55 MB (00.51%) -- libc-2.13.so [4]
     ├──3.55 MB (00.51%) -- Size [4]
     └──0.00 MB (00.00%) -- (13 omitted)

Thinking out loud about this:

 * The verbose output doesn't need to show bytes, since all the numbers are multiples of 4KB.

 * What I really want to know when I see the data is: At each level of the hierarchy, including the root level, what's the total mapping size, RSS, PSS (private set size), and swap size?  (Maybe I'll care about some of the other numbers once I figure out what they mean.)

This is particularly important since many libraries which take up a lot of virtual space don't take up much RSS and take up even less PSS.

It would kind of make sense to put mapping size, RSS, PSS, etc. in separate trees, rather than stuff them all into one tree.  But that gives you a lot of trees, and it's also useful to be able to look at all the figures for one mapping.

 * There's a long tail.  Omitted mappings (less than 0.5% each?) make up 226mb total.  In the current scheme, when we collapse the long tail into a single node, we also need to collapse their children.  So instead of just saying "(omitted) [one bajillion]", the "(omitted)" node should have children summarizing the omitted nodes' children.

 * We should impose a hierarchy where we can.  For instance, there are a bunch of mappings like

     d52a8644073d54c13679302ca1180695-le64.cache-3

   which come from /var/cache/fontconfig. We could either collapse them into a single node or place them individually under a "/map/fontconfig" node.

[1] http://hg.mozilla.org/build/buildbot-configs/file/a82620789557/mozilla2/linux64/mozilla-2.0/release/mozconfig
Maybe if I had different trees for virtual bytes/RSS/PSS/swap, etc and let you click to open/collapse them, that could help?  Unfortunately that doesn't scale well to the verbose case.
Wow, you're being more ambitious than I expected.

Some thoughts:

- I'd be interested to see the patch.  Are you using nsIMemoryMultiReporter?

- Bug 674158's patch refactors aboutMemory.js a bit, which will probably conflict with your changes :(

- I almost always view about:memory in verbose mode.  One nice property it currently has is that its length is roughly proportional to the number of tabs open.  With the "maps" tree this would no longer be true -- the "maps" tree would be huge immediately after start-up.  So presentation is key here.

- There are a lot of repeated numbers.  You have 14 numbers per segment?  Surely not all of them are worth reporting, even in verbose mode?

- In bug 472209, Jez is implementing a graph in about:memory, and it can consult all the memory reporters frequently, e.g. once per second.  He currently only graphs the 'explicit' measurements, and these /proc/self/maps ones wouldn't go into that graph, so if we're parsing /proc/self/maps every time that would be wasted effort.

- How do you identify "[heap]" memory?
[heap] is something that smaps is giving me.  I'm not really sure what it is yet.
Attached patch WIP v1 (comment 18) (obsolete) — Splinter Review
Comment on attachment 551176 [details] [diff] [review]
WIP v1 (comment 18)

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

The code so far looks exactly as I'd expect, which is good.  I'll apply this patch and try browsing with it next week to get a feel for the output.
(In reply to Justin Lebar [:jlebar] from comment #21)
> [heap] is something that smaps is giving me.  I'm not really sure what it is
> yet.

This is what the Linux kernel source says about it:
if (!name) {
  (...)
  if (vma->vm_start <= mm->brk && vma->vm_end >= mm->start_brk) {
    name = "[heap]";
  }

Which means any anonymous mapping inside the boundaries defined by brk() and sbrk() is called [heap].
I'm running with this patch.  Here's example output I'll use in my comments:

├─────32,006,144 B (03.08%) -- libxul.so [4]
│     ├──32,006,144 B (03.08%) -- Size [4]
│     ├──20,660,224 B (01.99%) -- Rss [4]
│     ├──20,660,224 B (01.99%) -- Referenced [4]
│     ├──17,035,264 B (01.64%) -- Pss [4]
│     ├──10,948,608 B (01.05%) -- Private_Clean [4]
│     ├───7,249,920 B (00.70%) -- Shared_Clean [4]
│     ├───2,461,696 B (00.24%) -- Private_Dirty [4]
│     ├───2,461,696 B (00.24%) -- Anonymous [4]
│     ├──────16,384 B (00.00%) -- KernelPageSize [4]
│     ├──────16,384 B (00.00%) -- MMUPageSize [4]
│     ├───────────0 B (00.00%) -- Shared_Dirty [4]
│     ├───────────0 B (00.00%) -- Swap [4]
│     ├───────────0 B (00.00%) -- Locked [4]
│     └──-81,510,400 B (-7.84%) -- other


Thoughts:

- Wow, there's a *lot* of output, too much IMO.  Maybe just showing "size" and "RSS" for each entry would be enough;  if someone wants to dig deeper they can consult the actual /proc/self/maps file directly.

- The top node of the 'maps' tree doesn't match vsize.  Is this because of the negative entries?

- "heap-unclassified" is computed for the 'maps' tree, it clearly shouldn't be.

- When you have a non-leaf reporter, R, about:memory expects that all R's children will sum to less than or equal to R.  Oh, and a lot of the children overlap, and about:memory assumes they don't.  These two things explain the negative entries.

- You're not distinguishing between segments with the same name but different permissions.  To me this is one of the most interesting distinctions;  eg. compare the size of libxul's code segment to its data segment.

I realize the patch is in an early state and you probably have plans to fix most of this stuff already :)
> I realize the patch is in an early state and you probably have plans to fix 
> most of this stuff already :)

Yes.  :)

RSS and vsize are the two big ones.  But Firefox loads a bunch of shared libraries (libc, gnome, etc.), and I think it'd be helpful to show PSS so we can distinguish between shared and private memory.  I also would like to show what's swapped out, although I presume this tree will usually be small or empty.

I think I'm going to try making separate trees for each of these, see what it looks like, and iterate from there.  In non-verbose mode, using separate trees should let us summarize the data more usefully than if we stuffed everything into one tree.
Attached patch WIP v2 (obsolete) — Splinter Review
Attachment #551176 - Attachment is obsolete: true
Attached patch WIP v3 (obsolete) — Splinter Review
Attachment #551659 - Attachment is obsolete: true
Comment on attachment 551668 [details] [diff] [review]
WIP v3

Still need to update the about:memory test and write some descriptions, but Nick, what do you think of the format?

What stands out to me is that PSS is largely the same as RSS; the only real differences are, surprise surprise, in shared libraries.

I'm leaning towards not showing PSS, or showing only the total but hiding the tree.  We could add a ?pss flag to about:memory without much trouble, but I'm not sure it would be helpful.
Attachment #551668 - Flags: feedback?(nnethercote)
That's much better!

Yes, PSS isn't adding much.

about:memory?verbose is still really long.  Hmm.  I suspect we should not show RSS and virtual by default, only show them with e.g. about:memory?rss?virtual.  (Then you could show PSS.)

I found the "anonymous, outside data segment" description odd, as we discussed on IRC.  That use of "data segment" is weird to me.

I haven't looked at the code at all.

You're not grouping different kinds of segments together. E.g.:

│   ├──23,523,328 B (07.46%) -- shared-libraries-mozilla
│   │  ├──18,333,696 B (05.81%) -- libxul.so [r-xp]
│   │  ├───2,068,480 B (00.66%) -- libxul.so [r--p]
│   │  ├─────626,688 B (00.20%) -- libnss3.so [r-xp]
│   │  ├─────462,848 B (00.15%) -- libmozsqlite3.so [r-xp]

I kind of want to know what the sum of all the r-xp (code) segments is here.  Like in comment 3.  Not sure if that can be done in a reasonable way.
> I kind of want to know what the sum of all the r-xp (code) segments is here.  
> Like in comment 3.  Not sure if that can be done in a reasonable way.

Yeah, I'm not sure what to do here.  I experimented grouping first by library name and then by permissions, e.g.

  42 libxul.so
   |-- 24 libxul.so [r-xp]
   |-- 18 libxul.so [r--p]

but that added a lot of depth to the tree.

We could certainly sum all [r-xp], etc. reporters and put that in "other".  If you want to sum at each level of the tree, that might be OK...
Attachment #551668 - Flags: feedback?(nnethercote)
Ah, PSS isn't "private" but is "proportional" set size.

 * Proportional Set Size(PSS): my share of RSS.
 *
 * PSS of a process is the count of pages it has in memory, where each
 * page is divided by the number of processes sharing it.  So if a
 * process has 1000 pages all to itself, and 1000 shared with one other
 * process, its PSS will be 1500.
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #551785 - Flags: review?(nnethercote)
Attachment #551668 - Attachment is obsolete: true
Attached patch Patch v1.1 (obsolete) — Splinter Review
Fixing a mistake in the Makefile.
Attachment #551827 - Flags: review?(nnethercote)
Attachment #551785 - Attachment is obsolete: true
Attachment #551785 - Flags: review?(nnethercote)
Comment on attachment 551827 [details] [diff] [review]
Patch v1.1

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

I think this is a good thing, and the code quality looks good.  But I'm going to be a PITA and r- it because I don't think the new trees should be in about:memory by default :/  Reasons:

(a) In verbose mode, they're just too long.  They make the page *much* longer at startup.
(b) These trees are less generally useful than the 'explicit' tree.
(c) They're only implemented on Linux, so having a default uniform about:memory view that's the same across platforms is good.  (This is admittedly a minor point.)

So I'd like you to add links at the bottom like the "less verbose"/"more verbose" ones to turn on/off these trees.

Other minor things:

- The "resident" and "vsize" numbers in the "other measurements" list don't quite match those at the top of the trees, presumably because the measurements are taken at slightly different times.  It's probably not worth trying to fix, but maybe mention it in the tool-tip descriptions?

- The tool-tip descriptions for "resident" and "vsize" are different in the "other measurements" list and the trees.  Perhaps they should be the same?

- I can imagine reading this output and then wanting to consult the actual /proc/pid/smaps file.  If you printed the pid somewhere (in the tree heading?) that would make it easier.  Actually, that's arguably useful even if these trees aren't shown, so we could put it into the "Main Process" header (though test_aboutmemory.xul would need some tweaking).  There is precedent for this, we show the pid for the content process in Fennec.  (Hmm, we'd need to get the pid in aboutMemory.js, and I don't know how to do that.)

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +81,5 @@
> +
> +  'swap':
> +    "This tree shows how much space in the swap file each of the process's " +
> +    "mappings is currently using. Mappings which are not in the swap file " +
> +    "(i.e., nodes which would have a value of 0 in this tree) are omitted.",

I didn't see "swap" in about:memory -- did you remove it?

(Oh, am I not seeing it because my machine has 16GB of RAM and so never uses swap space?)

@@ +311,3 @@
>  {
>    assert(a._units === undefined && b._units === undefined,
> +         "'explicit' and 'map' tree nodes must not have _units defined");

What is 'map'?  Maybe just future-proof this comment by saying "Tree nodes must...".

(Oh, I see later you have map/vsize, map/rss, etc.  Is that necessary?  It's not clear to me this is better than rooting them at vsize, rss, etc.  If they're shown as separate trees in about:memory, it makes sense for them to their paths to be separate.)

@@ +397,5 @@
>    // treeName at the root.
>    t = t._kids[0];
>  
> +  if (!t) {
> +    // No elements in this tree!  Just bail.

Can you give an example of why that might be (e.g. the swap case).  Hmm, once the trees are chosen via links (or the URL) it would look like a bug if I asked for the "swap" tree and it didn't show up.  So we should probably show a tree that has zero bytes at its root.  (The threshold percentage calculation will have to be careful to avoid div-by-zero for such a tree.)

@@ +460,4 @@
>    fillInTree(t, "");
>  
> +  // Reduce the depth of the tree by the number of occurrences of '/' in
> +  // treeName.  (Thus the tree named 'foo/bar/baz' will be rooted at 'baz'.)

You wouldn't need this if you removed the "map/" prefix.

@@ +469,5 @@
> +    }
> +  }
> +
> +  // Set the description on the root node.
> +  t._description = treeDescriptions[t._name];

This is nice.  With this in place we don't need the tool-tip on the tree headings (e.g. "Explicit Allocations"), can you remove them?  The tree headings also don't need to be underlined when you hover the mouse over them.

@@ +930,5 @@
>      perc = "<span class='mrPerc'>(" + perc + "%)</span> ";
>  
> +    // We don't want to show '(nonheap)' on a tree like map/vsize, since
> +    // the whole tree is non-heap.
> +    var kind = isExplicitTree ? aT._kind : null;

In bug 674158 (which will conflict somewhat with this, sorry) I did something similar, but allowed "undefined" instead of "null".

::: toolkit/components/aboutmemory/tests/chrome/test_aboutmemory.xul
@@ +115,5 @@
> +          f("map/swap/a",       1);
> +          f("map/swap/a",       2);
> +          f("map/vsize/a",      19);
> +          f("map/swap/b/c",     10);
> +          f("map/rss/a",        42);

Thanks for augmenting the test :)

@@ +377,5 @@
> +      dump("*******ACTUAL*******\n");
> +      dump(a);
> +      dump("******EXPECTED******\n");
> +      dump(expected);
> +      dump("********************\n");

Yeah, good idea.

::: xpcom/base/MapsMemoryReporter.cpp
@@ +36,5 @@
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +#include "mozilla/MapsMemoryReporter.h"

All the .cpp functions in xpcom/base/ bar one has "ns" at the start of their name.  Should this one do likewise?

@@ +54,5 @@
> +// mozillaLibraries is a list of all the shared libraries we build.  This list
> +// is used for determining whether a library is a "Mozilla library" or a
> +// "third-party library".  But even if this list is missing items, about:memory
> +// will identify a library in the same directory as libxul.so as a "Mozilla
> +// library".

Nice comment.

@@ +76,5 @@
> +};
> +
> +namespace {
> +
> +bool EndsWithLiteral(const nsCString &aHaystack, const char *aNeedle)

Any reason this isn't within MapsReporter?  Likewise for GetDirname and GetBasename.

@@ +148,5 @@
> +  nsCString mLibxulDir;
> +  nsCStringHashSet mMozillaLibraries;
> +};
> +
> +NS_IMPL_THREADSAFE_ISUPPORTS1(MapsReporter, nsIMemoryMultiReporter)

Is the THREADSAFE necessary?

@@ +158,5 @@
> +  for (PRUint32 i = 0; i < len; i++) {
> +    nsCAutoString lib;
> +    lib.Assign(mozillaLibraries[i]);
> +    mMozillaLibraries.Put(lib);
> +  }

Is there a reason you can't use NS_LITERAL_STRING to define a static array of nsCStrings within MapsReporter, thus avoiding this dance?

@@ +184,5 @@
> +MapsReporter::FindLibxul()
> +{
> +  mLibxulDir.Truncate();
> +
> +  FILE *f = fopen("/proc/self/maps", "r");

Maybe add a comment emphasizing that this scans |maps|, but the other stuff scans |smaps|.  I didn't realize that at first and wondered why you opened the same file twice.

@@ +222,5 @@
> +{
> +  // We need to use native types in order to get good warnings from fscanf, so
> +  // let's make sure that the native types have the sizes we expect.
> +  PR_STATIC_ASSERT(sizeof(long long) == sizeof(PRInt64));
> +  PR_STATIC_ASSERT(sizeof(int) == sizeof(PRInt32));

Nice comment and checking!

@@ +295,5 @@
> +  nsCAutoString basename;
> +  GetBasename(absPath, basename);
> +
> +  if (basename.EqualsLiteral("[heap]")) {
> +    aName.Append("[Anonymous]/anonymous, within brk()");

You leave [vdso] and [stack] unchanged, so why not do the same for [heap]?

The "[Anonymous]" looks odd, anything wrong with just "anonymous"?

I'll leave the final naming up to you, but I'd personally just use "anonymous/[heap]" here and "anonymous/anonymous" for the outside-brk ones.

@@ +305,5 @@
> +  }
> +  else if (basename.EqualsLiteral("[stack]")) {
> +    aName.Append("[stack]");
> +    aDesc.Append("This corresponds to '[stack]' in /proc/self/smaps; it appears "
> +                 "that this node measures the stack size only of the process's "

Drop the "it appears that";  I'm pretty sure that's correct.

@@ +345,5 @@
> +      }
> +    }
> +
> +    aName.Append(basename);
> +    aDesc.Append("Memory mapped for file ");

"Memory mapped for file" is accurate for the "vsize" tree, but isn't really right for the "rss" tree.  Something like "Memory in physical memory for file" would be better.  And the "swap" tree would need something else again.

@@ +408,5 @@
> +
> +  char desc[1025];
> +  unsigned long long size;
> +  if (fscanf(aFile, "%1024[a-zA-Z_]: %llu kB\n",
> +             desc, &size) != argCount) {

Should desc's length be a bit more than 1025?  More like 1040 or something?

@@ +421,5 @@
> +  if (strcmp(desc, "Size") == 0) {
> +    category = "vsize";
> +  }
> +  else if (strcmp(desc, "Rss") == 0) {
> +    category = "rss";

The "vsize" tree matches the name of the "vsize" entry in "other measurements", but the "rss" tree doesn't match "resident" in "other measurements".  We should be consistent in the names used within the page, so please change "rss" to "resident".  You can keep the tree heading unchanged as "Resident Set Size (RSS)" if you like, that doesn't bother me.

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +625,5 @@
> +        rv = r->GetPath(path);
> +        NS_ENSURE_SUCCESS(rv, rv);
> +
> +        // We're only interested in NONHEAP explicit reporters and
> +        // heap-allocated.

Can you put quotes around "heap-allocated"?  It took me a couple of attempts to parse that sentence.
Attachment #551827 - Flags: review?(nnethercote) → review-
(In reply to Nicholas Nethercote [:njn] from comment #35)
> I think this is a good thing, and the code quality looks good.  But I'm
> going to be a PITA and r- it because I don't think the new trees should be
> in about:memory by default :/  Reasons:
> 
> (a) In verbose mode, they're just too long.  They make the page *much*
> longer at startup.
> (b) These trees are less generally useful than the 'explicit' tree.
> (c) They're only implemented on Linux, so having a default uniform
> about:memory view that's the same across platforms is good.  (This is
> admittedly a minor point.)
> 
> So I'd like you to add links at the bottom like the "less verbose"/"more
> verbose" ones to turn on/off these trees.

I agree with your reasons, but I'm not a huge fan of adding maps-on / maps-off
in the same way we have verbose-on / verbose-off.

What if the trees were collapsible?  You'd click on "vsize", "swap", "RSS" to
expand that tree.

> ::: toolkit/components/aboutmemory/content/aboutMemory.js
> @@ +81,5 @@
> > +
> > +  'swap':
> > +    "This tree shows how much space in the swap file each of the process's " +
> > +    "mappings is currently using. Mappings which are not in the swap file " +
> > +    "(i.e., nodes which would have a value of 0 in this tree) are omitted.",
> 
> I didn't see "swap" in about:memory -- did you remove it?
> 
> (Oh, am I not seeing it because my machine has 16GB of RAM and so never uses
> swap space?)

Yeah, I'm not showing it because nothing is swapped out.  But I agree that's
confusing; I'll fix this.

> 
> @@ +311,3 @@
> >  {
> >    assert(a._units === undefined && b._units === undefined,
> > +         "'explicit' and 'map' tree nodes must not have _units defined");
> 
> What is 'map'?  Maybe just future-proof this comment by saying "Tree nodes
> must...".
> 
> (Oh, I see later you have map/vsize, map/rss, etc.  Is that necessary?  It's
> not clear to me this is better than rooting them at vsize, rss, etc.  If
> they're shown as separate trees in about:memory, it makes sense for them to
> their paths to be separate.)

I kind of like having them grouped under a common root because that lets us
apply operations to all the map trees at once, without maintaining a list of
them.

For example, we can say that map trees appear after all the other trees in
about:memory, or that only map trees are collapsible.

> @@ +295,5 @@
> > +  nsCAutoString basename;
> > +  GetBasename(absPath, basename);
> > +
> > +  if (basename.EqualsLiteral("[heap]")) {
> > +    aName.Append("[Anonymous]/anonymous, within brk()");
> 
> You leave [vdso] and [stack] unchanged, so why not do the same for [heap]?

I guess [stack] has the same problem as [heap], which is that neither of them
is actually what it says it is.  So I'll change [stack] too.  :)

> @@ +408,5 @@
> > +
> > +  char desc[1025];
> > +  unsigned long long size;
> > +  if (fscanf(aFile, "%1024[a-zA-Z_]: %llu kB\n",
> > +             desc, &size) != argCount) {
> 
> Should desc's length be a bit more than 1025?  More like 1040 or something?

Don't like living on the edge?  :)

If 1025 isn't big enough, how do you know that 1040 is?
Attached patch Patch v2 (v1.1, rebased) (obsolete) — Splinter Review
Attachment #551827 - Attachment is obsolete: true
Attached patch Patch v3 (obsolete) — Splinter Review
Attachment #555114 - Flags: review?(nnethercote)
Attachment #554993 - Attachment is obsolete: true
Comment on attachment 555114 [details] [diff] [review]
Patch v3

This addresses all the review comments except hiding the reporters.

Instead, I went for the low-tech solution of moving them below explicit / other.  I don't find that they get in the way any longer.  What do you think, Nick?
Attachment #555114 - Attachment description: Patch v2 → Patch v3
> All the .cpp functions in xpcom/base/ bar one has "ns" at the start of their 
> name.  Should this one do likewise?

I think this is the new style now that we're using namespaces.  The header is included as |#include "mozilla/MapsMemoryReporter.h"|, so the "ns" doesn't do us much good.
I wanted to try this out but it's bit-rotted and fails to apply, probably due to bug 674158.  Can you update, please?
Attached patch Patch v3.1 (v3 rebased) (obsolete) — Splinter Review
I didn't get any merge conflicts updating this patch, so I'm kind of surprised it didn't work for you.
Attachment #555114 - Attachment is obsolete: true
Attachment #555114 - Flags: review?(nnethercote)
Attachment #555603 - Flags: review?(nnethercote)
Comment on attachment 555114 [details] [diff] [review]
Patch v3

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

The code looks good, thanks for the fixes.  But I'm still not happy about showing the new stuff by default :(  Moving it after "other measurements" is worse, because at least previously I could skip to it by hitting the "end" key.  Making the new trees collapsable sounds ok, as long as cut and paste still works, though I'm not sure exactly how you'd do it.

You'll need super-review for this, BTW.  You can ask for that independently while we hash out the show-by-default issue.

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +57,5 @@
>  const UNITS_PERCENTAGE = Ci.nsIMemoryReporter.UNITS_PERCENTAGE;
>  
>  const kUnknown = -1;    // used for _amount if a memory reporter failed
>  
> +const treeDescriptions = {

This should be called "kTreeDescriptions".  Likewise for several other global constants.
Attachment #555603 - Flags: review?(nnethercote) → review-
Comment on attachment 555603 [details] [diff] [review]
Patch v3.1 (v3 rebased)

What do you think I need SR on?  I'm not changing any interfaces, beyond cosmetic changes to nsIMemoryReporter...

In any case, dmandelin mentioned in m.d.platform that he was interested in this bug; do you mind taking a look, David?

I'll think more about how to hide the trees.
Attachment #555603 - Flags: superreview?(dmandelin)
(In reply to Justin Lebar [:jlebar] from comment #44)
> 
> What do you think I need SR on?  I'm not changing any interfaces, beyond
> cosmetic changes to nsIMemoryReporter...

You've got whole new files in there... maybe it doesn't need super-review, but you should get a 2nd review from someone who knows Gecko better than me, for all the Gecko stuff.
> Making the new trees collapsable sounds ok, as long as cut and paste still works, though I'm not 
> sure exactly how you'd do it.

By "cut and paste works", do you mean copy-paste should only include the visible trees, that it should include all visible and hidden trees?
Comment on attachment 555603 [details] [diff] [review]
Patch v3.1 (v3 rebased)

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

I don't think there's anything for me to SR here. I wouldn't mind looking at a snapshot of the output if you have one handy. Otherwise I can just check it out in nightlies and come back if I see anything noteworthy.
Attachment #555603 - Flags: superreview?(dmandelin)
Attached patch Patch v4 (obsolete) — Splinter Review
Attachment #555603 - Attachment is obsolete: true
Comment on attachment 555865 [details] [diff] [review]
Patch v4

njn, is this like what you had in mind?  Does it look and feel right?  Did you want copy-paste to always copy the whole thing?
Attachment #555865 - Flags: feedback?(nnethercote)
One thing I still want to experiment with is giving some indication of whether swap is empty.  You shouldn't have to click just to see that it's empty.  Maybe I can put bytes used in the header or something.
(In reply to Justin Lebar [:jlebar] from comment #46)
> 
> By "cut and paste works", do you mean copy-paste should only include the
> visible trees, that it should include all visible and hidden trees?

Sorry, I should have been clearer.  I want copy-paste to produce output that matches whatever you see in about:memory.  I'll take a look at the updated patches later today.
Comment on attachment 555865 [details] [diff] [review]
Patch v4

Hey, that's pretty slick, I like it!

Having to click on "swap" to see it's zero doesn't worry me.

I'm not sure about the dotted lines under the headings... I understand you're trying to communicate that there's an action to be done, but it's a bit ugly.  Maybe just solid underlining (regardless of whether you're hovering over it)?

Also, we already have a sentence at the bottom about hovering over reporter names, it be good to add another sentence about clicking on headings to expand them.
Attachment #555865 - Flags: feedback?(nnethercote) → feedback+
How's this going? It looks like there's a patch that's considered just about ready. (I want this really badly!)
(In reply to David Mandelin from comment #54)
> How's this going? It looks like there's a patch that's considered just about
> ready. (I want this really badly!)

Sorry, I've been distracted by TArrays!  I'll bump this up in priority.

> Maybe just solid underlining (regardless of whether you're hovering over it)?

The problem is that underline already means "I have a tooltip" in about:memory.  I guess it's true that these headings will have tooltips, but we're overloading the meaning here.

I'll try it and see how it looks.  I shouldn't keep this from landing just because I'm not wild about the underlines!
Attached patch Patch v5Splinter Review
Attachment #557932 - Flags: review?(nnethercote)
Attachment #555865 - Attachment is obsolete: true
Comment on attachment 557932 [details] [diff] [review]
Patch v5

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

r=me!  Thanks for persevering with this, I think it'll be a useful feature.

One big thing is that it doesn't appear to work properly for Fennec, at least for a Linux64 desktop build of it that I just tried -- the RSS/vsize/swap trees show up for the chrome process, but they're empty for the content process.  The code responsible for communicating content process results is in dom/ipc/ContentChild.cpp;  the code responsible for receiving them is in dom/ipc/ContentParent.cpp (look for "MemoryReport" in both files).  Basically, ContentChild.cpp just iterates over all reporters and stuffs their results into an array that gets sent to the parent, which unpacks that array and temporarily registers some static reporters.  (This is a bit hacky -- I didn't implement it! -- and bug 673323 has a design sketch for proper async memory reporters.)

I'm not sure why the RSS/vsize/swap trees are empty, I did a little bit of debugging and found that MapsReporter::Init is being called in both processes, but didn't get any further.

I'm happy to r+ this without Fennec working if there's a follow-up bug, but it would be better if you could fix it as part of this bug.  Hopefully it'll be something simple.

Other minor things:
- I'd prefer having "Other Measurements" at the end, because then we have all the tree views together, with the list last.

- Oh, we still get a tool-tip when hovering over the "Other Measurements" heading.  I guess it's hard to avoid that inconsistency because there's no tree root node to apply the description to.

- I like the solid underlining of the headings.  It's different to the underlining of individual reporters because it's always on, instead of only appearing when hovering.

- If you accidentally click a long way to the right of a heading, the expand/collapse still triggers.  This is surprising but I guess it's hard to avoid and I can live with it.  (Actually, it makes me wonder if we should replace the "text-decoration: underline" on those h2 elements with something like "background-color: #ddd".  I just tried that and I think it works quite well, but I'll let you decide.)

::: toolkit/components/aboutmemory/content/aboutMemory.css
@@ +71,5 @@
>  
> +h2 {
> +  cursor: pointer;
> +  text-decoration: underline;
> +}

Nit: I think it would be nicer to have an actual class, instead of blanket-applying this to all h2 elements.

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +100,5 @@
> +const kTreeNames = {
> +  'explicit': 'Explicit Allocations',
> +  'resident': 'Resident Set Size (RSS) breakdown',
> +  'vsize':    'Virtual Size breakdown',
> +  'swap':     'Swap file usage breakdown'

Nit: Capitalize the first letter of every word for consistency in the headings, please.

@@ +185,5 @@
>  
> +function toggleTreeVisibility(aEvent)
> +{
> +  var headerElem = aEvent.target;
> +  var treeElem = $(headerElem.id.replace(/^header-/, 'pre-'));

I confess to not understanding this line.

@@ +188,5 @@
> +  var headerElem = aEvent.target;
> +  var treeElem = $(headerElem.id.replace(/^header-/, 'pre-'));
> +
> +  headerElem.classList.toggle('collapsed');
> +  treeElem.classList.toggle('collapsed');

Why does the headerElem need to be toggled?

@@ +381,5 @@
>    // traversal.
> +
> +  // Is there any reporter which matches aTreeName?  If not, we'll create a
> +  // dummy one.  At the same time, we'll copy aReporters so we don't have to
> +  // modify it.

One thing I'm aiming for in about:memory is for the generation of the page itself to use the minimum amount of memory possible, so as to not perturb the results too much.  (Esp. with an eye towards bug 472209, which can cause buildTree to be called frequently.)

Copying the reporters doesn't help with this aim.  Is it really necessary?  It'd be good to remove it if possible.

@@ +935,5 @@
>  
>    var text = genTreeText2(aT, [], rootStringLength);
> +  var headerId = 'header-tree-' + aT._name;
> +  var preId = 'pre-tree-' + aT._name;
> +  var elemClass = isExplicitTree ? '' : 'collapsed';

Nit: a comment here like "By default, the explicit tree is expanded and the other trees are collapsed." would help.

@@ +1025,5 @@
> +  return "<h2 id='header-tree-other' " +
> +          "onclick='toggleTreeVisibility(event)' title='" + desc + "'>" +
> +            "Other Measurements" +
> +         "</h2>\n" +
> +         "<pre id='pre-tree-other'>" + text + "</pre>\n";

Nit: The "tree" in "header-tree-other" and "pre-tree-other" isn't accurate here :)

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +665,5 @@
>        nsCOMPtr<nsIMemoryMultiReporter> r;
>        e2->GetNext(getter_AddRefs(r));
>        r->CollectReports(cb, wrappedMRs);
>      }
>  

There's a comment a few lines lower "his is quadratic in the number of NONHEAP reporters" which I think should be changed to "number of explicit NONHEAP reporters".
Attachment #557932 - Flags: review?(nnethercote) → review+
inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/6cd3556fc807
Whiteboard: [MemShrink:P2] → [MemShrink:P2][inbound]
Backed out because of leaks in Linux debug builds:
http://hg.mozilla.org/integration/mozilla-inbound/rev/d01a282b5a40

e.g. https://tbpl.mozilla.org/php/getParsedLog.php?id=6303345
Rev3 Fedora 12 mozilla-inbound debug test mochitests-5/5 on 2011-09-06 20:20:07 PDT for push 6cd3556fc807
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 120 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 120 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 15 instances of nsStringBuffer with size 8 bytes each (120 bytes total)
Whiteboard: [MemShrink:P2][inbound] → [MemShrink:P2]
The leak is due to

const nsCString mozillaLibraries[] =
{
  NS_LITERAL_CSTRING("libfreebl3.so"),
  NS_LITERAL_CSTRING("libmozalloc.so"),
  NS_LITERAL_CSTRING("libmozsqlite3.so"),
  etc.

There are 15 NS_LITERAL_CSTRING's in all.  Guess I'll go back to using C strings here.
Inbound with the leak fixed: http://hg.mozilla.org/integration/mozilla-inbound/rev/45b321b68c2f
Whiteboard: [MemShrink:P2] → [MemShrink:P2][inbound]
http://hg.mozilla.org/mozilla-central/rev/45b321b68c2f
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [MemShrink:P2][inbound] → [MemShrink:P2]
Target Milestone: --- → mozilla9
Depends on: 685870
Blocks: 686172
Blocks: 686720
Depends on: 688233
Depends on: 693101
You need to log in before you can comment on or make changes to this bug.