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

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

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.

>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.

(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.

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.

(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.

> 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.

(In reply to comment #6) > > gfx/d2d/surfacevram can definitely be in GPU memory. Do you mean gfx/surface/d2d?

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.

Note that I probably bit-rotted you a little bit when I landed bug 615978 on mozilla-central today.

(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...

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.

(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.

(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.

(In reply to comment #10) > Created attachment 523525 [details] > screenshot for WIP patch --> uiwanted. :)

(In reply to comment #10) > Created attachment 523525 [details] > screenshot for WIP patch This looks like a good improvement to me.

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.

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.

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?

(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

Shawn has a patch that adds units to about:memory in bug 525288; presuming this bug makes it obsolete?

Is there any display for the typical user to understand which tab is the hog?

(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.

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.

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.)

This screenshot shows how sub-trees are aggregated -- with an eg. "(3 omitted)" entry.

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!)

(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. :-)

(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.

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. ;-)

Indeed, I'd split out the presentation issue as another bug, and get this enhanced diagnostic functionality in nowrightnow. :)

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

(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 :(

That's ok! We'll have another release in no time ;-)

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.

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.

Ed, this patch adds some code size reporters to Allocator. It's needed for some memory usage measurements inside Firefox.

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.

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.

(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.

Oh. Which meaning of megabyte do we use? 1000 * 1000, 1000 * 1024, or 1024 * 1024?

(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.

> Was this meant to be r?edwsmith ? It's currently r?nobody. Yes! I fixed it. Thanks.

(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.

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.

(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.

(In reply to comment #47) > > We use 1024 * 1024, but there's a lot of history here. For background, see bug > 106618. Yikes!

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.)

This screenshot shows the improved presentation. I shrunk the font size to show more of the output.

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 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 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

(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

(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!

(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.

(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.

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.

(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 on attachment 525305 [details] [diff] [review] patch v5, JS + xpconnect parts I'm not an XPC peer but it looks okay.

(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 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.

(oh hey, sdwilsh already covered MiB vs. MB, missed that in the middle of the comments!)

(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.

(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.

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 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 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.

Updated test; restores the real memory reporters at the end.

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".)

I just got a perfectly green run on the try server (ignoring for the perenially busted Windows debug Jetpack builds).

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.

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 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

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.

(In reply to comment #77) > Created attachment 529901 [details] [diff] [review] [review] > v9, ready to land http://hg.mozilla.org/mozilla-central/rev/1f0635e935d9

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 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.

(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.

(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.

(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.

(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.

(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?

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?

> 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.

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

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.

(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).

(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.

Documentation: https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIMemoryReporter https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIMemoryReporterManager Also mentioned on Firefox 6 for developers.

(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!

Setting resolution to Verified Fixed on Mozilla/5.0 (Windows NT 6.1; rv:7.0a1) Gecko/20110627 Firefox/7.0a1

Vlad please verify on all platforms using 6.0b1 Thanks, Matt

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