Last Comment Bug 730181 - Merge the "dom+style" and "layout" sub-trees in about:memory
: Merge the "dom+style" and "layout" sub-trees in about:memory
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Nicholas Nethercote [:njn]
:
Mentors:
Depends on:
Blocks: 713799
  Show dependency treegraph
 
Reported: 2012-02-23 18:59 PST by Nicholas Nethercote [:njn]
Modified: 2012-03-04 05:08 PST (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 1: merge the sub-trees (23.03 KB, patch)
2012-02-23 18:59 PST, Nicholas Nethercote [:njn]
n.nethercote: review+
Details | Diff | Splinter Review
patch 2: rename things (18.23 KB, patch)
2012-02-23 19:02 PST, Nicholas Nethercote [:njn]
khuey: review+
Details | Diff | Splinter Review

Description Nicholas Nethercote [:njn] 2012-02-23 18:59:53 PST
Created attachment 600276 [details] [diff] [review]
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 [:njn] 2012-02-23 19:02:11 PST
Created attachment 600277 [details] [diff] [review]
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-02-26 19:20:29 PST
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-02-26 19:24:39 PST
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 [:njn] 2012-02-26 19:47:45 PST
> 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) 2012-02-27 05:33:47 PST
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 [:njn] 2012-02-27 20:08:45 PST
> ::: 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 [:njn] 2012-03-01 20:56:28 PST
Comment on attachment 600276 [details] [diff] [review]
patch 1: merge the sub-trees

khuey gave r+ via IRC.

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