Closed
Bug 705987
Opened 13 years ago
Closed 13 years ago
Use mallocSizeOf in the layout memory reporters
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
(Blocks 1 open bug)
Details
(Whiteboard: [MemShrink:P2])
Attachments
(1 file, 2 obsolete files)
33.50 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
I plan to convert the existing style memory reporters to use the new mallocSizeOf style.
Assignee | ||
Updated•13 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P2]
Assignee | ||
Updated•13 years ago
|
Summary: Use mallocSizeOf in the style memory reporters → Use mallocSizeOf in the layout memory reporters
Assignee | ||
Comment 1•13 years ago
|
||
This patch converts the layout/* reporters to use mallocSizeOf. (https://wiki.mozilla.org/Memory_Reporting has details about mallocSizeOf and why it's good.) Things to note: - Slop is now measured. I did some rough measurements on Gmail that indicate that about 1.06x more memory usage under the "layout" reporters is being measured because of this. - I converted everything to use |size_t| throughout for memory sizes. - StackArena::mMarks is now measured. - RuleCascadeData::mRuleHash (the member itself, not what it points to) was measured twice because we didn't have both RuleHash::SizeOf{In,Ex}cludingThis. I added them, and then had to add them to various sub-classes. Also, some of those sub-classes (nsTransitionManager, nsAnimationManager) didn't have their own implementations of SizeOf() and so were improperly being measured with CommonAnimationManger::SizeOf(). - nsTArray::SizeOf() hasn't been converted to use mallocSizeOf yet, that'll be a follow-up bug. (And this bug will make that much easier.) - I used |PL_DHashTableSizeOfExcludingThis| (which I introduced in bug 704723) in various places, which made things nicer. - In |RuleCascadeData|, |mXULTreeRules| was mis-measured before this patch due to a cut and paste error -- the code was measuring its entry storage, but not measuring the things its entries point to, instead measuring (for a second time!) the things pointed to by |mAnonBoxRules|. I fixed this.
Attachment #578963 -
Flags: review?(khuey)
Assignee | ||
Comment 2•13 years ago
|
||
I just found that we need to measure the size of a PLArenaPool in two different places: layout/base/nsPresArena.cp and layout/style/nsCSSRuleProcessor.cpp. But PLArena is in NSPR and so we can't create a SizeOfExcludingThis() function because NSPR doesn't have access to the definition of nsMallocSizeOfFun. And there's no good place to put code shared by those two files. So I've just inlined the PLArenaPool measuring code twice, sigh. (BTW, the old measuring code in nsCSSRuleProcessor.cpp incorrectly measured the first PLArena in the PLArenaPool twice.)
Attachment #578963 -
Attachment is obsolete: true
Attachment #578963 -
Flags: review?(khuey)
Attachment #578975 -
Flags: review?(khuey)
Comment 3•13 years ago
|
||
Nicholas, the arena in the rule processor is unused and goes away in the patch for bug 689443. I wouldn't bother adding memory measurement stuff for it; it'll just beget merge conflicts, and won't measure anything since we're not allocating out of that arena. But for future reference, either nsLayoutUtils or nsContentUtils can work for code shared by two layout files...
Assignee | ||
Updated•13 years ago
|
Attachment #578963 -
Attachment is obsolete: false
Attachment #578963 -
Flags: review?(khuey)
Assignee | ||
Comment 4•13 years ago
|
||
Comment on attachment 578975 [details] [diff] [review] patch v2 In that case, we can revert to the first patch! Thanks, bz.
Attachment #578975 -
Attachment is obsolete: true
Attachment #578975 -
Flags: review?(khuey)
Assignee | ||
Updated•13 years ago
|
Blocks: DarkMatter
Comment on attachment 578963 [details] [diff] [review] patch Review of attachment 578963 [details] [diff] [review]: ----------------------------------------------------------------- r=me, but I think you should get a style system peer to look this over too. ::: layout/style/nsCSSRuleProcessor.cpp @@ +786,5 @@ > + return aMallocSizeOf(this, sizeof(RuleHash)) + > + SizeOfExcludingThis(aMallocSizeOf); > +} > + > + extra newline ::: layout/style/nsHTMLCSSStyleSheet.h @@ +93,5 @@ > + SizeOfIncludingThis(nsMallocSizeOfFun aMallocSizeOf) MOZ_OVERRIDE const > + { > + return aMallocSizeOf(this, sizeof(nsHTMLCSSStyleSheet)) + > + SizeOfExcludingThis(aMallocSizeOf); > + } We should probably move these into the .cpp file. ::: layout/style/nsIStyleRuleProcessor.h @@ +159,5 @@ > * Report the size of this style rule processor to about:memory. A > * processor may return 0. > */ > + virtual size_t SizeOfExcludingThis(nsMallocSizeOfFun mallocSizeOf) const = 0; > + virtual size_t SizeOfIncludingThis(nsMallocSizeOfFun mallocSizeOf) const = 0; This interface needs an IID change.
Attachment #578963 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 6•13 years ago
|
||
khuey wanted a review from a style peer. This version has minor fix-ups over the first version.
Attachment #578963 -
Attachment is obsolete: true
Attachment #579947 -
Flags: review?(bzbarsky)
Comment 7•13 years ago
|
||
Comment on attachment 579947 [details] [diff] [review] patch v2 Instead of const_cast, can we just make PL_DHashTableSizeOfExcludingThis take a const table? The code in PL_DHashTableSizeOfExcludingThis is what knows it'll never return REMOVE and hence can const_cast safely... r=me with that
Attachment #579947 -
Flags: review?(bzbarsky) → review+
Comment 8•13 years ago
|
||
Nicholas, the try run for this looks green, so I pushed it as https://hg.mozilla.org/integration/mozilla-inbound/rev/2d5421ea1758
Flags: in-testsuite-
Target Milestone: --- → mozilla11
Assignee | ||
Comment 9•13 years ago
|
||
Thanks!
Comment 10•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2d5421ea1758
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•