704621 - Add per-window DOM memory reporters

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Johnny Stenback (:jst)]

Right now we lump all of the DOM memory usage into one number in about:memory. If we'd split that up to show per window usage we'd give people an idea of what windows are currently in memory which could help with tracking sites that leak window objects etc. We could also split things up to show how much of the DOM memory goes to active windows (currently loaded in a tab), frozen windows (in bfcache), and ones that are neither, which means they're potentially leaked window objects.

Comment 1

[Nicholas Nethercote [inactive]]

> We could also split things up
> to show how much of the DOM memory goes to active windows (currently loaded
> in a tab), frozen windows (in bfcache), and ones that are neither, which
> means they're potentially leaked window objects.

Bug 687724 comment 2 and subsequent comments discuss this as well... with compartment-per-global it sounds like we'll be able to do an even better job of this active/frozen/other categorization.

Comment 2

[Andrew McCreight [:mccr8]]

What about the DOMs associated with other misc. nsDocuments like chrome://global/content/console.xul, chrome://browser/content/hiddenWindow.xul and chrome://global/content/bindings/autocomplete.xml?  Are they accounted for here?  These include a few hundred DOM nodes as far as I've seen.  I'm also generally curious about what owns them.

Comment 3

[Johnny Stenback (:jst)]

XUL files (.xul) are generally loaded in a window, so they should show up with this patch (which I'll attach in a sec), but XML files (.xml) are typically XBL bindings used by both XUL documents and HTML files (indirectly) don't have their own windows and thus won't show up here. But they will be accounted for in the orphan nodes patch that I have in progress too. Those nodes are not necessarily orphan, but they have no other home yet, so until then they'll be counted as orphan nodes (and they're the majority of orphan nodes, in fact).

Comment 4

[Johnny Stenback (:jst)]

Attachment: Fix.

This splits the "dom" memory reporter up such that it shows memory usage per active, frozen, and "leaked?" windows, dealing with inner/outer etc. The output looks like this:

├───30,086,053 B (04.37%) -- dom
│   ├──23,048,652 B (03.35%) -- window-objects
│   │  ├──18,972,984 B (02.75%) -- content
│   │  │  ├──18,459,336 B (02.68%) -- active
│   │  │  │  ├───4,567,292 B (00.66%) -- top=18 (inner=323)
│   │  │  │  │   ├──2,115,395 B (00.31%) -- inner window(id=323, uri=http://www.google.com/reader/view/)
│   │  │  │  │   ├────696,722 B (00.10%) -- inner window(id=344, uri=https://plus.google.com/_/apps-static/_/js/nw/nw_i/rt=h/ver=XHPepUjD9ME.en./sv=1/am=!ODN3Pl2FBN69zP5btYr5vWgf2Jl6YUS10J7z/d=1/)
│   │  │  │  │   ├─────41,713 B (00.01%) -- inner window(id=333, uri=https://plus.google.com/u/0/_/notifications/frame?hl=en&origin=http%3A%2F%2Fwww.google.com&jsh=r%3Bgc%2F25220049-88ab8d80#pid=32&id=gbsf&parent=http%3A%2F%2Fwww.google.com&rpctoken=591452605&_methods=onError%2ConInfo%2ChideNotificationWidget%2CpostSharedMessage%2CsetNotificationWidgetHeight%2CswitchTo%2CnavigateTo%2CsetNotificationText%2CsetNotificationAnimation%2ChandlePosted%2C_ready%2C_close%2C_open%2C_resizeMe)
│   │  │  │  │   ├─────13,351 B (00.00%) -- inner window(id=1745, uri=https://plusone.google.com/u/0/_/+1/fastbutton?url=http%3A%2F%2Fwww.vasabladet.fi%2FStory%2F%3FlinkID%3D178740&source=google%3Areader&expandTo=right&size=small&count=true&annotation=&hl=en&jsh=r%3Bgc%2F25220049-88ab8d80#id=I344_1322470791229&parent=http%3A%2F%2Fwww.google.com&rpctoken=162315398&_methods=onPlusOne%2C_ready%2C_close%2C_open%2C_resizeMe)
│   │  │  │  │   ├─────13,350 B (00.00%) -- inner window(id=1683, uri=https://plusone.google.com/u/0/_/+1/fastbutton?url=http%3A%2F%2Fwww.vasabladet.fi%2FStory%2F%3FlinkID%3D179115&source=google%3Areader&expandTo=right&size=small&count=true&annotation=&hl=en&jsh=r%3Bgc%2F25220049-88ab8d80#id=I323_1322470654016&parent=http%3A%2F%2Fwww.google.com&rpctoken=663993062&_methods=onPlusOne%2C_ready%2C_close%2C_open%2C_resizeMe)
│   │  │  │  │   ├─────13,349 B (00.00%) -- inner window(id=1484, uri=https://plusone.google.com/u/0/_/+1/fastbutton?url=http%3A%2F%2Fwww.vasabladet.fi%2FStory%2F%3FlinkID%3D179336&source=google%3Areader&expandTo=right&size=small&count=true&annotation=&hl=en&jsh=r%3Bgc%2F25220049-88ab8d80#id=I266_1322470080207&parent=http%3A%2F%2Fwww.google.com&rpctoken=442438422&_methods=onPlusOne%2C_ready%2C_close%2C_open%2C_resizeMe)
...
│   │  │  └─────513,648 B (00.07%) -- leaked?
│   │  │        └──513,648 B (00.07%) -- top=null
│   │  │           ├──────696 B (00.00%) -- inner window(id=843, uri=http://www.google.com/reader/view/)
│   │  │           ├──────696 B (00.00%) -- inner window(id=466, uri=https://plusone.google.com/u/0/_/+1/fastbutton?url=https%3A%2F%2Fgithub.com%2FMythTV%2Fmythtv%2Fcommit%2Fd378d3c7913fe32c009315447cd9f667166a2554&source=google%3Areader&expandTo=right&size=small&count=true&annotation=&hl=en&jsh=r%3Bgc%2F25220049-88ab8d80#id=I13_1322463948220&parent=http%3A%2F%2Fwww.google.com&rpctoken=422711755&_methods=onPlusOne%2C_ready%2C_close%2C_open%2C_resizeMe)
│   │  │           ├──────696 B (00.00%) -- inner window(id=1076, uri=http://www.google.com/reader/view/)
...
│   │  └───4,075,668 B (00.59%) -- system
│   │      ├──4,071,492 B (00.59%) -- active
│   │      │  ├────301,274 B (00.04%) -- top=1 (inner=2)
│   │      │  │    ├──296,198 B (00.04%) -- inner window(id=2, uri=chrome://browser/content/browser.xul)
│   │      │  │    ├────2,538 B (00.00%) -- inner window(id=10, uri=about:blank)
│   │      │  │    └────2,538 B (00.00%) -- inner window(id=9, uri=about:blank)
│   │      │  ├────277,840 B (00.04%) -- top=41 (inner=42)
│   │      │  │    ├──272,460 B (00.04%) -- inner window(id=42, uri=chrome://browser/content/browser.xul)
│   │      │  │    ├────2,690 B (00.00%) -- inner window(id=127, uri=about:blank)
│   │      │  │    └────2,690 B (00.00%) -- inner window(id=126, uri=about:blank)
...

And while that's not necessarily pretty, it shows window id's that can be useful when debugging leaks locally (nsGlobalWindow::GetInnerWindowWithId(id) can be called in gdb to find a window by id). And I'm in no way married to the way this looks or whatever, but it's pretty handy to have.

Comment 5

[Johnny Stenback (:jst)]

Oh, and in about:memory with this patch outer windows are reported as one single unit per section (except in the "leaked?" section where they might be useful to see separated out with their uri shown) as they basically never contain anything interesting (their size tends to be constant).

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 577506 [details] [diff] [review]
Fix.

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

First of all, this is cool!  Thanks for doing it.

The nesting is pretty deep.  Can we remove the "content" vs "system" split?  That doesn't seem to add much, esp. since it looks like you can tell if a window is content or system by looking at the URI?  Maybe you could add some marker to system windows without requiring the extra nesting.  (That's how it works with JS compartments.)

Re the "leaked?" category -- how likely is it that a window is actually leaked if it appears under this label?  If it's less than "highly likely" I'd suggest a less provocative name such as "other", and the description could talk about possible leaks.

Nit: can you change "inner window(...)" to "inner-window(...)"?  We generally avoid spaces in the paths, except within parens.

::: dom/base/nsDOMMemoryReporter.cpp
@@ +49,5 @@
>  void
>  nsDOMMemoryReporter::Init()
>  {
>    // The memory reporter manager is going to own this object.
> +  NS_RegisterMemoryMultiReporter(new nsDOMMemoryReporter());

Nit: call it nsDOMMemoryMultiReporter?

@@ +59,5 @@
> +                         nsIMemoryMultiReporterCallback *aCb,
> +                         nsISupports *aClosure)
> +{
> +  NS_NAMED_LITERAL_CSTRING(kWindowDesc,
> +                           "Memory used by a window and the DOM within it.");

Given how complex the memory reporter paths are now it would be nice if this description explained things a bit more... probably a full legend is too much, but hopefully you can find a good compromise?

@@ +62,5 @@
> +  NS_NAMED_LITERAL_CSTRING(kWindowDesc,
> +                           "Memory used by a window and the DOM within it.");
> +
> +  // Build the string "explicit/dom/window(<uri of the document>)"
> +  nsCAutoString str("explicit/dom/window-objects/");

That comment doesn't match the code.

Given how complex the paths can get, a bigger comment that gives the grammar of possible paths would be *very* helpful here.

@@ +88,5 @@
> +      aCb->Callback(EmptyCString(), str, nsIMemoryReporter::KIND_HEAP,
> +                    nsIMemoryReporter::UNITS_BYTES, windowSize, kWindowDesc,
> +                    aClosure);
> +
> +      return;

This early return complicates the control flow.  Can the code be refactored so it's avoided or put in an easier-to-understand place?

@@ +146,5 @@
> +
> +    // A hack: replace forward slashes with '\\' so they aren't
> +    // treated as path separators.  Users of the reporters
> +    // (such as about:memory) have to undo this change.
> +    spec.ReplaceChar('/', '\\');

Ugh, I really wish we used a different path separator for memory reporters, something that doesn't appear in URIs such as '!'.  Oh well, it's hard to change now.

Comment 7

[Johnny Stenback (:jst)]

Attachment: Updated fix.

Nick, I think this addresses your comments, if not, let me know. And please read the massive comment in the reporter and let me know if that makes sense to you. I'm happy to tweak in any way, but it's not easy to describe the paths in just a few lines, which might mean that this is overly complex still, but it's IMO all useful info to have in about:memory.

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 579174 [details] [diff] [review]
Updated fix.

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

This looks good.  Some nits below.

Someone who knows about the DOM should review the DOM-specific parts.

::: dom/base/nsDOMMemoryReporter.cpp
@@ +105,5 @@
> +  // button in the tab where it used to be displayed. "other" means
> +  // that it's neither "active" or "cached", meaning that it's either
> +  // a window that's closed (or navigated away from w/o being cached)
> +  // yet held alive by either a website or our code. In the latter
> +  // case that may mean it's a memory leak, but not necessarily.

I couldn't resist making this comment shorter :)

DOM window objects fall into one of three categories:
- "active" windows are currently either displayed in an active tab, or a
  child of such a window. 
- "cached" windows are in the fastback cache.
- "other" windows are closed (or navigated away from w/o being cached) yet
  held alive by either a website or our code.  The latter case may
  be a memory leak, but not necessarily.

@@ +122,5 @@
> +  // as follows:
> +  //
> +  //   explicit/dom/window-objects/active/top=<top-outer-id> (inner=<top-inner-id>)/inner-window(id=<id>, uri=<uri>)
> +  //   explicit/dom/window-objects/cached/top=<top-outer-id> (inner=<top-inner-id>)/inner-window(id=<id>, uri=<uri>)
> +  //   explicit/dom/window-objects/other/top=<top-outer-id> (inner=<top-inner-id>)/inner-window(id=<id>, uri=<uri>)

I think this single line is clearer:

// explicit/dom/window-objects/{active,cached,other}/top=<top-outer-id> (inner=<top-inner-id>)/inner-window(id=<id>, uri=<uri>)

Or maybe use "<category>" instead of "{active,cached,other}".

I was going to suggest that you remove the "window-objects" layer since there are no other "dom" sub-trees, but I guess there will be in the future?

@@ +128,5 @@
> +  // Where <top-outer-id> is the window id (nsPIDOMWindow::WindowID())
> +  // of the top outer window (i.e. tab, or top level chrome window),
> +  // <top-inner-id> is the window id of the top window's inner window,
> +  // <id> is the window id of the inner window in question, and <uri>
> +  // is the URI per above description. Exposing the window ids is done

Can you list <top-outer-id>, <top-inner-id>, etc, in a bulleted list?  E.g.:

Where:
- <top-outer-id> is...
- <top-inner-id> is ...
- etc.

@@ +139,5 @@
> +  // And for outer windows we simply use:
> +  // 
> +  //   explicit/dom/window-objects/active/outer-windows
> +  //   explicit/dom/window-objects/cached/outer-windows
> +  //   explicit/dom/window-objects/other/outer-windows

Do this on a single line, like the other case above.

@@ +142,5 @@
> +  //   explicit/dom/window-objects/cached/outer-windows
> +  //   explicit/dom/window-objects/other/outer-windows
> +  //
> +  // Which gives us simple counts of how many outer windows (and their
> +  // combined sizes) per group.

s/group/category ?

@@ +187,5 @@
> +    str += NS_LITERAL_CSTRING(")");
> +  } else {
> +    // Combine all outer windows per section (active/cached/other) as
> +    // they basically never contain anything of interest, and are always
> +    // of pretty much the same size. Return early here as none of the

"are always of pretty much" -> "are always pretty much"

Comment 9

[Johnny Stenback (:jst)]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> I was going to suggest that you remove the "window-objects" layer since
> there are no other "dom" sub-trees, but I guess there will be in the future?

Correct, more coming, specifically the patch in bug 704623. And I think we should also add a reporter for XMLHttpRequest objects (generally small, but could be heavily mis-used), and XBL too, which I suspect is a non-trivial chunk of heap-unclassified (though I don't have any numbers on that yet).

Now we could of course drop a level here if we want, we could get rid of "dom" and use "window-objects" (or just "windows"), "orphan-nodes", "xhr", etc... But it kinda feels natural to me to keep all those under "dom".

Fixed the rest, thanks!

Comment 10

[Johnny Stenback (:jst)]

Attachment: Updated fix.

Mounir, Nick already reviewed the memory reporter parts here, but the code for figuring out whether a window is "active", "cached", or "other", plus the uri extracting code, should get another pair of eyes. Can you have a look? Thanks!

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 579607 [details] [diff] [review]
Updated fix.

Kyle says he can probably look at this tomorrow, moving request.

Comment 12

[Nicholas Nethercote [inactive]]

(In reply to Johnny Stenback (:jst, [e-mail]) from comment #9)
> But it kinda feels natural to me to keep all those under "dom".

Fair enough!


> Mounir, Nick already reviewed the memory reporter parts here, but the code
> for figuring out whether a window is "active", "cached", or "other", plus
> the uri extracting code, should get another pair of eyes. Can you have a
> look? Thanks!

That's the part I'm not qualified to review, though I guess khuey will do it now.

Comment 13

[Mounir Lamouri (:mounir)]

Comment on attachment 579607 [details] [diff] [review]
Updated fix.

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

It doesn't seem to be very useful to change the name of the DOMMemoryReporter class in my opinion. Actually, it might lead the reader to think there is non-multiple DOM Memory Reporter.

(and sorry for the delay)

r=me with that comment addressed

::: dom/base/nsDOMMemoryReporter.cpp
@@ +176,5 @@
> +    str.AppendInt(aWindow->WindowID());
> +    str += NS_LITERAL_CSTRING(", uri=");
> +
> +    if (!AppendWindowURI(aWindow, str)) {
> +      str += NS_LITERAL_CSTRING("[system]");

Maybe it would be worth explaining what that means in the comment below?

Comment 14

[Nicholas Nethercote [inactive]]

> It doesn't seem to be very useful to change the name of the
> DOMMemoryReporter class in my opinion. Actually, it might lead the reader to
> think there is non-multiple DOM Memory Reporter.

StorageSQLiteMultiReporter and XPConnectJSCompartmentsMultiReporter are the names of existing multi-reporters.

Comment 15

[Mounir Lamouri (:mounir)]

(In reply to Nicholas Nethercote [:njn] from comment #14)
> > It doesn't seem to be very useful to change the name of the
> > DOMMemoryReporter class in my opinion. Actually, it might lead the reader to
> > think there is non-multiple DOM Memory Reporter.
> 
> StorageSQLiteMultiReporter and XPConnectJSCompartmentsMultiReporter are the
> names of existing multi-reporters.

We should fix those names too then :)
More seriously, I don't really care about the naming but I think it is wrong. If you want to keep it for consistency, that's fine but I believe the best solution would be to keep the previous name and fix the others if needed.

Comment 16

[Johnny Stenback (:jst)]

Attachment: Fix that landed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf3769560744

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cf3769560744