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

Status: RESOLVED FIXED

(Core :: Web Painting, enhancement, P1)

Description

[Mats Palmgren (inactive)]

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.

Comment 1

[u459114]

(bug 995267 comment 5 explained why we have this line of code.)

Comment 2

[u459114]

Attachment: Bug 1377329 - Enlarge the default capacity of nsDisplayBuilfer::mFramesMarkedForDisplayList.

Review commit: https://reviewboard.mozilla.org/r/157032/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/157032/

Comment 3

[u459114]

Comment on attachment 8886266 [details]
Bug 1377329 - Enlarge the default capacity of nsDisplayBuilfer::mFramesMarkedForDisplayList.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157032/diff/1-2/

Comment 4

[u459114]

For gmail case, nsFrameList::GetLength() is used at two place
1. nsDisplayListBuilder::MarkFramesForDisplayList
2. BCPaintBorderIterator::First() -> nsTableRowGroupFrame::GetRowCount()

Comment 5

[u459114]

Attachment: Bug 1377329 - Part 1. Cache the length of frames in nsFrameList.

nsTableRowGroupFrame::GetRowCount() also uses nsFrameList::GetLength(). In this
scenario, since using nsFrameList::GetLength() is not preventable, we can cache
the length of frames inside nsFrameList to reduce time spend on tracing frame
link list.

Review commit: https://reviewboard.mozilla.org/r/157276/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/157276/

Comment 6

[u459114]

Comment on attachment 8886266 [details]
Bug 1377329 - Enlarge the default capacity of nsDisplayBuilfer::mFramesMarkedForDisplayList.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157032/diff/2-3/

Comment 7

[u459114]

Comment on attachment 8886483 [details]
Bug 1377329 - Part 1. Cache the length of frames in nsFrameList.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157276/diff/1-2/

Comment 8

[u459114]

Comment on attachment 8886266 [details]
Bug 1377329 - Enlarge the default capacity of nsDisplayBuilfer::mFramesMarkedForDisplayList.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157032/diff/3-4/

Comment 9

[u459114]

Comment on attachment 8886483 [details]
Bug 1377329 - Part 1. Cache the length of frames in nsFrameList.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157276/diff/2-3/

Comment 10

[u459114]

Comment on attachment 8886266 [details]
Bug 1377329 - Enlarge the default capacity of nsDisplayBuilfer::mFramesMarkedForDisplayList.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157032/diff/4-5/

Comment 11

[u459114]

Comment on attachment 8886266 [details]
Bug 1377329 - Enlarge the default capacity of nsDisplayBuilfer::mFramesMarkedForDisplayList.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157032/diff/5-6/

Comment 12

[Matt Woodrow (:mattwoodrow)]

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.

Comment 13

[Matt Woodrow (:mattwoodrow)]

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 14

[u459114]

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 15

[u459114]

Comment on attachment 8886266 [details]
Bug 1377329 - Enlarge the default capacity of nsDisplayBuilfer::mFramesMarkedForDisplayList.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157032/diff/6-7/

Comment 16

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8886266 [details]
Bug 1377329 - Enlarge the default capacity of nsDisplayBuilfer::mFramesMarkedForDisplayList.

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/93af9545a6f4
Enlarge the default capacity of nsDisplayBuilfer::mFramesMarkedForDisplayList. r=mattwoodrow

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/93af9545a6f4