Use mallocSizeOf in the layout memory reporters

RESOLVED FIXED in mozilla11

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks: 1 bug)

unspecified
mozilla11
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
I plan to convert the existing style memory reporters to use the new mallocSizeOf style.
(Assignee)

Updated

6 years ago
Whiteboard: [MemShrink] → [MemShrink:P2]
(Assignee)

Updated

6 years ago
Summary: Use mallocSizeOf in the style memory reporters → Use mallocSizeOf in the layout memory reporters
(Assignee)

Comment 1

6 years ago
Created attachment 578963 [details] [diff] [review]
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.
Attachment #578963 - Flags: review?(khuey)
(Assignee)

Comment 2

6 years ago
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.)
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...
(Assignee)

Updated

6 years ago
Attachment #578963 - Attachment is obsolete: false
Attachment #578963 - Flags: review?(khuey)
(Assignee)

Comment 4

6 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

6 years ago
Blocks: 707865
(Assignee)

Updated

6 years ago
Blocks: 563700
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

6 years ago
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.
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 https://hg.mozilla.org/integration/mozilla-inbound/rev/2d5421ea1758
Flags: in-testsuite-
Target Milestone: --- → mozilla11
(Assignee)

Comment 9

6 years ago
Thanks!
https://hg.mozilla.org/mozilla-central/rev/2d5421ea1758
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.