688619 - Make FrameLayerBuilder::DisplayItemEntry use an nsAutoTArray

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Justin Lebar (not reading bugmail)]

On my nytimes/slashdot/gmail benchmark, this one nsTArray accounts for 5% of all mallocs in the browser.

See attachment 559017 [details].

Comment 1

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This makes the hashtable entry go from 4 to 44 bytes (32-bit) IIUC. I'm not 100% sure this is a good change.

Once we've built the hashtable, we attach the array to the frame it belongs to in FrameLayerBuilder::UpdateDisplayItemDataForFrame (see the ccall to SwapElements). Doesn't that mean your patch ends up doing the memory allocations there instead of earlier, not really saving us anything?

Comment 3

[Justin Lebar (not reading bugmail)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> This makes the hashtable entry go from 4 to 44 bytes (32-bit) IIUC. I'm not
> 100% sure this is a good change.

DisplayItemDataEntry inherits from nsPtrHashKey<nsIFrame>, so it's currently two words, one for the nsIFrame pointer, and one for the nsTArray.

DisplayItemData is nsRefPtr | PRUint32 | enum, so 12/16 bytes on 32/64-bit.  And nsAutoTArray is 16 bytes.

So the hashtable entry goes from 8/16 bytes to 2 * (12/16) + 16 = 40/48 bytes.  I think?

That's a big difference, for sure.

> Once we've built the hashtable, we attach the array to the frame it belongs
> to in FrameLayerBuilder::UpdateDisplayItemDataForFrame (see the ccall to
> SwapElements). Doesn't that mean your patch ends up doing the memory
> allocations there instead of earlier, not really saving us anything?

Hm, and it calls SwapElements with one of these asserted-to-be-sizeof-void* nsTArrays...

Thanks for the feedback!  I'll see if anything can be done here...

Comment 4

[Justin Lebar (not reading bugmail)]

Roc, can we bound the lifetime of these DisplayItemData objects?

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

They could live for quite a while ... they live until the next paint.

Comment 6

[Justin Lebar (not reading bugmail)]

This one tops the list of most malloc'y sites in the browser, accounting for about 3% of all mallocs (by count, not by size).  The only thing which comes close is hunspell (bug 691176), although that's a one-time fee.

Comment 7

[Justin Lebar (not reading bugmail)]

What if I put an nsAutoTArray<DisplayItemData, 1> on nsIFrame and used that array, instead of the frame's properties, for storing the DisplayItemData?  Would that be a Bad Idea?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, that would be absolutely horrible for memory usage.

Comment 9

[Justin Lebar (not reading bugmail)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
> Yes, that would be absolutely horrible for memory usage.

...because although most nsIFrames have a DisplayItemData attached at some point, most don't at any one point in time?  Or because most nsIFrames never have DisplayItemData attached?

Not to beg the question as to whether a fix is desired, if we fix this, I think it's going to be by either:

 * storing the nsTArray<DisplayItemData> somewhere other than in the frame's properties, since we can't stick an auto array into the properties, or

 * allocating the nsTArray<DisplayItemData>s' elements from an arena.

To do either, we have to find object onto which we can attach storage...

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Justin Lebar [:jlebar] from comment #9)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
> > Yes, that would be absolutely horrible for memory usage.
> 
> ...because although most nsIFrames have a DisplayItemData attached at some
> point, most don't at any one point in time?  Or because most nsIFrames never
> have DisplayItemData attached?

Only frames that are displayed have a DisplayItemData attached. So if you have a large document, only a small fraction of frames have a DisplayItemData attached at any given time.

>  * storing the nsTArray<DisplayItemData> somewhere other than in the frame's
> properties, since we can't stick an auto array into the properties, or
> 
>  * allocating the nsTArray<DisplayItemData>s' elements from an arena.
> 
> To do either, we have to find object onto which we can attach storage...

Putting them into an arena that we destroy at the next paint makes the most sense to me.

Actually we probably need two arenas, because we need to keep the entries for the last paint around while we construct the next one. The arenas can live in LayerManagerData.

Comment 11

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2

Comment 12

[Justin Lebar (not reading bugmail)]

Comment on attachment 564935 [details] [diff] [review]
Patch v2

I'm kind of surprised, but this seems to work on my machine.  I've pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=a87ffa614ab1

Comment 13

[Justin Lebar (not reading bugmail)]

Oh, and the patch disappears current the allocation hotspot without creating a new one (inasmuch as I can tell).

Comment 14

[Justin Lebar (not reading bugmail)]

Comment on attachment 564935 [details] [diff] [review]
Patch v2

This doesn't crash and burn spectacularly on try.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 564935 [details] [diff] [review]
Patch v2

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

This is great! I should have done this in the first place.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We could do additional work to make this more efficient. The LayerManagerProperty is a little bit superfluous since if present, it's almost always going to match frame->PresContext()->PresShell()->GetLayerManager(). (It only won't match that if the frame is inside a popup menu, combobox dropdown or XUL panel.) It also serves the function of notifying FrameLayerBuilder when a frame is deleted, but we could notify directly in nsFrame::Destroy.

But it would add some complexity to take advantage of this, and your patch should be a clear win, so let's leave it for now.

Comment 17

[Justin Lebar (not reading bugmail)]

Thanks for the r+, roc!  And thanks for being patient with me as I tried to figure things out.

Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/153237ff0f46

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually, I uh had some extra comments after thinking about it :-)

I think we might have a problem with layer managers being destroyed and leaving dangling pointers in the frame property. You should fix this by iterating through the array when we destroy the layer manager user data and removing the properties from the frames.

Also I think we can make this a little more efficient by storing a pointer to the layer manager user data in the frame property, instead of the layer manager itself.

Comment 19

[Justin Lebar (not reading bugmail)]

Okay, I backed this out for now.  https://hg.mozilla.org/integration/mozilla-inbound/rev/19d6cb5f9593

Comment 20

[Justin Lebar (not reading bugmail)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18)
> I think we might have a problem with layer managers being destroyed and
> leaving dangling pointers in the frame property.

Can I write a test for this?  I'm not sure how.

Comment 21

[Justin Lebar (not reading bugmail)]

> I think we might have a problem with layer managers being destroyed and leaving dangling pointers in > the frame property. You should fix this by iterating through the array when we destroy the layer 
> manager user data and removing the properties from the frames.

I think this already happens.  ~LayerManagerData() calls mFramesWithLayers.EnumerateEntries(RemoveDisplayItemForFrame) which, among other things, zeroes out the property.

Comment 22

[Justin Lebar (not reading bugmail)]

Attachment: Patch v3

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 565241 [details] [diff] [review]
Patch v3

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

Right. Thanks.

Comment 24

[Justin Lebar (not reading bugmail)]

Inbound again.  Thanks, Roc.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5209955b95c9

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5209955b95c9