872367 - Add memory reporter for layers

Status: RESOLVED WORKSFORME

(Core :: Graphics: Layers, defect)

Description

[David Zbarsky (:dzbarsky)]

Attachment: Mark some things const

Comment 1

[Reuben Morais [:reuben]]

Comment on attachment 749640 [details] [diff] [review]
Mark some things const

Review of attachment 749640 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. There's a bunch of similar fixes around the code you touched, we should fix those too at some point.

Comment 2

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac067baa4bc7

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ac067baa4bc7

Comment 4

[David Zbarsky (:dzbarsky)]

Attachment: Add memory reporter

Comment 5

[David Zbarsky (:dzbarsky)]

Attachment: Add memory reporter

Comment 6

[Justin Lebar (not reading bugmail)]

Comment on attachment 750720 [details] [diff] [review]
Add memory reporter

Sorry for taking so long to get back to you on this, David.  I think (I hope!)
I'm done with tef, so I should be faster with iterations of this.

I have some specific comments below, but there's a bigger question here: How
much resolution do we want here?  I don't know anything about this code, but it
seems to me that if any of these data structures becomes large, we'll want to
know that it's specifically /that/ data structure that's large, not just that
"layers" is large.  (For example, we had that leak in the mAnimationData
array.)

Do you think more resolution would be helpful here?

>@@ -387,16 +391,19 @@ nsWindowMemoryReporter::CollectReports(n
>          "This is the sum of all windows' 'layout/style-contexts' numbers.");
> 
>   REPORT("window-objects/layout/style-sets", windowTotalSizes.mLayoutStyleSets,
>          "This is the sum of all windows' 'layout/style-sets' numbers.");
> 
>   REPORT("window-objects/layout/text-runs", windowTotalSizes.mLayoutTextRuns,
>          "This is the sum of all windows' 'layout/text-runs' numbers.");
> 
>+  REPORT("window-objects/layout/text-runs", windowTotalSizes.mLayersData,
>+         "This is the sum of all windows' 'layers' numbers.");

copy paste mistake

>diff --git a/gfx/2d/UserData.h b/gfx/2d/UserData.h
>--- a/gfx/2d/UserData.h
>+++ b/gfx/2d/UserData.h

>+  size_t SizeOfIncludingThis(nsMallocSizeOfFun aMallocSizeOf) const
>+  {
>+    return aMallocSizeOf(this) + count * aMallocSizeOf(entries);

I think you want just aMallocSizeOf(entries).

>diff --git a/gfx/layers/Layers.h b/gfx/layers/Layers.h
>--- a/gfx/layers/Layers.h
>+++ b/gfx/layers/Layers.h

>@@ -1435,18 +1440,18 @@ public:
>   virtual void FillSpecificAttributes(SpecificLayerAttributes& aAttrs);
> 
>   void SortChildrenBy3DZOrder(nsTArray<Layer*>& aArray);
> 
>   // These getters can be used anytime.
> 
>   virtual ContainerLayer* AsContainerLayer() { return this; }
> 
>-  virtual Layer* GetFirstChild() { return mFirstChild; }
>-  virtual Layer* GetLastChild() { return mLastChild; }
>+  virtual Layer* GetFirstChild() const { return mFirstChild; }
>+  virtual Layer* GetLastChild() const { return mLastChild; }

I'm not sure we can call these const functions if they return non-const
pointers to members...  But I'll defer to a gfx peer once we get the rest of
this stuff worked out.

>diff --git a/gfx/layers/Layers.cpp b/gfx/layers/Layers.cpp
>--- a/gfx/layers/Layers.cpp
>+++ b/gfx/layers/Layers.cpp

>@@ -154,16 +154,23 @@ LayerManager::CreateImageContainer()
> 
> already_AddRefed<ImageContainer>
> LayerManager::CreateAsynchronousImageContainer()
> {
>   nsRefPtr<ImageContainer> container = new ImageContainer(ImageContainer::ENABLE_ASYNC);
>   return container.forget();
> }
> 
>+size_t
>+LayerManager::SizeOfIncludingThis(nsMallocSizeOfFun aMallocSizeOf) const
>+{
>+  return aMallocSizeOf(this) + mRoot->SizeOfIncludingThis(aMallocSizeOf) +
>+    mUserData.SizeOfIncludingThis(aMallocSizeOf);

There are two problems with calling SizeOfIncludingThis (as opposed to
SizeOfExcludingThis) on mUserData.

One problem is that this causes us to double-count mUserData; it's already
counted in aMallocSizeOf(LayerManager).

But the other, bigger, problem is that it's unsafe to call aMallocSizeOf() on
something that wasn't alloc'ed with malloc() or operator new; at best you'll
get a garbage value, at worst you can crash.

>@@ -179,16 +186,41 @@ Layer::Layer(LayerManager* aManager, voi

>+size_t
>+Layer::SizeOfIncludingThis(nsMallocSizeOfFun aMallocSizeOf) const
>+{
>+  return aMallocSizeOf(this) + SizeOfExcludingThis(aMallocSizeOf);
>+}
>+
>+size_t
>+Layer::SizeOfExcludingThis(nsMallocSizeOfFun aMallocSizeOf) const
>+{
>+  size_t size = mUserData.SizeOfIncludingThis(aMallocSizeOf);

Similarly we can't call SizeOfIncludingThis on mUserData.

>+  if (mMaskLayer) {
>+    size += mMaskLayer->SizeOfIncludingThis(aMallocSizeOf);
>+  }
>+  if (mPendingTransform) {
>+    size += aMallocSizeOf(mPendingTransform);
>+  }
>+  for (uint32_t i = 0; i < mAnimationData.Length(); i++) {
>+    for (uint32_t j = 0; j < mAnimationData[i].mFunctions.Length(); j++) {
>+      size += aMallocSizeOf(mAnimationData[i].mFunctions[j]);
>+    }
>+  }

Do you think we should also count the memory taken up by mAnimationData and
mAnimationData[i].mFunctions?

>@@ -836,17 +868,28 @@ ContainerLayer::DidInsertChild(Layer* aL

>+size_t
>+ContainerLayer::SizeOfExcludingThis(nsMallocSizeOfFun aMallocSizeOf) const
>+{
>+  size_t size = aMallocSizeOf(this) + Layer::SizeOfExcludingThis(aMallocSizeOf);

If this is SizeOfExcludingThis, it shouldn't include mallocSizeOf(this).

Comment 7

[David Zbarsky (:dzbarsky)]

(In reply to Justin Lebar [:jlebar] from comment #6)
> Comment on attachment 750720 [details] [diff] [review]
> Add memory reporter

> 
> I have some specific comments below, but there's a bigger question here: How
> much resolution do we want here?  I don't know anything about this code, but
> it
> seems to me that if any of these data structures becomes large, we'll want to
> know that it's specifically /that/ data structure that's large, not just that
> "layers" is large.  (For example, we had that leak in the mAnimationData
> array.)

Most of the thing aren't arrays, so they can't get arbitrarily large.  I think this should be ok for a first pass.

> 
> >diff --git a/gfx/2d/UserData.h b/gfx/2d/UserData.h
> >--- a/gfx/2d/UserData.h
> >+++ b/gfx/2d/UserData.h
> 
> >+  size_t SizeOfIncludingThis(nsMallocSizeOfFun aMallocSizeOf) const
> >+  {
> >+    return aMallocSizeOf(this) + count * aMallocSizeOf(entries);
> 
> I think you want just aMallocSizeOf(entries).

Ah, I forgot that aMallocSizeOf checks the size we called realloc with.  Good call.

> I'm not sure we can call these const functions if they return non-const
> pointers to members...  But I'll defer to a gfx peer once we get the rest of
> this stuff worked out.

We're just using them for iteration, so it should be ok.  If we want to return const pointers we'll probably need to provide 2 versions of each.


> There are two problems with calling SizeOfIncludingThis (as opposed to
> SizeOfExcludingThis) on mUserData.
> 
> One problem is that this causes us to double-count mUserData; it's already
> counted in aMallocSizeOf(LayerManager).

Those are different UserDatas.

> But the other, bigger, problem is that it's unsafe to call aMallocSizeOf() on
> something that wasn't alloc'ed with malloc() or operator new; at best you'll
> get a garbage value, at worst you can crash.

So I guess we want UserData to have SizeOfExcludingThis so we can still count aMallocSizeOf(entries).


> 
> Do you think we should also count the memory taken up by mAnimationData and
> mAnimationData[i].mFunctions?
> 

It shouldn't be much, but yeah, we should.

Comment 8

[Justin Lebar (not reading bugmail)]

I have a pretty strong bias towards splitting things out as much as we reasonably can, given that it's feasible to split things out here and given that a high "layers" number would not help us much in diagnosing a bug.

> Most of the thing aren't arrays, so they can't get arbitrarily large.

What does get large here?

Comment 9

[David Zbarsky (:dzbarsky)]

(In reply to Justin Lebar [:jlebar] from comment #8)
> I have a pretty strong bias towards splitting things out as much as we
> reasonably can, given that it's feasible to split things out here and given
> that a high "layers" number would not help us much in diagnosing a bug.
> 

Ok, I can split things up.

> > Most of the thing aren't arrays, so they can't get arbitrarily large.
> 
> What does get large here?

We may just end up with a large number of layers so total memory could be high.  We could also potentially be storing a lot of things in UserData if we don't clear it for some reason.

Comment 10

[David Zbarsky (:dzbarsky)]

(In reply to David Zbarsky (:dzbarsky) from comment #7)
> (In reply to Justin Lebar [:jlebar] from comment #6)
> > Comment on attachment 750720 [details] [diff] [review]

> 
> > 
> > Do you think we should also count the memory taken up by mAnimationData and
> > mAnimationData[i].mFunctions?
> > 
> 
> It shouldn't be much, but yeah, we should.

Actually wouldn't those be counted in aMallocSizeOf(this)?

Comment 11

[Justin Lebar (not reading bugmail)]

malloc_size_of_this counts the amount of memory allocated when we did |new Foo()|.  It's the same as sizeof(*this) rounded up to malloc's next bucket size.

mAnimationData is an InfallibleTArray member of this.  So aMallocSizeOf(this) counts sizeof(InfallibleTArray), but it does not count the heap memory allocated by the TArray.  It also doesn't count the heap memory allocated by the TArray's members.

Comment 12

[Nicholas Nethercote [inactive]]

Comment on attachment 750720 [details] [diff] [review]
Add memory reporter

Review of attachment 750720 [details] [diff] [review]:
-----------------------------------------------------------------

I'm back from vacation and happy to review subsequent iterations of this patch, to lighten jlebar's load.

::: dom/base/nsWindowMemoryReporter.cpp
@@ +243,5 @@
>           "Memory used for the PresContext in the PresShell's frame "
>           "within a window.");
>    aWindowTotalSizes->mLayoutPresContext += windowSizes.mLayoutPresContext;
>  
> +  REPORT("/layers", windowSizes.mLayersData,

The string should be "/layout/layers", AFAICT.

Comment 13

[Justin Lebar (not reading bugmail)]

I don't have any 1.0.1 work on my plate, so I'm happy to keep working on this while you dig yourself out from under your bugmail.

Comment 14

[Nicholas Nethercote [inactive]]

> > windowSizes.mLayersData,

Oh, and the names of the members in this class mirror the strings, so I think this should be |mLayers| rather than |mLayersData|.

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.

Comment 16

[Andrew McCreight [:mccr8]]

We have some kind of memory reporting for layers, so I think we can close this. A patch did land as part of this bug, but it was just marking some arguments as const.