Last Comment Bug 633653 - (revampaboutmemory) revamp about:memory
(revampaboutmemory)
: revamp about:memory
Status: VERIFIED FIXED
: dev-doc-complete
Product: Toolkit
Classification: Components
Component: about:memory (show other bugs)
: unspecified
: All All
: -- normal with 7 votes (vote)
: mozilla6
Assigned To: Nicholas Nethercote [:njn]
:
Mentors:
: 525288 (view as bug list)
Depends on: 655170 656773
Blocks: DarkMatter 585196 MemShrinkTools 648411 648415 649867 649881 653630 654041 654917
  Show dependency treegraph
 
Reported: 2011-02-11 16:11 PST by Jeff Muizelaar [:jrmuizel]
Modified: 2011-07-12 07:46 PDT (History)
65 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
WIP patch (against TM 64136:0477bca4863d) (81.98 KB, patch)
2011-03-31 22:53 PDT, Nicholas Nethercote [:njn]
no flags Details | Diff | Review
screenshot for WIP patch (95.13 KB, image/png)
2011-03-31 22:58 PDT, Nicholas Nethercote [:njn]
no flags Details
patch v2 (against m-c 64649:4e4c7457e8f7) (86.70 KB, patch)
2011-04-04 22:31 PDT, Nicholas Nethercote [:njn]
no flags Details | Diff | Review
v2 screenshot (83.48 KB, image/png)
2011-04-04 22:34 PDT, Nicholas Nethercote [:njn]
no flags Details
patch v3 (against m-c 64649:4e4c7457e8f7) (94.52 KB, patch)
2011-04-06 21:33 PDT, Nicholas Nethercote [:njn]
no flags Details | Diff | Review
v3 screenshot (101.97 KB, image/png)
2011-04-06 21:34 PDT, Nicholas Nethercote [:njn]
no flags Details
patch v4 (against m-c 64649:4e4c7457e8f7) (99.87 KB, patch)
2011-04-08 00:15 PDT, Nicholas Nethercote [:njn]
no flags Details | Diff | Review
v4 screenshot (104.57 KB, image/png)
2011-04-08 00:19 PDT, Nicholas Nethercote [:njn]
no flags Details
patch v5, minus JS + xpconnect + NJ changes (against m-c 67803:09b605eb3e0d) (57.73 KB, patch)
2011-04-11 22:19 PDT, Nicholas Nethercote [:njn]
no flags Details | Diff | Review
patch v5, JS + xpconnect parts (37.24 KB, patch)
2011-04-11 22:21 PDT, Nicholas Nethercote [:njn]
dvander: review+
Details | Diff | Review
patch v5, nanojit changes (4.21 KB, patch)
2011-04-11 22:22 PDT, Nicholas Nethercote [:njn]
edwsmith: review+
Details | Diff | Review
v5 screenshot, verbose mode (119.64 KB, image/png)
2011-04-11 22:28 PDT, Nicholas Nethercote [:njn]
no flags Details
patch v5b, minus JS + xpconnect + NJ changes (against m-c 67803:09b605eb3e0d) (57.73 KB, patch)
2011-04-11 22:30 PDT, Nicholas Nethercote [:njn]
no flags Details | Diff | Review
patch v5c, minus JS + xpconnect + NJ changes (against m-c 67803:09b605eb3e0d) (58.08 KB, patch)
2011-04-12 17:16 PDT, Nicholas Nethercote [:njn]
vladimir: review+
sdwilsh: review+
sdwilsh: feedback+
Details | Diff | Review
v5c screenshot (73.52 KB, image/png)
2011-04-12 17:19 PDT, Nicholas Nethercote [:njn]
no flags Details
patch v6 (against m-c 68692:07de8acf5d7a) (116.27 KB, patch)
2011-04-29 02:07 PDT, Nicholas Nethercote [:njn]
ehsan: review-
Details | Diff | Review
patch, v7, just the chrome test (against m-c 68827:af860093a19a) (13.72 KB, patch)
2011-05-01 19:48 PDT, Nicholas Nethercote [:njn]
ehsan: review+
Details | Diff | Review
patch v7, minus JS changes (82.83 KB, patch)
2011-05-01 19:52 PDT, Nicholas Nethercote [:njn]
benjamin: superreview+
Details | Diff | Review
patch v8, ready to land (against m-c 68895:86248f7209b7) (119.72 KB, patch)
2011-05-02 20:44 PDT, Nicholas Nethercote [:njn]
gavin.sharp: review+
n.nethercote: superreview+
Details | Diff | Review
v9, ready to land (102.36 KB, patch)
2011-05-03 17:19 PDT, Nicholas Nethercote [:njn]
n.nethercote: review+
n.nethercote: superreview+
Details | Diff | Review

Description Jeff Muizelaar [:jrmuizel] 2011-02-11 16:11:44 PST
Here's what my about:memory says:

Overview

            Memory mapped:
            1,521,242,112

            Memory in use:
            1,355,010,912

	malloc/allocated1,355,013,712
malloc/mapped1,521,242,112
malloc/zone0/committed1,354,912,224
malloc/zone0/allocated1,511,804,928
js/gc-heap249,561,088
js/string-data27,318,652
js/mjit-code21,088,607
images/chrome/used/raw0
images/chrome/used/uncompressed405,560
images/chrome/unused/raw0
images/chrome/unused/uncompressed0
images/content/used/raw29,099,506
images/content/used/uncompressed11,125,711
images/content/unused/raw13,062
images/content/unused/uncompressed32
storage/sqlite/pagecache119,626,744
storage/sqlite/other1,871,784
layout/all54,610,554
layout/bidi40,358
gfx/surface/image16,120,280
content/canvas/2d_pixel_bytes27,315,380
Comment 1 Nicholas Nethercote [:njn] 2011-02-11 17:29:45 PST
Indeed.  Also, about:memory currently only talks about heap (malloc'd) memory, I want to expand that to all memory, because there's a fair bit that isn't malloc'd (all the JIT code, for example).  (Well, js/mjit-code is actually a mix of heap memory and mmap'd memory, just to confused things.)

http://blog.mozilla.com/nnethercote/2011/02/09/a-vision-for-better-memory-profiling-with-aboutmemory/ describes my plans for about:memory.  I've started working on a list of more detailed counters, it's certainly not complete but I figure I'll post it here for early feedback.  There's more detail for JS counters because I know the JS engine much better than anything else.

In the list below, the text after the '#' on each line represents what the tool-tip would say.  Text in square brackets is just extra explanatory stuff that wouldn't be shown.

Bug 625305 is also relevant here.  I'd like the JS parts of the global breakdown to also be presented on a per-compartment basis.


Current global allocation breakdown
-----------------------------------
[PMDB == "Page mappings done by"]   Bytes mapped by
[MDB == "mallocs done by"]          [BMB]  Bytes malloc'd by
[SUB  == "Space used by"]           [BUB]  Bytes used by
[SUnB  == "Space unused by"]        [BUnB] Bytes unused by

mmap-total              # Bytes mapped by the entire process
- mmap-malloc           # Bytes mapped by malloc [== malloc/mapped]
  - malloc-used         # Bytes malloc'd by the entire process
                        #   [== malloc/allocated]
    - js                # Bytes malloc'd by the JavaScript engine
      - mjit            # Bytes malloc'd by the method JIT;  includes
                        #   JITScripts and IC data
                        #   [could potentially break this up into many
                        #   counts, one for JITScript-proper and one per
                        #   variable-length section]
      - tjit            # MDB the trace JIT
        - nj-alloc      # MDB the Nanojit allocators (dataAlloc, traceAlloc,
                        # tempAlloc) including the reserve space
          - used        # SUB the trace JIT within the Nanojit allocators
          - unused      # SUnB the trace JIT within the Nanojit allocators
                        # [used and unused may be overkill]
      - gc-heap         # ...
      - string-data     # ...
    - storage...        # ...
    - images...         # ...
    - layout...         # ...
    - gfx...            # ...
    - other             # HADB everything else, plus heap waste due to 
                        #   alignment requirements and similar causes
  - malloc-unused       # PMDB malloc, not in use (malloc-mapped - malloc-used)

- mmap-code             # Memory mapped for JIT code
  - mjit                # Memory mapped for method JIT code [via
                        #   ExecutablePool::systemAlloc()]
    - used              # Code generated by the method JIT
      - normal          # All inline code, out-of-line code, and MICs
                        #                  [after bug 631045 is done]
      - PICs            # All PICs [after bug 631045 is done]
    - unused            # Unused method JIT code mappings (mjit - used)
  - tjit                # Memory mapped for trace JIT code [via
                        #   TraceMonitor::codeAlloc, alloc'd in 
                        #   CodeAlloc::allocCodeChunk]
    - used              # Code generated by the trace JIT
      - trace           # Code generated in traces
      - exit            # Code generated in trace exits
    - unused            # Unused trace JIT code mappings (tjit - used)
- mmap-other            # PMDB everything else
Comment 2 Nicholas Nethercote [:njn] 2011-02-11 18:16:07 PST
A couple of things to note about the design above:
- It covers both heap and non-heap.
- The hierachical nature of it is explicit in the tree structure.
- There are various "other" counts, which account for the unknown parts.  It's good to have these to save you from having to mentally do the arithmetic.

W.r.t. that last point, about:memory will never cover all memory -- there are too many small allocations that are too hard and/or too expensive to track.  But my experience with Massif makes me think we can get 80% or 90% of the heap accounted for without too many extra counters.  And Massif will help us reach that goal -- if someone says "hey, 'malloc-used/other' is half of 'malloc-used'" for workload X, I can measure workload X under Massif to see what we're missing.

That will still leave code/data segments and thread stacks, together which account for more than half of all mapped memory on 64-bit Linux.  They are hard to count, though and so will likely end up in the "mmap-other" bucket.
Comment 3 The 8472 2011-03-16 12:48:03 PDT
>That will still leave code/data segments and thread stacks, together which
>account for more than half of all mapped memory on 64-bit Linux.

Shouldn't that amount be mostly constant? As the # of tabs goes up the fraction of that should go down. I'm seeing 50% unaccounted memory even with 2GB footprint, so we're missing other things too with the current about:memory.
Comment 4 Nicholas Nethercote [:njn] 2011-03-16 15:33:23 PDT
(In reply to comment #3)
> 
> Shouldn't that amount be mostly constant?

Hmm, the thread stacks won't be constant but the code size will be constant-ish.

> As the # of tabs goes up the fraction
> of that should go down. I'm seeing 50% unaccounted memory even with 2GB
> footprint, so we're missing other things too with the current about:memory.

Oh, absolutely, I'm sure we're missing plenty of stuff at the moment.  See paragraph 2 of comment 2.  If you can specify a particular website(s) that you think we're missing a lot of, that would be useful.  If you open 1 copy of a website, measure, then open, say, another 9 copies, and re-measure, that might allow a rough estimate of how much is being missed.
Comment 5 Hugh Nougher [:Hughman] 2011-03-18 02:07:07 PDT
An issue concerning about:memory reporting is the following. I think its relavent to this.

The case is a jQuery Flot graph with 15 lines loaded at startup and displayed as they arrive from an ajax call (ie: the graph/canvas is redrawn each time new data arrives). In the few seconds after this, gfx/d2d/surfacevram and content/canvas/2d_pixel_bytes show values new ~200MB each before returning to normal levels. This value is misleading because the actual ram usage is still ~100MB and windows task manager says peak working memory is only about ~130MB for the same process.

I don't actually know where these values are coming from but the 2 possibilities I see are:
- [At least] these two values are reporting the usage in last few seconds (its not the peak and its not the current) which I believe should be said somewhere so its known to people viewing it.
- These values are of image sizes now stored in the GPU memory which would make it completely different memory and should be grouped seperately in the display.
Comment 6 Jeff Muizelaar [:jrmuizel] 2011-03-18 05:17:51 PDT
(In reply to comment #5)
> I don't actually know where these values are coming from but the 2
> possibilities I see are:
> - [At least] these two values are reporting the usage in last few seconds (its
> not the peak and its not the current) which I believe should be said somewhere
> so its known to people viewing it.
> - These values are of image sizes now stored in the GPU memory which would make
> it completely different memory and should be grouped seperately in the display.

gfx/d2d/surfacevram can definitely be in GPU memory. Whether or not it actually is depends on what the Direct3D implementation wants to do with it. However, it probably does make sense to make this distinction clear. This memory is certainly not malloced.
Comment 7 The 8472 2011-03-18 09:17:56 PDT
> This value is misleading because the actual ram usage is still
~100MB and windows task manager says peak working memory is only about ~130MB
for the same process.

I think surface memory falls under virtual memory, not working memory since it's just mapped in from the gfx card. You're basically looking at the wrong figure.
Comment 8 Nicholas Nethercote [:njn] 2011-03-31 22:45:30 PDT
(In reply to comment #6)
> 
> gfx/d2d/surfacevram can definitely be in GPU memory.

Do you mean gfx/surface/d2d?
Comment 9 Nicholas Nethercote [:njn] 2011-03-31 22:53:56 PDT
Created attachment 523524 [details] [diff] [review]
WIP patch (against TM 64136:0477bca4863d)

Here's an in-progress reworking of about:memory.  I'll attach a screenshot
shortly.

Notable changes:
- about:memory looks completely different.  Mapped memory is presented as a
  tree;  total virtual memory use is now shown.  Used heap memory is also
  presented as a tree.  Other measurements (which may overlap the mapped and
  heap-used measurements) are presented as a list.

- Heap-related measurements have different names (eg. no more
  "malloc/mapped", "malloc/allocated").  I think the new names are better.
  Some other measurement names changed slightly.

- Tracking for the JavaScript method JIT and trace JIT is greatly improved.
  Code memory and data memory are counted separately.  Code memory is
  correctly shown in the "mapped" tree.

- Tooltips for each stat are more detailed, and more consistently worded.

Note that I've put little effort into making it pretty, because (a) I'm a
hopeless HTML+CSS programmer, and (b) I want to focus on the function for
now.  Having said that, I do like the strong alignment and indentation given
by the fixed-width font gives (including the ugly underscores), I think it
makes reading the tree easier.  It also cut+pastes quite well into a text
buffer, something else I think is important, eg. for Bugzilla reports.

Things not yet done:
- Nothing related to compartments.  I want to show per-compartment info in
  here eventually, instead of creating a separate about:compartments page.
  For each compartment I figure I'll show the "mapped/js" and "heap-used/js"
  subtrees.  However, the tjit/mjit changes I did in a fashion that I think
  will work nicely with compartments -- ie. to get the newly added stats I
  iterate over per-compartment data structures, rather than just maintaining
  a per-JSRuntime count.

- It only handles a single process.  Fennec's "Content Process" measurements
  will just get shunted down into "Other Measurements".  I have a basic plan
  for this, basically all the sections will be repeated for each extra
  process.  Plus I might sum some of the more important numbers (eg.
  "mapped", "other/resident") to give some measurements for the browser as a
  whole.

- I have no plan for the plugin-container process.  Seems like we could only
  get the most basic stats for it anyway, eg. "mapped", "other/resident".

- "mapped" and "other/resident" measurements are not implemented on Mac, and
  I'm not sure how to do them.  Looks like it might require crawling over
  some ugly Mach data structures, ugh.

- The "heap-used/other" count is often quite high.  We need to add more counts
  to get this down as low as possible.

- Only tested on Linux so far.

- All the gfx/surface/* numbers are still counted under heap-used.
Comment 10 Nicholas Nethercote [:njn] 2011-03-31 22:58:22 PDT
Created attachment 523525 [details]
screenshot for WIP patch
Comment 11 Shawn Wilsher :sdwilsh 2011-03-31 23:17:00 PDT
Note that I probably bit-rotted you a little bit when I landed bug 615978 on mozilla-central today.
Comment 12 Nicholas Nethercote [:njn] 2011-03-31 23:25:21 PDT
(In reply to comment #11)
> Note that I probably bit-rotted you a little bit when I landed bug 615978 on
> mozilla-central today.

Aw, geez:

  *desc = ::strdup("Number of lookaside memory slots currently checked out");

You're reporting numbers that aren't byte measurements?  That's not gonna fly with my redesign...
Comment 13 Mike Shaver (:shaver -- probably not reading bugmail closely) 2011-03-31 23:35:53 PDT
Yeah, that's why I built the interface that I did in the about:compartments bug, tbh.  It also allows a given reporter to generate multiple entries, which I found to be useful for compartments and likely other categories.
Comment 14 Shawn Wilsher :sdwilsh 2011-04-01 10:36:45 PDT
(In reply to comment #12)
> You're reporting numbers that aren't byte measurements?  That's not gonna fly
> with my redesign...
I'd take a patch to fix that.  The default size in SQLite is 100, although that's likely to change in the next version of SQLite.  The size is also configureable by calling sqlite3_config(SQLITE_CONFIG_LOOKASIDE, defaultSizePerSlot, defaultNumberOfSlots) to set it globally, and sqlite3_db_config(db, SQLITE_DBCONFIG_LOOKASIDE, NULL, sizePerSlot, slotCount) to set it per connection.  Size per slot is rounded down to the next multiple of 8 (if it isn't, and this is why the default of 100 is likely to change).

I can even file the detailed bug report for this if you want me to, but I can't make any promises of being able to patch it soon.
Comment 15 Jeff Muizelaar [:jrmuizel] 2011-04-01 11:26:09 PDT
(In reply to comment #8)
> (In reply to comment #6)
> > 
> > gfx/d2d/surfacevram can definitely be in GPU memory.
> 
> Do you mean gfx/surface/d2d?

Yes.
Comment 16 Justin Dolske [:Dolske] 2011-04-01 16:42:11 PDT
(In reply to comment #10)
> Created attachment 523525 [details]
> screenshot for WIP patch

--> uiwanted. :)
Comment 17 Jeff Muizelaar [:jrmuizel] 2011-04-01 19:44:42 PDT
(In reply to comment #10)
> Created attachment 523525 [details]
> screenshot for WIP patch

This looks like a good improvement to me.
Comment 18 Nicholas Nethercote [:njn] 2011-04-04 22:31:38 PDT
Created attachment 524037 [details] [diff] [review]
patch v2 (against m-c 64649:4e4c7457e8f7)

This version of the patch handles multiple processes.  I've tested it in Fennec.

Shawn's LookAside_Used values are currently incorrectly treated as if they are byte values.
Comment 19 Nicholas Nethercote [:njn] 2011-04-04 22:34:41 PDT
Created attachment 524038 [details]
v2 screenshot

Fennec screenshot for v2.  The top half of the picture shows the bottom half of the results for Fennec's main process;  the rest of the picture shows the results for Fennec's content process.  It's still ugly, but you can see it's working.
Comment 20 Nicholas Nethercote [:njn] 2011-04-06 21:33:21 PDT
Created attachment 524345 [details] [diff] [review]
patch v3 (against m-c 64649:4e4c7457e8f7)

More improvements:

- Prettified it a bit:
  - Using some colour to break things up.  Suggestions for better colour
    design are welcome.

  - Using Unicode line drawing chars for the tree structure, which makes it
    much clearer.

  - The entire output is now within a <pre> block, even the headings.  This
    means that cut+paste into a text buffer works perfectly.  (See comment 0
    for an example of why I think this cut+paste requirement is important.)

- Merged with mozilla-central, so sdwilsh's new sqlite reporters are
  present.  I renamed them slightly to better match the existing ones.
  aboutMemory.js can handle having multiple reporters with the same path.

- The trees are now sorted so that sub-trees that account for more memory
  use are shown earlier.

Vlad, are you still doing reviews and feedback requests?
Comment 21 Nicholas Nethercote [:njn] 2011-04-06 21:34:06 PDT
Created attachment 524347 [details]
v3 screenshot
Comment 22 Nicholas Nethercote [:njn] 2011-04-06 23:13:52 PDT
(In reply to comment #14)
> The default size in SQLite is 100, although
> that's likely to change in the next version of SQLite.  The size is also
> configureable by calling sqlite3_config(SQLITE_CONFIG_LOOKASIDE,
> defaultSizePerSlot, defaultNumberOfSlots) to set it globally, and
> sqlite3_db_config(db, SQLITE_DBCONFIG_LOOKASIDE, NULL, sizePerSlot, slotCount)
> to set it per connection.  Size per slot is rounded down to the next multiple
> of 8 (if it isn't, and this is why the default of 100 is likely to change).

To summarize (this is for my own benefit as much as anyone else's):  sqlite has its own custom "lookaside" allocator where it pre-allocates a moderate number (10s? 100s?) of "slots", all of which are the same size, typically 50--200 bytes.  And it satisfies small allocation requests out of these slots instead of using the general-purpose malloc.  When a slot is used it is "checked out";  slots cannot be split.

Now, the lookaside-used reporter (which corresponds to the SQLITE_DBSTATUS_LOOKASIDE_USED parameter to sqlite3_db_status()) reports how many slots are "checked out".  There doesn't appear to be a way to find how many slots have been allocated.

And the stmt-used reporter (which correspond to SQLITE_DBSTATUS_STMT_USED) tracks how many bytes have been allocated for prepared statements from either lookaside slots or the general heap.

Goodness, what a pain.  Because the lookaside slots are smallish and there aren't that many of them, my inclination is to remove the lookaside-used reporter.  Bytes of lookaside slots used by prepared statements will be covered by the stmt-used reporters.  Bytes of lookaside slots that are unused (because (a) the slot is unused or (b) the slot is only partially full) will fall through the cracks... but I don't see a better way to do it.

Nb: I used these pages to find all this out:
http://www.sqlite.org/malloc.html#lookaside
http://www.sqlite.org/c3ref/c_dbstatus_cache_used.html
Comment 23 Ed Morley [:emorley] 2011-04-07 03:07:37 PDT
Shawn has a patch that adds units to about:memory in bug 525288; presuming this bug makes it obsolete?
Comment 24 Nicholas Nethercote [:njn] 2011-04-07 16:15:11 PDT
*** Bug 525288 has been marked as a duplicate of this bug. ***
Comment 25 Rick Johnson 2011-04-07 16:34:57 PDT
Is there any display for the typical user to understand which tab is the hog?
Comment 26 Nicholas Nethercote [:njn] 2011-04-07 16:39:44 PDT
(In reply to comment #25)
> Is there any display for the typical user to understand which tab is the hog?

Not yet.  Bug 625305 will help with that.
Comment 27 Nicholas Nethercote [:njn] 2011-04-07 16:41:42 PDT
I'm going to retask this bug to just be about the about:memory revamp (ie. not 
about adding extra reporting coverage) since that's what it effectively been
about since comment 1.

More specifically, I'm pushing the following off to follow-up bugs:

- Compartments info.  See bug 625305.

- Adding more reporters to cover "other" memory.  This bug has already 
  improved this somewhat for JS.  I created bug 648411 as the tracking bug 
  for further improvements.
  
- Reporting on the plugin-container process.  I created bug 648415 for this.

Things left to do here:

- Filter insignificant entries to reduce the amount of info.

- Add a "verbose" mode that does no filtering and prints values in 
  bytes instead of MB.

- Determine if all the heap-used ones are actually on the heap.

- Implement "mapped" and "other/resident" on Mac.

- Any remaining changes to the formatting.  Specific suggestions are welcome.

- Testing on all platforms.
Comment 28 Nicholas Nethercote [:njn] 2011-04-08 00:15:51 PDT
Created attachment 524592 [details] [diff] [review]
patch v4 (against m-c 64649:4e4c7457e8f7)

New in this version:

- Implemented "mapped" and "other/resident" on Mac.  The "mapped" number is huge because it includes heaps of shared memory, but doing anything else is a huge pain, and this number isn't that useful anyway (Google Chrome parses the output of 'top' to get better info, I kid you not.)

- Aggregates insignificant (< 0.1% of total) sub-trees in the heap-used tree.

- Adds a verbose mode that does no aggregation and shows bytes instead of MB.  You switch between normal and verbose modes by clicking a link at the bottom of the page.

- Tweaks the box drawing chars used for the tree structure so that they cut+paste more reliably (by restricting myself to using the ones most commonly supported in various charsets).

Still to do:

- Determine if all the heap-used ones are actually on the heap.

- Any remaining changes to the formatting.

- More testing, esp. on Windows.  (I don't have easy access to a Windows box, if anyone wants to try the patch that'd be great.)
Comment 29 Nicholas Nethercote [:njn] 2011-04-08 00:19:56 PDT
Created attachment 524593 [details]
v4 screenshot

This screenshot shows how sub-trees are aggregated -- with an eg. "(3 omitted)" entry.
Comment 30 Nicholas Nethercote [:njn] 2011-04-08 00:23:15 PDT
Limi, CC'ing you on dolske's suggestion.  Currently the "uiwanted" keyword is attached to this bug, but that was added for an earlier, uglier version.  The current version (see attached screenshot) isn't exactly the CSS Zen Garden, but the old about:memory wasn't either.  Do you think this needs more UI work, and if so, do you have specific suggestions?  It's not a page the typical user will ever see.

(And please note that the new version has the very important property that you can cut+paste it into a text buffer perfectly, unlike the old version!)
Comment 31 :Ehsan Akhgari (busy, don't ask for review please) 2011-04-08 01:04:21 PDT
(In reply to comment #30)
> Limi, CC'ing you on dolske's suggestion.

Sorry, but I have to ask.  Why do we care about how nice this looks?  I guess making it not want to put a knife in our eyes is good, but I don't know why this needs any input from the UX folks at all.  FWIW, the current screenshot looks very nice to me as far as our obscure about: pages go.  :-)
Comment 32 Rick Johnson 2011-04-08 06:29:48 PDT
(In reply to comment #31)
Agree.  Screenshot is very good presentation for its obscurity.  :)
I am more interested in Tab or Compartment segregated resources.  Bug 625305 has my vote for user relevance.
Comment 33 Justin Dolske [:Dolske] 2011-04-08 12:26:18 PDT
We already talked about this on IRC last night, just wanted to note it here:

Improving the UI/design absolutely shouldn't block progress here, this is super useful data and is already much better than the page we have right now. I just wanted to get the UI folks involved because this is a classic data visualization problem, and UI folks love those kinds of problems. :) And I know how good it feels to implement a nice feature, and then mix in an awesome visual design to know it out of the park.

Developers deserve beautiful tools too. ;-)
Comment 34 Mike Beltzner [:beltzner, not reading bugmail] 2011-04-08 16:42:16 PDT
Indeed, I'd split out the presentation issue as another bug, and get this enhanced diagnostic functionality in nowrightnow. :)
Comment 35 christian 2011-04-08 16:44:08 PDT
Of note this has until Monday to land on mozilla-central or it won't make it in before the aurora merge for Firefox 5
Comment 36 Nicholas Nethercote [:njn] 2011-04-08 16:45:33 PDT
(In reply to comment #35)
> Of note this has until Monday to land on mozilla-central or it won't make it in
> before the aurora merge for Firefox 5

Not gonna happen :(
Comment 37 christian 2011-04-08 17:15:41 PDT
That's ok! We'll have another release in no time ;-)
Comment 38 Nicholas Nethercote [:njn] 2011-04-11 22:19:40 PDT
Created attachment 525304 [details] [diff] [review]
patch v5, minus JS + xpconnect + NJ changes (against m-c 67803:09b605eb3e0d)

It's ready for review.  This is everything except the JS/Nanojit/xpconnect changes.  Reiterating the improvements vs. the old about:memory...

- It distinguishes mapped and heap memory in a clearer fashion.

- Hierarchical (tree) reporting (including percentages) clarifies the
  breakdown of memory usage (esp. all the memory that's lumped in one of the
  "other" buckets).

- More reporters:
  - "mapped", "other/resident"
  - JS method JIT ones have been split and categorized appropriately, and
    JS trace JIT ones have been added (see next patch)

- Filtering of results allows more reporters to be added in the future
  without bloating the output.  Verbose mode allows full details to be seen
  if desired.

- More detailed explanations of each metric in the tool-tips.

Vlad told me he is happy to review this, BTW.  bsmedberg, sdwilsh recommended you for super-review.
Comment 39 Nicholas Nethercote [:njn] 2011-04-11 22:21:08 PDT
Created attachment 525305 [details] [diff] [review]
patch v5, JS + xpconnect parts

JS + xpconnect changes:

- In order to measure how much memory is has allocated, ExecutableAllocator
  now has a hashtable holding a weak reference to every live ExecutablePool.
  This required some rearranging:
  - each pool now has a pointer to the allocator that created it;
  - system{alloc,Release} moved from ExecutablePool to ExecutableAllocator,
    which is arguably a better place for them anyway.

  This change also means that pool leaks (due to erroneous ref-counting) can
  be detected.

- I renamed m_smallAllocationPools as m_smallPools to save typing.

- I fixed a leak-on-OOM in createPool() by calling systemRelease() if the
  allocation of 'pool' fails.

- I fixed a typo ("ProtectionSeting") that's been bugging me for ages.

- Added some code size reporters to the trace JIT.

- The shell built-in mjitcodestats() now reports total mjit memory
  allocated, not memory used.

- In xpconnect, added more reporters.  All the ones that traverse
  compartments are protected by an AutoLockGC, which I believe is necessary.
Comment 40 Nicholas Nethercote [:njn] 2011-04-11 22:22:45 PDT
Created attachment 525306 [details] [diff] [review]
patch v5, nanojit changes

Ed, this patch adds some code size reporters to Allocator.  It's needed for some  memory usage measurements inside Firefox.
Comment 41 Nicholas Nethercote [:njn] 2011-04-11 22:28:20 PDT
Created attachment 525307 [details]
v5 screenshot, verbose mode

In the final version I adjusted the printing of the tree structure so that there's no need to pad the MB (or byte) values with leading zeroes.  You can see particularly how the tree structure lines move to the right as all those sqlite sizes get smaller.  Note also that this screenshot is for verbose mode when we print sizes as bytes instead of MB.
Comment 42 Nicholas Nethercote [:njn] 2011-04-11 22:30:42 PDT
Created attachment 525308 [details] [diff] [review]
patch v5b, minus JS + xpconnect + NJ changes (against m-c 67803:09b605eb3e0d)

Oh, I want to print "MiB" instead of "MB" as the default unit, because it's more correct.  That's the only difference between this patch and the previous version.
Comment 43 Shawn Wilsher :sdwilsh 2011-04-11 22:47:54 PDT
(In reply to comment #42)
> Oh, I want to print "MiB" instead of "MB" as the default unit, because it's
> more correct.  That's the only difference between this patch and the previous
> version.
While it may be "more correct", it's contrary to how we display that same sizing everywhere else in the product.
Comment 44 Nicholas Nethercote [:njn] 2011-04-11 23:08:31 PDT
Oh.  Which meaning of megabyte do we use?  1000 * 1000, 1000 * 1024, or 1024 * 1024?
Comment 45 Ed Morley [:emorley] 2011-04-12 02:57:57 PDT
(In reply to comment #40)
> Created attachment 525306 [details] [diff] [review]
> patch v5, nanojit changes
> 
> Ed, this patch adds some code size reporters to Allocator.  It's needed for
> some  memory usage measurements inside Firefox.

Was this meant to be r?edwsmith ? It's currently r?nobody.
Comment 46 Nicholas Nethercote [:njn] 2011-04-12 03:57:28 PDT
> Was this meant to be r?edwsmith ? It's currently r?nobody.

Yes!  I fixed it.  Thanks.
Comment 47 Shawn Wilsher :sdwilsh 2011-04-12 08:59:00 PDT
(In reply to comment #44)
> Oh.  Which meaning of megabyte do we use?  1000 * 1000, 1000 * 1024, or 1024 *
> 1024?
We use 1024 * 1024, but there's a lot of history here.  For background, see bug 106618.
Comment 48 Ted Mielczarek [:ted.mielczarek] 2011-04-12 09:06:40 PDT
bug 106618 comment 81 says (and I verified), that we use KiB in about:cache, so about:memory seems like it would be fine, since it's not really user-facing.
Comment 49 Shawn Wilsher :sdwilsh 2011-04-12 09:10:27 PDT
(In reply to comment #48)
> bug 106618 comment 81 says (and I verified), that we use KiB in about:cache, so
> about:memory seems like it would be fine, since it's not really user-facing.
I wonder what changed that then.  One of the arguments for using KB/MB/GB in the download manager was that we didn't use KiB anywhere else in the application.

The other dominant argument is that Windows still uses KB/MB/GB, and that's where the vast majority of our users are, so we should stick with what is familiar on the platform.
Comment 50 Nicholas Nethercote [:njn] 2011-04-12 15:28:10 PDT
(In reply to comment #47)
>
> We use 1024 * 1024, but there's a lot of history here.  For background, see bug
> 106618.

Yikes!
Comment 51 Nicholas Nethercote [:njn] 2011-04-12 17:16:04 PDT
Created attachment 525568 [details] [diff] [review]
patch v5c, minus JS + xpconnect + NJ changes (against m-c 67803:09b605eb3e0d)

This version improves the presentation without changing the content:

- I'm now using about.css to make the presentation more consistent with
  (most) other about: pages.

- The entire page is no longer within a single <pre> block either -- only
  the trees and lists are.  This means that the headings and sub-headings
  are now normal <h1> and <h2> elements (and so are not in a monospace
  font).

- I added an <hr> element after each process, which looks nice and is in the
  spirit of about:license.
  
- Cutting+pasting into a text buffer still works nicely thanks to judicious
  use of newlines in the generated HTML.

- I kept the use of "MiB" to match about:cache, since this is a similarly
  obscure page.  (I described "MiB" as "more correct" above but
  "unambiguous" is the property I like most about it.  "MiB" has been around
  for more than 10 years now, so hopefully people are getting used to it.
  But I'll revert to "MB" -- probably with a tool-tip to define it -- if it
  avoids a religious war.)
Comment 52 Nicholas Nethercote [:njn] 2011-04-12 17:19:21 PDT
Created attachment 525570 [details]
v5c screenshot

This screenshot shows the improved presentation.  I shrunk the font size to show more of the output.
Comment 53 Nicholas Nethercote [:njn] 2011-04-12 17:27:13 PDT
With the presentation improvements in v5c, I've taken the liberty of removing the "uiwanted" keyword.  I don't plan to file a follow-up bug to make this prettier as I think it's now good enough, but please feel free to file one (and CC me!) if you disagree.  (Presentation is of course a welcome topic for discussion in reviews of these patches :)
Comment 54 Benjamin Smedberg [:bsmedberg] 2011-04-13 13:37:09 PDT
Comment on attachment 525568 [details] [diff] [review]
patch v5c, minus JS + xpconnect + NJ changes (against m-c 67803:09b605eb3e0d)

I don't understand the hunk in ContentChild.cpp. What is the magic number?
Comment 55 Shawn Wilsher :sdwilsh 2011-04-13 14:50:54 PDT
Comment on attachment 525568 [details] [diff] [review]
patch v5c, minus JS + xpconnect + NJ changes (against m-c 67803:09b605eb3e0d)

>+++ b/gfx/thebes/gfxASurface.cpp
>+    "heap-used/gfx/surface/beos",
I recognize that you just modified what was here, but AFAIK beos is dead in our codebase.  Feel free to nuke this.

>+++ b/storage/src/mozStorageConnection.cpp
>-  reporter =
>-    new StorageMemoryReporter(*this, StorageMemoryReporter::LookAside_Used);
>-  mMemoryReporters.AppendElement(reporter);
>+// Omitting this.  See https://bugzilla.mozilla.org/show_bug.cgi?id=633653#c22 for details.
>+//  reporter = new StorageMemoryReporter(*this, StorageMemoryReporter::LookAside_Used);
>+//  mMemoryReporters.AppendElement(reporter);
I would rather see this code ifdefd out instead.  Also, please file a follow-up bug for fixing this (Toolkit::Storage) and reference it instead.

>+++ b/storage/src/mozStorageService.cpp
> NS_MEMORY_REPORTER_IMPLEMENT(StorageSQLitePageCacheMemoryUsed,
>-                             "storage/sqlite/pagecache",
>-                             "Memory in use by SQLite for the page cache",
>+                             "heap-used/storage/sqlite/pagecache",
>+                             "Memory used by SQLite for the page cache.",
>                              GetStorageSQLitePageCacheMemoryUsed,
>                              nsnull)
> 
> NS_MEMORY_REPORTER_IMPLEMENT(StorageSQLiteOtherMemoryUsed,
>-                             "storage/sqlite/other",
>-                             "Memory in use by SQLite for other various reasons",
>+                             "heap-used/storage/sqlite/other",
>+                             "Memory used by SQLite for other various reasons.",
>                              GetStorageSQLiteOtherMemoryUsed,
>                              nsnull)
Note that both of these are totals, and the others are breakdowns (wanted to make sure that was clear to you since each database connection also has a "cache used" that comes from the page cache).

r=sdwilsh on the storage/ bits

>+++ b/toolkit/components/aboutmemory/content/aboutMemory.js
It would be very helpful if you added javadoc comments to all the methods you are adding here so people who want to hack on this in the future can jump into it a bit easier.  There were a number of methods that I really had to look at carefully to figure out what the arguments were for and what it did.  Please follow the formatting of NetUtil.jsm (http://hg.mozilla.org/mozilla-central/annotate/a6467a88b056/netwerk/base/src/NetUtil.jsm#l63).

>- * the Mozilla Foundation.
>+ *   Mozilla Corporation
Copyright should always be attributed to the foundation, not the corporation (the foundation owns the corporation).

>+"use strict";
cute :)

>+// Must use .href here instead of .search because "about:memory" is a non-standard URL.
>+var gVerbose = (location.href.split(/[\?,]/).indexOf("verbose") !== -1);
I would love to see a mochitest-chrome tests for this (I realize this code had no tests before, but it was also fairly simple before.  I'd hate to see your hard work break on accident moving forward).

>+function update()
>+{
>+    // First, clear the page contents.  Necessary because update() might be
>+    // called more than once due to ChildMemoryListener.
>+    var content = $("content");
>+    while (content.firstChild)
>+        content.removeChild(content.firstChild);
This is going to be a lot faster, especially now that you've added more things:
content.parentNode.replaceChild(content.cloneNode(false), content);
content = $("content");

>+    var mgr = Components
>+        .classes["@mozilla.org/memory-reporter-manager;1"]
>+        .getService(Components.interfaces.nsIMemoryReporterManager);
nit: this is a bit of an odd style for toolkit...

>+        if (!tmrTable[process])
>+            tmrTable[process] = {};
global-nit: brace all ifs (and fors and whiles)

>+    // njn: fake up a minimal set of Fennec-style content process reporters.
>+    var MB = 1024 * 1024;
>+    tmrTable["Content"] = {};
>+    tmrTable["Content"]["mapped"]           = { tpath:"mapped",           memoryUsed:100 * MB };
>+    tmrTable["Content"]["mapped/heap/used"] = { tpath:"mapped/heap/used", memoryUsed:  1 * MB };
>+    tmrTable["Content"]["heap-used/foo"]    = { tpath:"heap-used/foo",    memoryUsed:  1 * MB };
>+    tmrTable["Content"]["other/a/b/c"]      = { tpath:"other/a/b/c",      memoryUsed: 99 * MB };
I don't think we want this to actually land with this, right?
Also, please use const for MB.

>+    // Generate verbosity option link at the bottom.
>+    text += gVerbose
>+          ? "<span class='option'><a href='about:memory'>Less verbose</a></span>"
>+          : "<span class='option'><a href='about:memory?verbose'>More verbose</a></span>";
As this page gets more and more useful, we need to start looking at localizing it.  Can you file a follow-up on this please?

>+function formatBytes(bytes)
...
>+    var s;
>+    if (gVerbose) {
>+        s = formatInt(bytes) + " " + unit;
>+    } else {
>+        var mbytes = (bytes / (1024 * 1024)).toFixed(2);
>+        var a = String(mbytes).split(".");
>+        s = formatInt(a[0]) + "." + a[1] + " " + unit;
>+    }
In the non-verbose case, we should just use DownloadUtils.convertByteUnits (http://hg.mozilla.org/mozilla-central/annotate/a6467a88b056/toolkit/mozapps/downloads/DownloadUtils.jsm#l386) for a few reasons:
1) less code duplication
2) it has tests, this doesn't
3) output is localized already (not immediately important, but useful nonetheless)

>+        // - \u2502: "box drawings light horizontal"
>+        // - \u2502: "box drawings light vertical"
>+        // - \u2514: "box drawings light up and right"
>+        // - \u251c: "box drawings light vertical and right"
Turn these into constants please (with descriptive names) instead of having magic strings around.


It would also be good to have a test for the copy and paste bits here so that doesn't regress.  You clearly spent a good amount of time making that work, so I'd hate to see it break too.

r=sdwilsh on the toolkit bits too.

feedback+ on all the rest.  This is awesome work; thanks so much! :D
Comment 56 Nicholas Nethercote [:njn] 2011-04-13 17:25:15 PDT
(In reply to comment #54)
> Comment on attachment 525568 [details] [diff] [review]
> patch v5c, minus JS + xpconnect + NJ changes (against m-c 67803:09b605eb3e0d)
> 
> I don't understand the hunk in ContentChild.cpp. What is the magic number?

The revision number against which the patch applies:

changeset:   67803:09b605eb3e0d
tag:         tip
user:        Jim Mathies <jmathies@mozilla.com>
date:        Sun Apr 10 16:31:26 2011 -0500
summary:     Bug 645288 - Register Firefox as a webm capable player on Windows. r=rstrong
Comment 57 Nicholas Nethercote [:njn] 2011-04-13 18:27:31 PDT
(In reply to comment #55)
> 
> >+++ b/storage/src/mozStorageConnection.cpp
> >-  reporter =
> >-    new StorageMemoryReporter(*this, StorageMemoryReporter::LookAside_Used);
> >-  mMemoryReporters.AppendElement(reporter);
> >+// Omitting this.  See https://bugzilla.mozilla.org/show_bug.cgi?id=633653#c22 for details.
> >+//  reporter = new StorageMemoryReporter(*this, StorageMemoryReporter::LookAside_Used);
> >+//  mMemoryReporters.AppendElement(reporter);
> I would rather see this code ifdefd out instead.  Also, please file a follow-up
> bug for fixing this (Toolkit::Storage) and reference it instead.

Is a "#if 0" ok, or do you want "#ifdef SOME_CONSTANT"?
I filed bug 649867 as the follow-up.


> >+++ b/storage/src/mozStorageService.cpp
> >+                             "heap-used/storage/sqlite/pagecache",
> >+                             "heap-used/storage/sqlite/other",
>
> Note that both of these are totals, and the others are breakdowns (wanted to
> make sure that was clear to you since each database connection also has a
> "cache used" that comes from the page cache).

Oh, so those per-connection ones are a subset of the total ones?  I'll need to restructure the tree.  heap-used/storage/pagecache measures "number of bytes of page cache allocation which could not be satisfied by the SQLITE_CONFIG_PAGECACHE buffer and were forced to overflow to sqlite3_malloc()"
Hmm... are each of heap-used/storage/foo.sqlite/{cache,stmt,schema}-used clear subsets of either storage/pagecache or storage/other?  I hope so, because the tree structure I've used totally doesn't work with overlapping counts.


> >+"use strict";
> cute :)

Not cute, vital!  Seriously, it really helps.  My favourite is that it means you can't accidentally convert a local variable to a global just by forgetting a 'var' on a local variable.  Before I added the "use strict", at one point I had a bug caused by exactly that.


> >+var gVerbose = (location.href.split(/[\?,]/).indexOf("verbose") !== -1);
> I would love to see a mochitest-chrome tests for this (I realize this code had
> no tests before, but it was also fairly simple before.  I'd hate to see your
> hard work break on accident moving forward).

By "this" do you mean just this verbose flag, or for the entire about:memory page?  I assume the latter... it's a good idea though I've never written a Mochitest.  I found https://developer.mozilla.org/en/Mochitest, but that doesn't help me understand what exactly would I be testing for?  (This is my first patch outside the JS engine :)


> >+        if (!tmrTable[process])
> >+            tmrTable[process] = {};
> global-nit: brace all ifs (and fors and whiles)

Really?  I found https://developer.mozilla.org/en/JavaScript_style_guide, which doesn't mention this matter.  But it does point to  http://autonome.wordpress.com/2006/03/24/ and http://neil.rashbrook.org/JS.htm, and the former says "Braces are usually same-line, and only used when surrounding a multiline block", and the latter says either way is fine.


> >+    // njn: fake up a minimal set of Fennec-style content process reporters.
> >+    var MB = 1024 * 1024;
> >+    tmrTable["Content"] = {};
> >+    tmrTable["Content"]["mapped"]           = { tpath:"mapped",           memoryUsed:100 * MB };
> >+    tmrTable["Content"]["mapped/heap/used"] = { tpath:"mapped/heap/used", memoryUsed:  1 * MB };
> >+    tmrTable["Content"]["heap-used/foo"]    = { tpath:"heap-used/foo",    memoryUsed:  1 * MB };
> >+    tmrTable["Content"]["other/a/b/c"]      = { tpath:"other/a/b/c",      memoryUsed: 99 * MB };
> I don't think we want this to actually land with this, right?

Correct, I meant to take that out before putting up the patch :)


> As this page gets more and more useful, we need to start looking at localizing
> it.  Can you file a follow-up on this please?

Bug 649881.  BTW, I filed it under Core/General, like this bug, because I'm not aware of anywhere better to put about:memory bugs.  Maybe we need a new component?


> >+function formatBytes(bytes)
> ...
> >+    var s;
> >+    if (gVerbose) {
> >+        s = formatInt(bytes) + " " + unit;
> >+    } else {
> >+        var mbytes = (bytes / (1024 * 1024)).toFixed(2);
> >+        var a = String(mbytes).split(".");
> >+        s = formatInt(a[0]) + "." + a[1] + " " + unit;
> >+    }
> In the non-verbose case, we should just use DownloadUtils.convertByteUnits
> (http://hg.mozilla.org/mozilla-central/annotate/a6467a88b056/toolkit/mozapps/downloads/DownloadUtils.jsm#l386)
> for a few reasons:
> 1) less code duplication
> 2) it has tests, this doesn't
> 3) output is localized already (not immediately important, but useful
> nonetheless)

I thought about using that code, but didn't for three reasons:

1. I figured not localizing was better -- a page that is unlocalized except for the numbers seemed worse than a page that is entirely unlocalized.  

2. Using formatInt() for both cases seemed better than using it for only one.

3. Most importantly, I don't want to mix units, ie. GiB vs MiB vs KiB, as that makes things much harder to read.  I chose MiB because that's what I personally think in;  GiB are rare enough that showing thousands of MiB is fine, and anything that only registers as KiB is probably going to get filtered out for being below the significance threshold anyway.

It'll need to be changed when this is localized, clearly.  With the above reasons in mind, is this good enough to stay as-is for now?


> It would also be good to have a test for the copy and paste bits here so that
> doesn't regress.  You clearly spent a good amount of time making that work, so
> I'd hate to see it break too.

How would I test that?


All the other changes are easy and I'll do.  Thanks for the detailed review!
Comment 58 Daniel Cater 2011-04-14 17:36:41 PDT
(In reply to comment #56)
> (In reply to comment #54)
> > Comment on attachment 525568 [details] [diff] [review]
> > patch v5c, minus JS + xpconnect + NJ changes (against m-c 67803:09b605eb3e0d)
> > 
> > I don't understand the hunk in ContentChild.cpp. What is the magic number?
> 
> The revision number against which the patch applies

I think Benjamin was referring to the "31" in the ContentChild.cpp hunk.
Comment 59 Nicholas Nethercote [:njn] 2011-04-14 22:18:52 PDT
(In reply to comment #58)
> 
> I think Benjamin was referring to the "31" in the ContentChild.cpp hunk.

Oh, that.  Turns out that nsPrintfCString defaults to a max length of 15 chars, and if you want anything longer than that you have to provide your own length.  (As a result of this, the old string there was getting silently truncated, ho ho ho.)  I'll introduce a named constant.
Comment 60 Axel Hecht [:Pike] 2011-04-15 09:02:33 PDT
Nit, in https://bugzilla.mozilla.org/attachment.cgi?id=525568&action=diff#a/storage/src/mozStorageConnection.cpp_sec1 you end up with '//' in GetPath, AFAICT.

Re localizing the number formats, I'd not do that at this point.

Technical question, how does the new page work if you copy'n'paste it to, say, pastebin or etherpads? about:support has an extra button for that, for example. Not sure if that's good to have here.
Comment 61 Nicholas Nethercote [:njn] 2011-04-15 14:35:07 PDT
(In reply to comment #60)
> Nit, in
> https://bugzilla.mozilla.org/attachment.cgi?id=525568&action=diff#a/storage/src/mozStorageConnection.cpp_sec1
> you end up with '//' in GetPath, AFAICT.

True.  Hmm, how is that working?  I'll investigate, thanks.

> Technical question, how does the new page work if you copy'n'paste it to, say,
> pastebin or etherpads?

As mentioned above (eg. comment 51), cut+paste works perfectly :)
Comment 62 David Anderson [:dvander] 2011-04-15 18:47:17 PDT
Comment on attachment 525305 [details] [diff] [review]
patch v5, JS + xpconnect parts

I'm not an XPC peer but it looks okay.
Comment 63 Shawn Wilsher :sdwilsh 2011-04-18 09:45:55 PDT
(In reply to comment #57)
> Is a "#if 0" ok, or do you want "#ifdef SOME_CONSTANT"?
> I filed bug 649867 as the follow-up.
#if 0 is perfectly fine

> Hmm... are each of heap-used/storage/foo.sqlite/{cache,stmt,schema}-used clear
> subsets of either storage/pagecache or storage/other?  I hope so, because the
> tree structure I've used totally doesn't work with overlapping counts.
They should be subsets, yes (although things get a bit strange when you have more than one connection and use the shared cache, but we don't do that internally AFAIK).

> By "this" do you mean just this verbose flag, or for the entire about:memory
> page?  I assume the latter... it's a good idea though I've never written a
> Mochitest.  I found https://developer.mozilla.org/en/Mochitest, but that
> doesn't help me understand what exactly would I be testing for?  (This is my
> first patch outside the JS engine :)
I really want a test for the verbose stuff and making sure it matches the output you expect.  More tests are always nice though!

You can find me on irc or ask anyone in #developers and you should get help on how to do a test here.  What you want is a chrome mochitest, and you'll likely want to register some of your memory reporters so you can control the output in the test.  I'll be happy to help you figure this out.

> Really?  I found https://developer.mozilla.org/en/JavaScript_style_guide, which
> doesn't mention this matter.  But it does point to 
> http://autonome.wordpress.com/2006/03/24/ and http://neil.rashbrook.org/JS.htm,
> and the former says "Braces are usually same-line, and only used when
> surrounding a multiline block", and the latter says either way is fine.
Egads, I thought I killed that document.  https://developer.mozilla.org/En/Developer_Guide/Coding_Style is meant to cover C++ and JavaScript now.

> Bug 649881.  BTW, I filed it under Core/General, like this bug, because I'm not
> aware of anywhere better to put about:memory bugs.  Maybe we need a new
> component?
It's in Toolkit, so I suspect Toolkit::General is a better place for it.

> It'll need to be changed when this is localized, clearly.  With the above
> reasons in mind, is this good enough to stay as-is for now?
If you keep this code, you'll need to write extensive tests to make sure it does what it says it does (that is a benefit of using the other code).  I'm going to defer to the module owner (Mossop or vlad for this specific page) here though as to which you should use.

> > It would also be good to have a test for the copy and paste bits here so that
> > doesn't regress.  You clearly spent a good amount of time making that work, so
> > I'd hate to see it break too.
> 
> How would I test that?
My best guess here would be to select certain text on the page, dispatch the copy keyboard shortcut (or run its command), and then compare what is on the clipboard to what you expect.  I would suggest using some test-only memory reporters for this so you can control the output so it is deterministic.
Comment 64 Vladimir Vukicevic [:vlad] [:vladv] 2011-04-21 17:36:01 PDT
Comment on attachment 525568 [details] [diff] [review]
patch v5c, minus JS + xpconnect + NJ changes (against m-c 67803:09b605eb3e0d)

Looks great to me.  Some feedback/thoughts, none of which should block this landing or even necessarily needs to be fixed:

- It would be nice to keep some kind of summary at the top in a user-understandable way.  That was the goal of the Overview box, a quick way to see how much system memory firefox is using, according to our own data (and perhaps some from the system -- a large discrepancy indicating a potential problem).

- In the output, I'm not a fan of "MiB" instead of "MB"... I understand that that's the precise/correct unit, but people are accustomed to seeing MB (e.g. in just about every file size dialog on Windows and OSX) so it ends up looking like a typo.

- I don't really like the heap-used vs. other explicit root distinction.  I think it would be better if something like "heap/" was the magic heap root, and everything not in the "heap" or "mapped" roots is implicitly "other".  (Either way, "heap" I think is better than "heap-used", but that's a bit of an uber-nitpick/bikeshed :-).  If you do want to keep an explicit mapped/heap/other, then IMO NS_RegisterMemoryReporter should enforce that all paths are in one of those roots, at the very least with an assertion print in debug builds.
Comment 65 Vladimir Vukicevic [:vlad] [:vladv] 2011-04-21 17:38:35 PDT
(oh hey, sdwilsh already covered MiB vs. MB, missed that in the middle of the comments!)
Comment 66 Nicholas Nethercote [:njn] 2011-04-23 00:46:30 PDT
(In reply to comment #64)
> 
> - It would be nice to keep some kind of summary at the top in a
> user-understandable way.
> 
> - I don't really like the heap-used vs. other explicit root distinction.  I
> think it would be better if something like "heap/" was the magic heap root, and
> everything not in the "heap" or "mapped" roots is implicitly "other".  (Either
> way, "heap" I think is better than "heap-used", but that's a bit of an
> uber-nitpick/bikeshed :-).  If you do want to keep an explicit
> mapped/heap/other, then IMO NS_RegisterMemoryReporter should enforce that all
> paths are in one of those roots, at the very least with an assertion print in
> debug builds.

Any path not beginning with "mapped" or "heap-used" gets put into the list of "Other Measurements".  The use of the "other" prefix is just a convention, albeit one I wanted everyone to follow;  I'll think about removing it. 

As for the summary and mapped vs. heap-used distinction... here's my design rationale:

- I want about:memory to the be one place people need to look, ie. it subsumes 'top' and 'ps' and the Window task manager.

- I want it to cover all memory, so it has to start with the total "mapped".  But lots of the mapped memory is not that interesting -- code and data segments and thread stacks account for > 50% of mapped memory, on Linux64 at least.  Really, the most interesting memory is that dynamically allocated.  And that correlates closely with the used heap memory... there's some other stuff, like JIT code space, but the heap is most of it, and that's what is usually the best thing to focus on when trying to identify leaks and ways to reduce memory usage, so that's why I show it separately.  (Separating the heap means that the percentages shown for the heap aren't diluted by all the uninteresting mapped memory.)

- You ask for a summary at the top, but what numbers go in that summary?  "mapped" is one possibility, but it's not that interesting, as I said.  "heap-used" is a possibility, but I already highlight that in its own tree.

I see that what I currently have isn't perfect, but in lieu of specific suggestions for improvement, I'll give it some more thought but it'll probably not change much.
Comment 67 Nicholas Nethercote [:njn] 2011-04-27 22:20:11 PDT
(In reply to comment #55)
> 
> >+++ b/gfx/thebes/gfxASurface.cpp
> >+    "heap-used/gfx/surface/beos",
> I recognize that you just modified what was here, but AFAIK beos is dead in our
> codebase.  Feel free to nuke this.

There's heaps of beos surface stuff in cairo, eg. gfx/cairo/cairo/src/cairo-beos-surface.cpp.  Removing that is way outside the scope of this bug, but please feel free to file a follow-up.
Comment 68 Nicholas Nethercote [:njn] 2011-04-29 02:07:25 PDT
Created attachment 529058 [details] [diff] [review]
patch v6 (against m-c 68692:07de8acf5d7a)

New all-in-one patch.  Main new feature is a chrome test which disables the normal reporters, adds some fake deterministic ones, then copies the generated page and checks the copy against an expected result.  Ehsan, bz, I'm asking just for review/feedback on the new test and the added Makefile.in files.

bsmedberg, the patch still needs a super-review.  Thanks!

Getting close now :)
Comment 69 Benjamin Smedberg [:bsmedberg] 2011-04-29 12:14:01 PDT
Comment on attachment 529058 [details] [diff] [review]
patch v6 (against m-c 68692:07de8acf5d7a)

What part of these needs sr? I'm not qualified to review most of the JS changes here, so I'd like at least an guide to what parts of this are API changes that require my thought. Or just break up the patch a bit more.
Comment 70 :Ehsan Akhgari (busy, don't ask for review please) 2011-04-29 12:15:08 PDT
Comment on attachment 529058 [details] [diff] [review]
patch v6 (against m-c 68692:07de8acf5d7a)

I think you should reset the real memory reporters to make sure that this test doesn't affect other tests in the suite in a strange way.  It looks fine otherwise, though.
Comment 71 Nicholas Nethercote [:njn] 2011-05-01 19:48:30 PDT
Created attachment 529406 [details] [diff] [review]
patch, v7, just the chrome test (against m-c 68827:af860093a19a)

Updated test;  restores the real memory reporters at the end.
Comment 72 Nicholas Nethercote [:njn] 2011-05-01 19:52:02 PDT
Created attachment 529407 [details] [diff] [review]
patch v7, minus JS changes

JS and nanojit code doesn't need super-review, so don't worry about them.

As for the rest, the only API change is that the memory reporters have been
completely overhauled:  some were removed (ones that were incorrect or
inappropriate in some fashion), more were added (notably "mapped",
"resident" and several JS ones), and all the rest had their path (eg.
"malloc/allocated") changed to (a) facilitate the new tree-based view, and
(b) so they are named more consistently.  Also, reporters that aren't from
the main process now have their process name at the start of the path,
separated from the rest of the path with a ':' char.

The only consequences I know of these path changes is that it will break
some add-ons.  The ones I know about are MozMill (which used "malloc/mapped"
and "malloc/allocated") and Memory Meter (which used "malloc/allocated").
And Firebug may have added some memory reporting recently, I'm not sure.
The fixes for these add-ons will be to change to the new reporter paths.
"mapped/heap/used" is the new name for "malloc/allocated", and
"malloc/allocated" can be found by summing the new reporters
"mapped/heap/used" and "mapped/heap/unused", though the new reporters
perhaps should be used instead.  It shouldn't be hard for the add-on authors
to look for both the old and the new reporters and use whichever is present,
if that is necessary.  (I asked on dev-platform a while ago whether this
breakage would be a problem.  I got a couple of responses from people saying
"I wouldn't worry about it".)
Comment 73 Nicholas Nethercote [:njn] 2011-05-01 21:58:54 PDT
I just got a perfectly green run on the try server (ignoring for the perenially busted Windows debug Jetpack builds).
Comment 74 Benjamin Smedberg [:bsmedberg] 2011-05-02 08:55:39 PDT
Comment on attachment 529407 [details] [diff] [review]
patch v7, minus JS changes

Make sure you get dev docs on this when it lands, set tracking-firefoxWhatever? and inform the addons guys so they can blog about the change if necessary.
Comment 75 Nicholas Nethercote [:njn] 2011-05-02 20:44:47 PDT
Created attachment 529630 [details] [diff] [review]
patch v8, ready to land (against m-c 68895:86248f7209b7)

On Callek's suggestion, putting the final patch up for re-review.  (Esp. since I don't have m-c commit access, sigh.)  sdwilsh, only very minor changes have occurred since all the prior reviews happened.  sr=bsmedberg was already granted.
Comment 76 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-03 11:57:16 PDT
Comment on attachment 529630 [details] [diff] [review]
patch v8, ready to land (against m-c 68895:86248f7209b7)

I only looked at the toolkit/ portions of this.

>diff --git a/toolkit/components/aboutmemory/content/aboutMemory.css b/toolkit/components/aboutmemory/content/aboutMemory.css

>+.mrPerc {
> }

This should be removed.

>+.option {

>+  -moz-user-select: -moz-none;  /* no need to include this when cutting+pasting */

nit: use "none", not "-moz-none"

>diff --git a/toolkit/components/aboutmemory/content/aboutMemory.js b/toolkit/components/aboutmemory/content/aboutMemory.js

>+function onLoad()
>+{
>+  var os = Cc["@mozilla.org/observer-service;1"].
>+      getService(Ci.nsIObserverService);

Would be nice to use Services.jsm here (just Components.utils.import("resource://gre/modules/Services.jsm") and then you can use Services.obs).

>+function ChildMemoryListener(aSubject, aTopic, aData)
>+{
>+  update();
> }

Why not just add update() as the listener directly?

I did not review the rest of this code closely, but that doesn't seem necessary.

>diff --git a/toolkit/components/aboutmemory/tests/chrome/test_aboutmemory.xul b/toolkit/components/aboutmemory/tests/chrome/test_aboutmemory.xul

>+  addLoadEvent(function() {
>+    test("amFrame",  amExpectedText,  function() {
>+    test("amvFrame", amvExpectedText, function() {
>+    finish() })});
>+  });

nit: This indentation style is fairly confusing, I'd reformat it
Comment 77 Nicholas Nethercote [:njn] 2011-05-03 17:19:49 PDT
Created attachment 529901 [details] [diff] [review]
v9, ready to land

This version ready to land.  Has r+ from six people and sr+ from bsmedberg.



(In reply to comment #76)
> 
> >+.mrPerc {
> > }
> 
> This should be removed.

I kept it because I generate <span class='mrPerc'> tags, which are similar to other tags 'mrName' and 'mrValue'.  It's just the 'mrPerc' doesn't have any formatting applied.


> >+function onLoad()
> >+{
> >+  var os = Cc["@mozilla.org/observer-service;1"].
> >+      getService(Ci.nsIObserverService);
> 
> Would be nice to use Services.jsm here (just
> Components.utils.import("resource://gre/modules/Services.jsm") and then you can
> use Services.obs).

This code pre-dates my changes, and there's similar code for "@mozilla.org/memory-reporter-manager;1" so I kept it unchanged for consistency.

> >+function ChildMemoryListener(aSubject, aTopic, aData)
> >+{
> >+  update();
> > }
> 
> Why not just add update() as the listener directly?

Because the arguments don't match;  it would work in JavaScript, but I like having ChildMemoryListener's arguments explicit.  Also, this code again pre-dates my changes.
Comment 78 Justin Wood (:Callek) 2011-05-03 17:35:27 PDT
(In reply to comment #77)
> Created attachment 529901 [details] [diff] [review] [review]
> v9, ready to land

http://hg.mozilla.org/mozilla-central/rev/1f0635e935d9
Comment 79 Daniel Cater 2011-05-04 10:39:10 PDT
Mozilla/5.0 (X11; Linux i686; rv:6.0a1) Gecko/20110504 Firefox/6.0a1

I can't reproduce it at the moment, but the first time I loaded about:memory in today's build there was an entry at the bottom of the heap table that had a negative size and negative percentage. I clicked on "More verbose" but the values changed, and when I went back to "Less verbose" the value was positive so I don't have a copy of the table to paste.

I assume this is unintended? If it happens again I'll make sure to copy the table.
Comment 80 Jeff Muizelaar [:jrmuizel] 2011-05-04 11:07:27 PDT
The mac numbers look wrong. On startup I get:

3,079.03 MB (100.0%) -- mapped
├──2,936.53 MB (95.37%) -- other
├────135.56 MB (04.40%) -- heap
│    ├───88.84 MB (02.89%) -- used
│    └───46.72 MB (01.52%) -- unused
└──────6.94 MB (00.23%) -- js
       ├──6.81 MB (00.22%) -- mjit-code
       └──0.13 MB (00.00%) -- (1 omitted)

notice the huge amount mapped.
Comment 81 Nicholas Nethercote [:njn] 2011-05-04 14:07:04 PDT
(In reply to comment #79)
> Mozilla/5.0 (X11; Linux i686; rv:6.0a1) Gecko/20110504 Firefox/6.0a1
> 
> I can't reproduce it at the moment, but the first time I loaded about:memory in
> today's build there was an entry at the bottom of the heap table that had a
> negative size and negative percentage. I clicked on "More verbose" but the
> values changed, and when I went back to "Less verbose" the value was positive
> so I don't have a copy of the table to paste.
> 
> I assume this is unintended? If it happens again I'll make sure to copy the
> table.

The sqlite reporters double-count some memory, which causes the negative numbers.  See bug 653630.
Comment 82 Nicholas Nethercote [:njn] 2011-05-04 14:07:51 PDT
(In reply to comment #80)
> The mac numbers look wrong. On startup I get:
> 
> 3,079.03 MB (100.0%) -- mapped

Hover your mouse over "mapped", the tool-tip explains why the number is so big.
Comment 83 bomfog 2011-05-04 20:53:25 PDT
(In reply to comment #81)
> (In reply to comment #79)
[...]
> 
> The sqlite reporters double-count some memory, which causes the negative
> numbers.  See bug 653630.

Is this also covered by that?

After one run of the kraken benchmark on a clean profile, I get this:

Used Heap Memory
121,174,804 B (100.0%) -- heap-used
├──4,277,801,142 B (3530.27%) -- js
│  ├──4,234,772,742 B (3494.76%) -- string-data
│  ├─────34,603,008 B (28.56%) -- gc-heap
│  ├──────7,336,784 B (06.05%) -- tjit-data
│  │      └──7,336,784 B (06.05%) -- allocators
│  │         ├──5,550,000 B (04.58%) -- reserve
│  │         └──1,786,784 B (01.47%) -- main
│  └──────1,088,608 B (00.90%) -- mjit-data

with most of the string-data given back below:

├────3,137,357 B (02.59%) -- layout
│    ├──3,137,357 B (02.59%) -- all
│    └──────────0 B (00.00%) -- bidi
└──-4,192,137,179 B (-3459.58%) -- other

This on a win7 notebook w/2gigs of ram, same result (also around 4 GB) on a vista desktop w/4GB ram. Seems to persist through a lot of browsing, but not a restart. Mostly notable for the magnitude, I guess.
Comment 84 Nicholas Nethercote [:njn] 2011-05-04 21:13:59 PDT
(In reply to comment #83)
> 
> Used Heap Memory
> 121,174,804 B (100.0%) -- heap-used
> ├──4,277,801,142 B (3530.27%) -- js
> │  ├──4,234,772,742 B (3494.76%) -- string-data

Bug 648490!  Thanks for the report.
Comment 85 Nicholas Nethercote [:njn] 2011-05-04 21:47:59 PDT
(In reply to comment #74)
> 
> Make sure you get dev docs on this when it lands, set tracking-firefoxWhatever?
> and inform the addons guys so they can blog about the change if necessary.

I've added "tracking-firefox6?".  Who are the addons guys I should inform?
Comment 86 Alfred Kayser 2011-05-06 07:51:32 PDT
Three things I have noticed playing with the new ‘about:memory’ page:
1. The total amount of memory used for a blank started Nightly is toggles between 60 MB or 70 MB. One run it is 60MB, the next it is 70MB. What is causing this?
2. The total amount of memory is growing quickly when reloading the ‘about:memory’ page. It looks like that the ‘about:memory’ page itself is leaking memory?
3. The other category of Mapped Memory is a big chunk, and grows also. What is there?
Comment 87 Nicholas Nethercote [:njn] 2011-05-08 18:17:38 PDT
> 1. The total amount of memory used for a blank started Nightly is toggles
> between 60 MB or 70 MB. One run it is 60MB, the next it is 70MB. What is
> causing this?

If I start-up Firefox a few times in a row, showing only about:memory, I see a difference like this between the first and second time, but subsequent times are the same.

> 2. The total amount of memory is growing quickly when reloading the
> ‘about:memory’ page. It looks like that the ‘about:memory’ page itself is
> leaking memory?

How quickly is the memory usage growing?  Also, which number are you looking at?

It shouldn't be *leaking* memory, but it is *using* memory, because it runs some JavaScript code to generate the page.  I see "heap-used" go up by 1 or 2MB when I reload repeatedly, but it then comes back down when a GC happens.  

> 3. The other category of Mapped Memory is a big chunk, and grows also. What
> is there?

Hover your mouse over "other" and you'll get a tool-tip explaining it.  In short, code and data segments and thread stacks -- all things that Firefox doesn't explicitly ask for, and which aren't worth worrying about.
Comment 88 Nicholas Nethercote [:njn] 2011-05-17 23:38:16 PDT
Re-landed the nanojit-specific parts.  See the log message for why this is necessary.

http://hg.mozilla.org/projects/nanojit-central/rev/79ed08770916
http://hg.mozilla.org/tracemonkey/rev/c74d3b574b6a
Comment 89 Bug Filler 2011-06-11 22:29:37 PDT
Is it possible to list page-wise/tab-wise memory usage? Like easy page creates its own arena or whatever it's call. Then the memory usage can be retrieved and shown on the page.
Comment 90 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-11 22:41:21 PDT
(In reply to comment #89)
> Is it possible to list page-wise/tab-wise memory usage? Like easy page
> creates its own arena or whatever it's call. Then the memory usage can be
> retrieved and shown on the page.

This is what Bug 661474 seeks to do (to a first approximation).
Comment 91 Nicholas Nethercote [:njn] 2011-06-12 15:37:03 PDT
(In reply to comment #90)
> (In reply to comment #89)
> > Is it possible to list page-wise/tab-wise memory usage? Like easy page
> > creates its own arena or whatever it's call. Then the memory usage can be
> > retrieved and shown on the page.
> 
> This is what Bug 661474 seeks to do (to a first approximation).

Bug 400120 is also open for the broader idea, which is harder but would be more useful.  I'm giving it some thought.
Comment 93 Nicholas Nethercote [:njn] 2011-06-13 16:41:00 PDT
(In reply to comment #92)
> 
> https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIMemoryReporter
> https://developer.mozilla.org/en/XPCOM_Interface_Reference/
> nsIMemoryReporterManager

Looks good, thanks!
Comment 94 Vlad [QA] 2011-06-28 02:18:49 PDT
Setting resolution to Verified Fixed on Mozilla/5.0 (Windows NT 6.1; rv:7.0a1) Gecko/20110627 Firefox/7.0a1
Comment 95 Matt Evans [:mevans] 2011-07-12 05:30:28 PDT
Vlad please verify on all platforms using 6.0b1
Thanks,
Matt
Comment 96 Vlad [QA] 2011-07-12 07:46:22 PDT
Verified Fixed on 
Mozilla/5.0 (Windows NT 6.1; rv:6.0) Gecko/20100101 Firefox/6.0,
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0) Gecko/20100101 Firefox/6.0,
Mozilla/5.0 (X11; Linux i686; rv:6.0) Gecko/20100101 Firefox/6.0 and
Mozilla/5.0 (Windows NT 5.1; rv:6.0) Gecko/20100101 Firefox/6.0

Note You need to log in before you can comment on or make changes to this bug.