1363919 - spend too much time getting frame properties in FrameLayerBuilder::GetPaintedLayerScale in order to snap scroll areas to layer pixels

Status: RESOLVED WORKSFORME

(Core :: Layout, enhancement)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I just took a profile of scrolling my facebook timeline.  I noticed that about 0.1% of the total time in the profile (including idle) was getting frame properties in FrameLayerBuilder::GetPaintedLayerScaleForFrame.  This is called a *lot* because we call it multiple times when we reflow a scroll frame, as a result of bug 1012752:

nsHTMLScrollFrame::Reflow -> ScrollFrameHelper::GetScrolledRect -> FrameLayerBuilder::GetPaintedLayerScaleForFrame

ScrollFrameHelper::ReflowFinished -> ScrollFrameHelper::ScrollToImpl -> FrameLayerBuilder::GetPaintedLayerScaleForFrame

It seems like we could consider avoiding this computation, e.g., by:
 * guarding the LayerManagerDataProperty() with a frame state bit (or bool:1 on nsIFrame if we're out of bits) that says that we have it, if my suspicion that most frames don't have it is correct
 * storing the scale in the ReflowInput during reflow (and mostly copying from parent to child) -- although I'm not sure how this helps with the ReflowFinished case

Or maybe there's something more clever we can do based on where this scale changes?  Any ideas, Markus?

Comment 1

[Markus Stange [:mstange]]

See bug 1293031 comment 17 - I think some caching should help a lot here. Milan was playing with this a little but I don't know how far he got. I'll ask him tomorrow.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It looks like tn wrote a patch in bug 1159459 that fixes this, but couldn't measure any improvement so didn't land it.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I wonder if we could avoid the pixel-snapping accuracy when we're:
 * determining the height of the scrollbar
 * scrolling somewhere not near the edge of the scrollport

Comment 4

[Daniel Holbert [:dholbert]]

I think tnikkel was going to look into resurrecting his patch.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Probably caching the information we need, and then invalidating it appropriately, is the best option, though.

Comment 6

[Jet Villegas (inactive)]

load-balance to jwatt

Comment 7

[Jean Gong :jgong]

No reply from Jwatt on ETA for this bug to be fixed within fx57. Moving to qf:p2

Comment 8

[Jonathan Watt [:jwatt]]

Sorry, Jean, I didn't see a question regarding ETA. I hope to have some patches up for review shortly though.

Comment 9

[Markus Stange [:mstange]]

Note that the work in bug 1363922 may have made this obsolete.

Comment 10

[Jonathan Watt [:jwatt]]

Yeah, I'm not seeing this on Windows 10 any more.