705987 - Use mallocSizeOf in the layout memory reporters

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Nicholas Nethercote [inactive]]

I plan to convert the existing style memory reporters to use the new mallocSizeOf style.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: patch

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.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: 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

[Boris Zbarsky [:bzbarsky]]

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

[Nicholas Nethercote [inactive]]

Comment on attachment 578975 [details] [diff] [review]
patch v2

In that case, we can revert to the first patch!  Thanks, bz.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: patch v2

khuey wanted a review from a style peer.  This version has minor fix-ups over the first version.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

Nicholas, the try run for this looks green, so I pushed it as https://hg.mozilla.org/integration/mozilla-inbound/rev/2d5421ea1758

Comment 9

[Nicholas Nethercote [inactive]]

Thanks!

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2d5421ea1758