Last Comment Bug 705987 - Use mallocSizeOf in the layout memory reporters
: Use mallocSizeOf in the layout memory reporters
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla11
Assigned To: Nicholas Nethercote [:njn]
: Jet Villegas (:jet)
Depends on:
Blocks: DarkMatter 700914 707865
  Show dependency treegraph
Reported: 2011-11-28 21:50 PST by Nicholas Nethercote [:njn]
Modified: 2011-12-10 20:38 PST (History)
4 users (show)
bzbarsky: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (31.87 KB, patch)
2011-12-04 17:36 PST, Nicholas Nethercote [:njn]
khuey: review+
Details | Diff | Splinter Review
patch v2 (31.96 KB, patch)
2011-12-04 20:59 PST, Nicholas Nethercote [:njn]
no flags Details | Diff | Splinter Review
patch v2 (33.50 KB, patch)
2011-12-07 19:33 PST, Nicholas Nethercote [:njn]
bzbarsky: review+
Details | Diff | Splinter Review

Description User image Nicholas Nethercote [:njn] 2011-11-28 21:50:17 PST
I plan to convert the existing style memory reporters to use the new mallocSizeOf style.
Comment 1 User image Nicholas Nethercote [:njn] 2011-12-04 17:36:46 PST
Created attachment 578963 [details] [diff] [review]

This patch converts the layout/* reporters to use mallocSizeOf.
( 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.
Comment 2 User image Nicholas Nethercote [:njn] 2011-12-04 20:59:32 PST
Created attachment 578975 [details] [diff] [review]
patch v2

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.)
Comment 3 User image Boris Zbarsky [:bz] (still a bit busy) 2011-12-04 21:46:53 PST
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...
Comment 4 User image Nicholas Nethercote [:njn] 2011-12-04 21:54:33 PST
Comment on attachment 578975 [details] [diff] [review]
patch v2

In that case, we can revert to the first patch!  Thanks, bz.
Comment 5 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-12-07 12:31:34 PST
Comment on attachment 578963 [details] [diff] [review]

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.
Comment 6 User image Nicholas Nethercote [:njn] 2011-12-07 19:33:47 PST
Created attachment 579947 [details] [diff] [review]
patch v2

khuey wanted a review from a style peer.  This version has minor fix-ups over the first version.
Comment 7 User image Boris Zbarsky [:bz] (still a bit busy) 2011-12-07 21:37:04 PST
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
Comment 8 User image Boris Zbarsky [:bz] (still a bit busy) 2011-12-09 02:30:03 PST
Nicholas, the try run for this looks green, so I pushed it as
Comment 9 User image Nicholas Nethercote [:njn] 2011-12-09 03:09:18 PST
Comment 10 User image Ed Morley [:emorley] 2011-12-10 20:38:19 PST

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