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

RESOLVED FIXED in mozilla13

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

Trunk
mozilla13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
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
Attachment #600276 - Flags: review?(khuey)
(Assignee)

Comment 1

5 years ago
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.
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Attachment #600277 - Flags: review?(khuey)
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?
Attachment #600276 - Flags: review?(khuey) → review-
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?
Attachment #600277 - Flags: review?(khuey) → review+
(Assignee)

Comment 4

5 years ago
> 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.
(Assignee)

Updated

5 years ago
Summary: Merge the "dom+style" and "layout" in about:memory → Merge the "dom+style" and "layout" sub-trees in about:memory
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.
(Assignee)

Comment 6

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

Bug 731108.
(Assignee)

Updated

5 years ago
Whiteboard: [MemShrink] → [MemShrink:P2]
(Assignee)

Comment 7

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

khuey gave r+ via IRC.
Attachment #600276 - Flags: review- → review+
(Assignee)

Comment 8

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

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/c12cfaf9cd4e
https://hg.mozilla.org/mozilla-central/rev/4069a04e8e81
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.