Last Comment Bug 688619 - Make FrameLayerBuilder::DisplayItemEntry use an nsAutoTArray
: Make FrameLayerBuilder::DisplayItemEntry use an nsAutoTArray
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla10
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
Depends on:
Blocks: needs-more-autoarray
  Show dependency treegraph
 
Reported: 2011-09-22 14:38 PDT by Justin Lebar (not reading bugmail)
Modified: 2011-10-07 04:08 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (1.78 KB, patch)
2011-09-22 14:38 PDT, Justin Lebar (not reading bugmail)
justin.lebar+bug: review-
Details | Diff | Review
Patch v2 (17.38 KB, patch)
2011-10-05 11:34 PDT, Justin Lebar (not reading bugmail)
roc: review+
Details | Diff | Review
Patch v3 (17.10 KB, patch)
2011-10-06 08:36 PDT, Justin Lebar (not reading bugmail)
roc: review+
Details | Diff | Review

Description Justin Lebar (not reading bugmail) 2011-09-22 14:38:11 PDT
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) 2011-09-22 14:38:55 PDT
Created attachment 561887 [details] [diff] [review]
Patch v1
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-09-22 17:01:10 PDT
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) 2011-09-22 21:03:00 PDT
(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) 2011-09-23 11:00:33 PDT
Roc, can we bound the lifetime of these DisplayItemData objects?
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-09-24 00:11:44 PDT
They could live for quite a while ... they live until the next paint.
Comment 6 Justin Lebar (not reading bugmail) 2011-10-04 07:44:32 PDT
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) 2011-10-04 10:16:30 PDT
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) (Exited; email my personal email if necessary) 2011-10-04 16:06:48 PDT
Yes, that would be absolutely horrible for memory usage.
Comment 9 Justin Lebar (not reading bugmail) 2011-10-04 18:05:11 PDT
(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) (Exited; email my personal email if necessary) 2011-10-04 18:14:22 PDT
(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) 2011-10-05 11:34:28 PDT
Created attachment 564935 [details] [diff] [review]
Patch v2
Comment 12 Justin Lebar (not reading bugmail) 2011-10-05 11:38:23 PDT
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) 2011-10-05 11:39:26 PDT
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) 2011-10-05 13:44:44 PDT
Comment on attachment 564935 [details] [diff] [review]
Patch v2

This doesn't crash and burn spectacularly on try.
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-10-05 15:48:33 PDT
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) (Exited; email my personal email if necessary) 2011-10-05 15:53:21 PDT
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) 2011-10-05 17:18:06 PDT
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) (Exited; email my personal email if necessary) 2011-10-05 17:58:10 PDT
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) 2011-10-05 18:03:35 PDT
Okay, I backed this out for now.  https://hg.mozilla.org/integration/mozilla-inbound/rev/19d6cb5f9593
Comment 20 Justin Lebar (not reading bugmail) 2011-10-06 07:39:31 PDT
(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) 2011-10-06 08:15:30 PDT
> 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) 2011-10-06 08:36:47 PDT
Created attachment 565241 [details] [diff] [review]
Patch v3
Comment 23 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-10-06 14:20:00 PDT
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) 2011-10-06 14:36:48 PDT
Inbound again.  Thanks, Roc.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5209955b95c9
Comment 25 Ed Morley [:emorley] 2011-10-07 04:08:12 PDT
https://hg.mozilla.org/mozilla-central/rev/5209955b95c9

Note You need to log in before you can comment on or make changes to this bug.