730181 - Merge the "dom+style" and "layout" sub-trees in about:memory

Status: RESOLVED FIXED

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

Description

[Nicholas Nethercote [inactive]]

Attachment: patch 1: merge the sub-trees

This patch:

- Folds the "layout" sub-tree into the "dom+style" subtree.  This allowed
  all the sLiveShells stuff to be removed.

- Collapses "dom+style/window-objects" as "window-objects", because the
  extra level of indentation wasn't achieving anything.

- Changes the |name| of nsDOMMemoryMultiReporter from "dom+style" to
  "window-objects".

- Adds totals to "Other Measurements" for each of the five per-window
  numbers (dom, layout-arenas, layout-style-sets, layout-text-runs,
  style-sheets).

- Removes |WindowTotals| and replaced its uses with |nsWindowSizes|, which
  is almost the same.

Here's some sample output:

├───9,491,192 B (10.24%) -- window-objects
│   └──9,491,192 B (10.24%) -- active
│      ├──6,367,376 B (06.87%) -- top=14 (inner=17)
│      │  ├──3,616,488 B (03.90%) -- inner-window(id=17, uri=http://techcrunch.com/)
│      │  │  ├──1,330,576 B (01.44%) -- layout
│      │  │  │  ├────962,672 B (01.04%) ── arenas
│      │  │  │  ├────361,056 B (00.39%) ── style-sets
│      │  │  │  └──────6,848 B (00.01%) ── text-runs
│      │  │  ├──1,264,128 B (01.36%) ── style-sheets
│      │  │  └──1,021,784 B (01.10%) ── dom
│      │  ├────621,008 B (00.67%) -- inner-window(id=31, uri=http://www.facebook.com/plugins/like.php
?href=http%3A%2F%2Fwww.facebook.com%2Ftechcrunch&send=false&layout=standard&width=270&show_faces=true
&action=like&colorscheme=light&font&height=80)
│      │  │    ├──289,352 B (00.31%) ── style-sheets
│      │  │    ├──278,272 B (00.30%) -- layout
│      │  │    │  ├──200,336 B (00.22%) ── style-sets
│      │  │    │  └───77,936 B (00.08%) ── arenas
│      │  │    └───53,384 B (00.06%) ── dom

and:

  1,815,544 B ── window-objects-dom
  3,287,248 B ── window-objects-layout-arenas
  1,967,504 B ── window-objects-layout-style-sets
      8,496 B ── window-objects-layout-text-runs
  2,412,400 B ── window-objects-style-sheets

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: patch 2: rename things

This patch:
- Renames nsDOMMemoryMultiReporter as nsWindowsMemoryReporter.
- Renames nsDomMemoryReporter.h as nsWindowsMemoryReporter.h.
- Removes some unnecessary #includes of that header.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 600276 [details] [diff] [review]
patch 1: merge the sub-trees

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

r- for the template array weirdness.  Looks good otherwise.

::: dom/base/nsDOMMemoryReporter.cpp
@@ +93,5 @@
> +template <int M, int N>
> +inline void
> +ReportWindowSize(const nsACString &aPath1, const char (&aPath2)[M],
> +                 size_t aAmount, size_t *aTotalAmount, const char (&aDesc)[N],
> +                 nsIMemoryMultiReporterCallback *aCb, nsISupports *aClosure)

This templatized array length stuff seems silly.  Why don't you just use XPCOM string types here?

::: dom/base/nsDOMMemoryReporter.h
@@ +49,5 @@
>  
>  class nsWindowSizes {
>  public:
> +    nsWindowSizes(nsMallocSizeOfFun aMallocSizeOf) {
> +      memset(this, 0, sizeof(nsWindowSizes));

Just use NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW.

::: layout/base/nsIPresShell.h
@@ +1179,5 @@
> +  virtual void SizeOfIncludingThis(nsMallocSizeOfFun aMallocSizeOf,
> +                                   size_t *aArenasSize,
> +                                   size_t *aStyleSetsSize,
> +                                   size_t *aTextRunsSize) const = 0;
> +

Adding a virtual function requires an IID change.

::: layout/base/nsPresShell.cpp
@@ +8978,1 @@
>    gCaptureTouchList.Init();

Yuck.  This should definitely be heap allocated.  File that bug please?

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 600277 [details] [diff] [review]
patch 2: rename things

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

Can we call this 'nsWindowMemoryReporter' to avoid confusion with the operating system?

Comment 4

[Nicholas Nethercote [inactive]]

> Can we call this 'nsWindowMemoryReporter' to avoid confusion with the
> operating system?

I used "Windows" because it measures all the windows, not just one, but ok.

Comment 5

[Justin Lebar (not reading bugmail)]

FWIW, one might be able to argue in favor of this templatized string code where perf matters.  But this is cold code, and code size is much more important.

Comment 6

[Nicholas Nethercote [inactive]]

> ::: layout/base/nsPresShell.cpp
> @@ +8978,1 @@
> >    gCaptureTouchList.Init();
> 
> Yuck.  This should definitely be heap allocated.  File that bug please?

Bug 731108.

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 600276 [details] [diff] [review]
patch 1: merge the sub-trees

khuey gave r+ via IRC.

Comment 8

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c12cfaf9cd4e
https://hg.mozilla.org/integration/mozilla-inbound/rev/4069a04e8e81

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c12cfaf9cd4e
https://hg.mozilla.org/mozilla-central/rev/4069a04e8e81