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

RESOLVED FIXED in Firefox 56

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mats, Assigned: u459114)

Tracking

({perf})

Trunk
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

2 years ago
(bug 995267 comment 5 explained why we have this line of code.)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Assignee: nobody → cku
Flags: needinfo?(cku)
Status: NEW → ASSIGNED
Priority: -- → P1
(Assignee)

Comment 4

2 years ago
For gmail case, nsFrameList::GetLength() is used at two place
1. nsDisplayListBuilder::MarkFramesForDisplayList
2. BCPaintBorderIterator::First() -> nsTableRowGroupFrame::GetRowCount()
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

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

Comment 14

2 years ago
mozreview-review-reply
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
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
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+

Comment 17

2 years ago
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
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.