Closed
Bug 730181
Opened 12 years ago
Closed 12 years ago
Merge the "dom+style" and "layout" sub-trees in about:memory
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
(Whiteboard: [MemShrink:P2])
Attachments
(2 files)
23.03 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
18.23 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
This patch: - Renames nsDOMMemoryMultiReporter as nsWindowsMemoryReporter. - Renames nsDomMemoryReporter.h as nsWindowsMemoryReporter.h. - Removes some unnecessary #includes of that header.
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•12 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•12 years ago
|
Summary: Merge the "dom+style" and "layout" in about:memory → Merge the "dom+style" and "layout" sub-trees in about:memory
Comment 5•12 years ago
|
||
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•12 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•12 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P2]
Assignee | ||
Comment 7•12 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•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c12cfaf9cd4e https://hg.mozilla.org/integration/mozilla-inbound/rev/4069a04e8e81
Comment 9•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c12cfaf9cd4e https://hg.mozilla.org/mozilla-central/rev/4069a04e8e81
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•