713799 - Rearrange the "window-objects" sub-tree in about:memory to make per-tab memory usage more obvious

Status: RESOLVED FIXED

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

Description

[Nicholas Nethercote [inactive]]

The "dom" sub-tree in about:memory will soon become the "dom+layout" tree (bug 671299), and each inner window entry will have "dom" and "layout" children.  This is a step towards per-tab memory reporting.

The next step is to rearrange the "dom+layout" tree to make its structure correspond more obviously to the user's tabs.  My current ideas:

- Merge the current "layout" tree (with per-shell reporting) into the "dom+layout" tree.

- Collapse the "dom+layout" and "window-objects" levels into a single level called "windows".

- Don't distinguish between inner windows and outer windows, because it's not meaningful to those who don't know the difference, and not much help to those who do.  Each inner/outer pair can just be combined, since the outer window's size in memory is always small.

- Distinguish user windows (i.e. tabs) from system tabs (e.g. chrome, etc).  Also put non-window things (like SQLite) under "system".

- Rename the user "top" entries to "tab" or something similar that makes it clear that each one represents a tab, and put the URL from the address bar in that entry.

- Interleave "active" and "cached" entries within each user "tab" window, to make it clear which "tab" a bfcache'd window belongs to.

- Maybe get rid of all the window ids.  They're very confusing to non-experts.  (This'll depend on how useful they are to experts.)

With all these changes (except the id removal), about:memory will look roughly like this:

-- js
   -- ...
-- user-windows
   -- active+cached
      -- tab=7 (inner=21, uri=www.bar.com)
         -- active(id=29, uri=www.foo.com)
            -- layout
            -- dom
         -- active(id=21, uri=www.bar.com)
            -- layout
            -- dom
         -- cached(id=38, uri=www.old.com)
            -- layout
            -- dom
   -- other
      -- tab=none
         -- other(id=45, uri=www.who.com)
            -- layout
            -- dom
-- system-windows
   -- top=7 (inner=21)
      -- active(id=29, uri=chrome://browser/content/browser.xul)
         -- layout
         -- dom
-- system-other
   -- storage
      -- ...
   -- startup-cache
      -- ...
   -- xpti-working-set
   -- cycle-collector
   -- network-memory-cache
-- images
   -- ...
-- heap-unclassified


These changes will get us quite close to letting users know which tab(s) are causing high memory usage.  (It doesn't cover JS, compartment-per-global is necessary for that.  Nor does it cover images, I'm not sure what's required for that.)

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

To me, it seems worth distinguishing between dom, style sheets, and layout, rather than having style sheets together with layout.  (Not sure whether that comment makes more sense here or in bug 671299.)

Comment 2

[Nicholas Nethercote [inactive]]

(In reply to David Baron [:dbaron] from comment #1)
> To me, it seems worth distinguishing between dom, style sheets, and layout,
> rather than having style sheets together with layout.

By "style sheets" you mean the stuff measured in bug 671299, and by "layout" you mean the per-shell stuff ("arenas", "styledata", "textruns") currently measured under the "layout"?  That sounds fine.  We'll probably go finer-grained eventually, like the JS engine, but I want to get this structure in place first and then experiment with how to sensibly display larger amounts of data.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Nicholas Nethercote [:njn] from comment #2)
> By "style sheets" you mean the stuff measured in bug 671299, and by "layout"
> you mean the per-shell stuff ("arenas", "styledata", "textruns") currently
> measured under the "layout"?

Yes

(I'm not sure what style data isn't in the arenas... though it does sound useful to separate if we can do so.)

Comment 5

[Kurt Pruenner]

Is this bug really just 64-bit Linux only?

Comment 6

[Timothy Nikkel (:tnikkel)]

No. The platform field is frequently ignored.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: patch

This patch goes part of the way to the goal suggested in comment 0:

- Top windows have entries of the form "top(<URI>)".  These mostly 
  correspond to tabs.

- The "cached" and "active" path segments are beneath the "top" segment.

- All ids have been removed.  This makes things shorter, because it's common
  to have multiple window objects with the same URI;  I think this is a good
  thing.
  
- Top inner windows are now not inspected at all.

- The reporting of inner and outer windows has been merged.  This just 
  results in the "dom" child having more than one report, which is fine.
  
Things I haven't done:

- The chrome://browser/content/browser.xul and
  resource://gre-resources/hiddenWindow.html top windows are treated no 
  differently to windows for user tabs.  I'm not sure if there is an easy
  way to distinguish them, and maybe they shouldn't be distinguished anyway.  

- As a result, I haven't renamed the "top" entries as "tab" entries, because
  that wouldn't be accurate.
  
I'll attach some example output shortly.

I'm asking for lots of feedback from people who might be interested, because this structure is likely to stay more or less in place for bug 687724.  Feel free to ignore the feedback request (except for jst :)

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: sample output

Sample output from the patch.

Comment 9

[Nicholas Nethercote [inactive]]

Oh, another detail of the patch:  AFAICT, "other" windows never have a top window, i.e. they are always within the "top(none)" sub-tree.  So I don't bother using a "other/" path segment for them, because it would always be the only child of "top(none)".  Some assertions check that this is true.

Comment 10

[Justin Lebar (not reading bugmail)]

> 6,290,328 B (05.11%) -- active

Could we call this "active-windows" (and correspondingly "cached-windows")?  I know it's redundant because these are under "window-objects", but it seems clearer to me this way.

> - As a result, I haven't renamed the "top" entries as "tab" entries, because
>  that wouldn't be accurate.

Seems easy enough to special-case "chrome://" and "resource://", and then we could say "tab" (and "tabs" in the level above), which would be nice.

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 602809 [details] [diff] [review]
patch

It would be nice if you could keep the 'id' around for the top node.  I think it would make the most sense to put this after the URL so things are still alphabetically sorted.  The benefits from my perspective/about:nosy's perspective are:

1) Avoids merging of two tabs with the same outer URL.  For about:memory, this keeps you to one-memory-tree-per-tab, which seems nice.  Obviously, one could also simply not consolidate reporter results in the tree and avoid this.

2) Avoids ambiguity when attempting to map the memory usage of a tab to a specific tab in the face of the same outer URL (regardless of consolidation).  nsIDOMWindowUtils surfaces the window ID, which allows this correlation.  Without the id, one would need to try to reverse-engineer the proper mapping.  An example of where this would be useful is an extension that shows memory usage for tabs in the actual UI tab or in the web console.

Another potential win is that if memory reporters adopted an (optional) simple filtering mechanism, the id would be a great thing to key off.  I've been thinking about how to make about:memory be less horrible in terms of resource utilization, and letting it have multi-memory reporters report in smaller bites so it doesn't dominate the event loop seems like one of the simpler ways.  Asking the memory reporter to maintain a complicated state token is way too much, but if we could ask this reporter for "id=N" and then CollectReports could only call CollectWindowReports for that window, that could be a major win.  about:nosy would then update its list of windows with an unconstrained query at a much lower rate.  I realize this particular idea is not super relevant to this bug, so feel free to respond privately or to dev.platform.

Comment 12

[Nicholas Nethercote [inactive]]

> 
> It would be nice if you could keep the 'id' around for the top node.
> 
> 1) Avoids merging of two tabs with the same outer URL.  For about:memory,
> this keeps you to one-memory-tree-per-tab, which seems nice.

Mmm, good suggestion.  I'll do that.

> Obviously, one
> could also simply not consolidate reporter results in the tree and avoid
> this.

Consolidation is quite important to avoid repetition in some case.

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: patch, v2

New patch.  It adds the top-id as Andrew suggested, updates the big comment about paths, and updates a test.


> Could we call this "active-windows" (and correspondingly "cached-windows")? 
> I know it's redundant because these are under "window-objects", but it seems
> clearer to me this way.

How strongly do you feel about it?  I prefer just "active" and "cached" because "window" appears immediately below.


> Seems easy enough to special-case "chrome://" and "resource://", and then we
> could say "tab" (and "tabs" in the level above), which would be nice.

I haven't done anything because I worry that there are other cases as well.  I'd love to hear if we have any documentation (or code) that might tell me more about such cases.

Comment 14

[Justin Lebar (not reading bugmail)]

> How strongly do you feel about it?  I prefer just "active" and "cached" because "window" appears 
> immediately below.

Not strongly.

> I haven't done anything because I worry that there are other cases as well.  I'd love to hear if we > have any documentation (or code) that might tell me more about such cases.

e.g. I think jetpack loads a bunch of things as resource://'s.  jst can probably help you untangle this mess...

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Nicholas Nethercote [:njn] from comment #13)
> > Seems easy enough to special-case "chrome://" and "resource://", and then we
> > could say "tab" (and "tabs" in the level above), which would be nice.
> 
> I haven't done anything because I worry that there are other cases as well. 
> I'd love to hear if we have any documentation (or code) that might tell me
> more about such cases.

Jetpacks and other extensions will definitely define resource URIs to get to their HTML files which can then be displayed in tabs or *panels*.  It's just also very common to try and make things look prettier by using the about protocol to make a prettier alias for that URI path.

Panels are pop-up looking things documented here:
https://addons.mozilla.org/en-US/developers/docs/sdk/latest/packages/addon-kit/docs/panel.html

Although the display of the panels is transient (they go away when they lose focus), they are actually implemented using that "hey, draw the contents of some other window instead of me here" magic.  As such, the panel's contents are persistent and will look like a top-level window.  At least, it did when I just checked right now.

I would vote for just naming things all the same for sorting (and automated processing) reasons.  'window' seems reasonable given that we have a 'window' global inside HTML pages even though everything is really a tab now.

Comment 16

[Nicholas Nethercote [inactive]]

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

Comment 17

[Matt Brubeck (:mbrubeck)]

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