Closed Bug 1368654 Opened 5 years ago Closed 5 years ago

Implement memory reporter support for FrameProperties

Categories

(Core :: Layout, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

References

Details

Attachments

(2 files)

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.)
(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.
This should make it easier to look at FrameProperties memory usage in real-world scenarios, without needing a custom-instrumented browser.
Attachment #8872701 - Flags: review?(mats)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
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.
Attachment #8872701 - Flags: review?(mats) → review+
... 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?
(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...
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.
This at least gets us back to as good as things were prior to the FrameProperties change.
Attachment #8872771 - Flags: review?(mats)
(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.
Attachment #8872771 - Flags: review?(mats) → review+
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
https://hg.mozilla.org/mozilla-central/rev/172218dd971d
https://hg.mozilla.org/mozilla-central/rev/3947c1473651
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.