Closed Bug 705987 Opened 8 years ago Closed 8 years ago

Use mallocSizeOf in the layout memory reporters


(Core :: CSS Parsing and Computation, defect)

Not set





(Reporter: njn, Assigned: njn)


(Blocks 1 open bug)


(Whiteboard: [MemShrink:P2])


(1 file, 2 obsolete files)

I plan to convert the existing style memory reporters to use the new mallocSizeOf style.
Whiteboard: [MemShrink] → [MemShrink:P2]
Summary: Use mallocSizeOf in the style memory reporters → Use mallocSizeOf in the layout memory reporters
Attached patch patch (obsolete) — Splinter 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.
Attachment #578963 - Flags: review?(khuey)
Attached patch patch v2 (obsolete) — Splinter Review
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)
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...
Attachment #578963 - Attachment is obsolete: false
Attachment #578963 - Flags: review?(khuey)
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)
Blocks: 707865
Blocks: DarkMatter
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.
Attachment #578963 - Flags: review?(khuey) → review+
Attached patch patch v2Splinter Review
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 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+
Nicholas, the try run for this looks green, so I pushed it as
Flags: in-testsuite-
Target Milestone: --- → mozilla11
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.