1368654 - Implement memory reporter support for FrameProperties

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Jonathan Kew [:jfkthame]]

Since bug 1368369 moved frame properties from a single hashtable in the presContext to be attached to the individual frames, they are no longer exposed in any way in about:memory. (Formerly, they contributed to the layout/pres-contexts number.)

Comment 1

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Kew (:jfkthame) from comment #0)
> Since bug 1368369 

Actually, that was bug 1365982. But bug 1368369 would be helped by having memory reporting.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: Implement memory reporter support for FrameProperties

This should make it easier to look at FrameProperties memory usage in real-world scenarios, without needing a custom-instrumented browser.

Comment 3

[Mats Palmgren (inactive)]

Comment on attachment 8872701 [details] [diff] [review]
Implement memory reporter support for FrameProperties

LGTM, r=mats

But now that I looked at FrameProperties::SizeOfExcludingThis again,
I think it's wrong:
http://searchfox.org/mozilla-central/source/layout/base/FrameProperties.cpp#101

We should return zero if mProperties.Length() <= 1 (for an AutoTArray<1>)
and aMallocSizeOf(mProperties.Elements()) otherwise.  IOW, SizeOfExcludingThis
should return the actual allocated size rather than what we currently use
of that allocated size.

Please fix that in a part 2 here or in a follow-up bug.

Comment 4

[Mats Palmgren (inactive)]

... actually I suspect Elements() doesn't return the start of the malloced
block, so it's probably aMallocSizeOf(mProperties.mHdr) or something like that.
Maybe it's just mProperties.ShallowSizeOfExcludingThis that we want here?

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to Mats Palmgren (:mats) from comment #4)
> Maybe it's just mProperties.ShallowSizeOfExcludingThis that we want here?

Yes, I believe that's correct. Part 2 coming up...

Comment 6

[Mats Palmgren (inactive)]

But as the comment on ShallowSizeOfExcludingThis says it just measures
the array storage, so we're underestimating the real memory used...
(it's good enough to compare nsTArray vs AutoTArray though)

I wonder if it would be possible to add a SizeOfExcludingThis to each
of the descriptors, returning sizeof(T) or something like that?
I think we have the real type there.  The *PROPERTY_SMALL_VALUE
properties should return zero though, since they fit in the array
entry.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - (Partially) fix SizeOf method in FrameProperties

This at least gets us back to as good as things were prior to the FrameProperties change.

Comment 8

[Jonathan Kew [:jfkthame]]

(In reply to Mats Palmgren (:mats) from comment #6)
> But as the comment on ShallowSizeOfExcludingThis says it just measures
> the array storage, so we're underestimating the real memory used...
> (it's good enough to compare nsTArray vs AutoTArray though)

And (FWIW) it's equivalent to what the old code for the FramePropertyTable implementation was doing.

> I wonder if it would be possible to add a SizeOfExcludingThis to each
> of the descriptors, returning sizeof(T) or something like that?
> I think we have the real type there.  The *PROPERTY_SMALL_VALUE
> properties should return zero though, since they fit in the array
> entry.

Maybe worth exploring, yes.

Comment 9

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/172218dd971dc5ede35cdb26a706124576c85efd
Bug 1368654 - pt 1 - Implement memory reporter support for FrameProperties. r=mats

https://hg.mozilla.org/integration/mozilla-inbound/rev/3947c1473651b4736840973914f83d655e3acbe3
Bug 1368654 - pt 2 - (Partially) fix SizeOf method in FrameProperties. r=mats

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/172218dd971d
https://hg.mozilla.org/mozilla-central/rev/3947c1473651