Closed Bug 1377329 Opened 7 years ago Closed 7 years ago

nsFrameList::GetLength() calls in nsDisplayListBuilder::MarkFramesForDisplayList() are slow

Categories

(Core :: Web Painting, enhancement, P1)

Product:
Core
Component:
Web Painting
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: u459114)

References

Details

(Keywords: perf)

Attachments

(1 file, 1 obsolete file)

Bug 1377329 - Enlarge the default capacity of nsDisplayBuilfer::mFramesMarkedForDisplayList.
59 bytes, text/x-review-board-request
mattwoodrow
: review+
Details 
I'm looking at the perf profile for Gmail in bug 1371668 comment 0 and
nsFrameList::GetLength() shows up there with a 11ms self-time.

nsFrameList::GetLength() is O(n) and should be avoided in performance
sensitive code.  It should really only be used for DEBUG code.

nsDisplayListBuilder::MarkFramesForDisplayList() uses it for an optimization(!):
http://searchfox.org/mozilla-central/rev/17ebac68112bd635b458e831670c1e506ebc17ad/layout/painting/nsDisplayList.cpp#1282
(from bug 999121)

I doubt that's an improvement over not setting SetCapacity at all.
We should collect statistics and benchmark this.
Whiteboard: [qf] → [qf:p1]
Flags: needinfo?(cku)
(bug 995267 comment 5 explained why we have this line of code.)
Assignee: nobody → cku
Flags: needinfo?(cku)
Status: NEW → ASSIGNED
Priority: -- → P1
For gmail case, nsFrameList::GetLength() is used at two place
1. nsDisplayListBuilder::MarkFramesForDisplayList
2. BCPaintBorderIterator::First() -> nsTableRowGroupFrame::GetRowCount()
Attachment #8886483 - Flags: review?(matt.woodrow)
Attachment #8886266 - Flags: review?(matt.woodrow)
Comment on attachment 8886483 [details]
Bug 1377329 - Part 1. Cache the length of frames in nsFrameList.

https://reviewboard.mozilla.org/r/157276/#review162718

I'm worried about how fragile this is likey to be. It's hard to verify that we're resetting the cached count at every location where its value might change.
It looks like nsFrameList::SetFrames is one where we need to reset and we aren't.

I wonder if we really need this.
Attachment #8886483 - Flags: review?(matt.woodrow)
Comment on attachment 8886266 [details]
Bug 1377329 - Enlarge the default capacity of nsDisplayBuilfer::mFramesMarkedForDisplayList.

https://reviewboard.mozilla.org/r/157032/#review162720

Increasing the capacity seems reasonable, but once we run out, calling GetLength still seems like a bad idea.

Can't we just rely on the nsTArray growth algorithm to do a decent job of resizing to something hopefully big enough?
Comment on attachment 8886266 [details]
Bug 1377329 - Enlarge the default capacity of nsDisplayBuilfer::mFramesMarkedForDisplayList.

https://reviewboard.mozilla.org/r/157032/#review162720

Hmn, yes, look fine by depending on nsTArray growth algorithm at [1]. Will update this patch

[1] http://searchfox.org/mozilla-central/source/xpcom/ds/nsTArray-inl.h#165
Attachment #8886483 - Attachment is obsolete: true
Comment on attachment 8886266 [details]
Bug 1377329 - Enlarge the default capacity of nsDisplayBuilfer::mFramesMarkedForDisplayList.

https://reviewboard.mozilla.org/r/157032/#review162760
Attachment #8886266 - Flags: review?(matt.woodrow) → review+
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/93af9545a6f4
Enlarge the default capacity of nsDisplayBuilfer::mFramesMarkedForDisplayList. r=mattwoodrow
https://hg.mozilla.org/mozilla-central/rev/93af9545a6f4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: