Closed
Bug 688619
Opened 13 years ago
Closed 13 years ago
Make FrameLayerBuilder::DisplayItemEntry use an nsAutoTArray
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla10
People
(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)
References
(Blocks 1 open bug)
Details
(Whiteboard: [inbound])
Attachments
(1 file, 2 obsolete files)
17.10 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
On my nytimes/slashdot/gmail benchmark, this one nsTArray accounts for 5% of all mallocs in the browser.
See attachment 559017 [details].
Assignee | ||
Updated•13 years ago
|
Blocks: needs-more-autoarray
Assignee | ||
Comment 1•13 years ago
|
||
Assignee | ||
Updated•13 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•13 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•13 years ago
|
Attachment #561887 -
Flags: review?(roc) → review-
Assignee | ||
Comment 4•13 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•13 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•13 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•13 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•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #561887 -
Attachment is obsolete: true
Assignee | ||
Comment 12•13 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•13 years ago
|
||
Oh, and the patch disappears current the allocation hotspot without creating a new one (inasmuch as I can tell).
Assignee | ||
Updated•13 years ago
|
Attachment #564935 -
Flags: feedback?(roc)
Assignee | ||
Comment 14•13 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•13 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•13 years ago
|
||
Okay, I backed this out for now. https://hg.mozilla.org/integration/mozilla-inbound/rev/19d6cb5f9593
Assignee | ||
Updated•13 years ago
|
Whiteboard: [inbound]
Target Milestone: mozilla10 → ---
Assignee | ||
Comment 20•13 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•13 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•13 years ago
|
Assignee: nobody → justin.lebar+bug
Assignee | ||
Comment 22•13 years ago
|
||
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•13 years ago
|
Attachment #564935 -
Attachment is obsolete: true
Assignee | ||
Comment 24•13 years ago
|
||
Inbound again. Thanks, Roc. https://hg.mozilla.org/integration/mozilla-inbound/rev/5209955b95c9
Whiteboard: [inbound]
Comment 25•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5209955b95c9
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in
before you can comment on or make changes to this bug.
Description
•