Make FrameLayerBuilder::DisplayItemEntry use an nsAutoTArray

RESOLVED FIXED in mozilla10

Status

()

Core
Layout
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

(Blocks: 1 bug)

unspecified
mozilla10
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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

See attachment 559017 [details].
(Assignee)

Updated

6 years ago
Blocks: 688532
(Assignee)

Comment 1

6 years ago
Created attachment 561887 [details] [diff] [review]
Patch v1
(Assignee)

Updated

6 years ago
Attachment #561887 - Attachment description: roc → Patch v1
Attachment #561887 - Flags: review?(roc)
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?
(Assignee)

Comment 3

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

Updated

6 years ago
Attachment #561887 - Flags: review?(roc) → review-
(Assignee)

Comment 4

6 years ago
Roc, can we bound the lifetime of these DisplayItemData objects?
They could live for quite a while ... they live until the next paint.
(Assignee)

Comment 6

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

Comment 7

6 years ago
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?
Yes, that would be absolutely horrible for memory usage.
(Assignee)

Comment 9

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

Comment 11

6 years ago
Created attachment 564935 [details] [diff] [review]
Patch v2
(Assignee)

Updated

6 years ago
Attachment #561887 - Attachment is obsolete: true
(Assignee)

Comment 12

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

Comment 13

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

Updated

6 years ago
Attachment #564935 - Flags: feedback?(roc)
(Assignee)

Comment 14

6 years ago
Comment on attachment 564935 [details] [diff] [review]
Patch v2

This doesn't crash and burn spectacularly on try.
Attachment #564935 - Flags: feedback?(roc) → review?(roc)
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.
Attachment #564935 - Flags: review?(roc) → review+
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.
(Assignee)

Comment 17

6 years ago
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
Whiteboard: [inbound]
Target Milestone: --- → mozilla10
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.
(Assignee)

Comment 19

6 years ago
Okay, I backed this out for now.  https://hg.mozilla.org/integration/mozilla-inbound/rev/19d6cb5f9593
(Assignee)

Updated

6 years ago
Whiteboard: [inbound]
Target Milestone: mozilla10 → ---
(Assignee)

Comment 20

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

Comment 21

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

Updated

6 years ago
Assignee: nobody → justin.lebar+bug
(Assignee)

Comment 22

6 years ago
Created attachment 565241 [details] [diff] [review]
Patch v3
Attachment #565241 - Flags: review?(roc)
Comment on attachment 565241 [details] [diff] [review]
Patch v3

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

Right. Thanks.
Attachment #565241 - Flags: review?(roc) → review+
(Assignee)

Updated

6 years ago
Attachment #564935 - Attachment is obsolete: true
(Assignee)

Comment 24

6 years ago
Inbound again.  Thanks, Roc.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5209955b95c9
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/5209955b95c9
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.