Bug 86383 – Scrolling is sluggish on pages with many relative elements

• Status: NEW
• Component: CSS

[Lars Clausen]

Created attachment 141752 [details]
Test case

The attached example scrolls slowly and sluggishly on WebKit (both Linux/Chrome, Mac/Chrome and Mac/Safari) browsers, but fine in Firefox. The presence of a bunch of layered divs as well as position:relative and display:block on the inner elements seem necessary for this bug to trigger.

[Shane Stephens]

Scrolling of the provided test case is pretty smooth on Chrome Canary. At any rate, scrolling sluggishness is a bug in the browser(s) rather than in WebKit, so I'm going to close this.

[Lars Clausen]

It's not smooth at all on Chrome Canary (just got Mike West to help me try it) nor on Safari 5.1.7. If you in Chrome do "Inspect Element" on one of the lines and untick "position: relative", you can see how much a difference it makes. Note that you have to hold down the arrow or pagedown key to see the effect.

We had one user say "The javascript is so heavy that it is usually a PITA to do anything these days." about cs/, and this was one of the problems he'd encountered. Not that he was right about the javascript usage, but he was right about the scrolling.

[Shane Stephens]

You're right, it isn't smooth. I don't know what happened last time I tried the test.

Is this something we can trace back to a slow/inefficient code path in WebKit, or is it a bug in the browsers?

[Lars Clausen]

Since both Chrome and Safari behave in exactly the same way, and Firefox doesn't, I expect it's a webkit issue. But I don't know first thing about WebKit, so I can't say much more than that.

[Sam Weinig]

(In reply to comment #1)
> Scrolling of the provided test case is pretty smooth on Chrome Canary. At any rate, scrolling sluggishness is a bug in the browser(s) rather than in WebKit, so I'm going to close this.

Scrolling performance is very much in the realm of WebKit, I am not sure where you got the idea that it is not.

[Allan Sandfeld Jensen]

After profiling it, it looks like most of the time spend scrolling is spend inside
RenderBox::mapLocalToContainer  and RenderLayer::convertToLayerCoords.

RenderBox::mapLocalToContainer is called indirectly RenderLayer::computeRepaintRects.

RenderLayer::convertToLayerCoords is called from several places.

[Allan Sandfeld Jensen]

(In reply to comment #6)
> After profiling it, it looks like most of the time spend scrolling is spend inside
> RenderBox::mapLocalToContainer  and RenderLayer::convertToLayerCoords.
> 
> RenderBox::mapLocalToContainer is called indirectly RenderLayer::computeRepaintRects.
> 
> RenderLayer::convertToLayerCoords is called from several places.

Specifically it is RenderBox::outlineBoundsForRepaint that is the problem. clippedOverflowRectForRepaint are fast enough, but outlineBoundsForRepaint is really slow, probably because it uses a generic method to build up the transformation matrix and applies it, instead of using a custom build algorithm like RenderBox::computeRectForRepaint does.

[Allan Sandfeld Jensen]

Created attachment 160444 [details]
Patch

Avoid recalculating the outline bounds on scroll, this fixes the biggest thorn in slow scrolling with many layers.

[Allan Sandfeld Jensen]

Eventhough my patch fixes the largest part of the problem, there is a bigger underlying problem. We apparently do recognize that this case can be scrolled trivially, which we really should. I propose treating my patch as just a regression fix, but leave the bug open, or open a new bug on not recognizing trivial scrolling cases.

Note I suspect it is a regression since QtWebKit 2.2 is much faster with the test-case, and QtWebKit 2.2 is based of webkit trunk around 1 year ago, but there could be something else going on. I haven't looked at the code difference.

[Simon Fraser (smfr)]

Created attachment 160605 [details]
Sample

When I load the testcase in Safari and try to scroll, the sample shows that we spend a lot of time in RenderLayer::calculateRects() under hitTestLayer(), and that calls RenderLayer::backgroundClipRect(). I don't think your patch is going to improve this.

[Allan Sandfeld Jensen]

(In reply to comment #10)
> Created an attachment (id=160605) [details]
> Sample
> 
> When I load the testcase in Safari and try to scroll, the sample shows that we spend a lot of time in RenderLayer::calculateRects() under hitTestLayer(), and that calls RenderLayer::backgroundClipRect(). I don't think your patch is going to improve this.

The patch by no means solves all the problems. It just solves the single biggest problem I saw in while profiling. I guess you see more hit-tests on Safari due to animated scrolling, so you end up doing 60 hit-tests for updated scroll positions each second.

[Allan Sandfeld Jensen]

(In reply to comment #11)
> (In reply to comment #10)
> > Created an attachment (id=160605) [details] [details]
> > Sample
> > 
> > When I load the testcase in Safari and try to scroll, the sample shows that we spend a lot of time in RenderLayer::calculateRects() under hitTestLayer(), and that calls RenderLayer::backgroundClipRect(). I don't think your patch is going to improve this.
> 
> The patch by no means solves all the problems. It just solves the single biggest problem I saw in while profiling. I guess you see more hit-tests on Safari due to animated scrolling, so you end up doing 60 hit-tests for updated scroll positions each second.

And maybe you have overlay scrollbars, which makes useTemporaryClipRects true in hitTestLayer, and thereby shuts off the clip-rect cache, causing O(n²) behaviour in calculateClipRects?

[Radar WebKit Bug Importer]

<rdar://problem/12180284>

[Simon Fraser (smfr)]

Comment on attachment 160444 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160444&action=review

> Source/WebCore/ChangeLog:13
> +        Two minor optimization has also been added, one to make profiling of debug-builds slightly less misleading, and one
> +        to optimize calculating the outline bounds when a cached offset is provided.

You should never profile debug builds, so I don't see the point of the FractionalLayoutUnit.h changes.

[Allan Sandfeld Jensen]

(In reply to comment #14)
> (From update of attachment 160444 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160444&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        Two minor optimization has also been added, one to make profiling of debug-builds slightly less misleading, and one
> > +        to optimize calculating the outline bounds when a cached offset is provided.
> 
> You should never profile debug builds, so I don't see the point of the FractionalLayoutUnit.h changes.


The point is it removes two wrong boundary checks that unnecessarily slows down debug builds, but I can remove that part from the patch, it doesn't do anything to release builds. With that removed do you have any objections to the meat of the patch? 

And just in case I got you worried with my changelog comment; the profile data I have been referring in my comments and based the patch on, is from release builds ;)

[Allan Sandfeld Jensen]

Created attachment 161660 [details]
Patch

Minimized patch. Fixes the hottest path of scrolling on platforms that does not have overlay scrollbars. On platforms with overlay scrollbars, hit-testing still need to be improved.

[Simon Fraser (smfr)]

Comment on attachment 161660 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161660&action=review

> Source/WebCore/ChangeLog:10
> +        Only the repaint box is affected by clipping, so we can save recalculating the outline box which is the most
> +        expensive part.

Does this do the right thing for all of:
1. page scrolling
2. overflow scrolling on some ancestor of our enclosing overflow ancestor
3. scrolling on our enclosing overflow ancestor
4. various compositing configurations mixed with 1-3 above.

[Allan Sandfeld Jensen]

(In reply to comment #17)
> (From update of attachment 161660 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161660&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Only the repaint box is affected by clipping, so we can save recalculating the outline box which is the most
> > +        expensive part.
> 
> Does this do the right thing for all of:
> 1. page scrolling
> 2. overflow scrolling on some ancestor of our enclosing overflow ancestor
> 3. scrolling on our enclosing overflow ancestor
> 4. various compositing configurations mixed with 1-3 above.

I am not strong enough in the repaint logic to account for how all of those cases work step by step, but the correctness of the patch is based based the fact that computeRepaintRects is only called here in the case an ancestor clips have been observed. The clipped repaint rect is recalculated because the ancestor clips might have moved and affects the rect, but clips do not affect the outline bounds, the fact that it is recalculated just seems to be a side-effect of calling the generic function computeRepaintRects. 

So all cases without ancestor clips are unaffected, they never used this code path in the first place. Cases with ancestor clips only save recalculating outline bounds, and outline bounds should not be affected by clips. 

But ofcourse if outline bounds are affected somehow by ancestor clips, my patch would be wrong.

[Julien Chaffraix]

Comment on attachment 161660 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161660&action=review

> When I load the testcase in Safari and try to scroll, the sample shows that we spend a lot of time in RenderLayer::calculateRects() under hitTestLayer(), and that calls 
> RenderLayer::backgroundClipRect(). I don't think your patch is going to improve this.

Just my $.02, what Simon says matches my experience with optimizing a similar case with overflow: hidden RenderLayers where the bottlenecks were in painting and hit testing due to calculateRects and unneeded computations. This is what lead to the optimization in bug 85678 part of a more global optimization surrounding not walking subtree without self-painting layers (see bug 88888). It may be possible to broaden this optimization to cover your case.

A scroll performance test would be a good addition too but I don't know how feasible it is.

>>> Source/WebCore/ChangeLog:10
>>> +        expensive part.
>> 
>> Does this do the right thing for all of:
>> 1. page scrolling
>> 2. overflow scrolling on some ancestor of our enclosing overflow ancestor
>> 3. scrolling on our enclosing overflow ancestor
>> 4. various compositing configurations mixed with 1-3 above.
> 
> I am not strong enough in the repaint logic to account for how all of those cases work step by step, but the correctness of the patch is based based the fact that computeRepaintRects is only called here in the case an ancestor clips have been observed. The clipped repaint rect is recalculated because the ancestor clips might have moved and affects the rect, but clips do not affect the outline bounds, the fact that it is recalculated just seems to be a side-effect of calling the generic function computeRepaintRects. 
> 
> So all cases without ancestor clips are unaffected, they never used this code path in the first place. Cases with ancestor clips only save recalculating outline bounds, and outline bounds should not be affected by clips. 
> 
> But ofcourse if outline bounds are affected somehow by ancestor clips, my patch would be wrong.

2 and 3 have some coverages in fast/repaint/ and fast/layers/ (look for the scroll* tests). I don't know about 1 and 4. Also not sure how adequate our testing is as I have been hit pretty hard by that in the past.

> Source/WebCore/rendering/RenderLayer.cpp:523
> -        // FIXME: Is it worth passing the offsetFromRoot around like in updateLayerPositions?
> +        // FIXME: We should pass offsetFromRoot around like in updateLayerPositions.

Do you have some data that this should be done? There are some issues with passing |offsetFromRoot| across transformed layers which makes this not as easy as it looks (a lot of our code is wrong from that perspective).

[Simon Fraser (smfr)]

You should re-measure this. I'm made a lot of improvements in this area.

[Anders Carlsson]

Comment on attachment 161660 [details]
Patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.

[Dieter Komendera]

re-testing the original tes tcase in current STP 106, scrolling is smooth, but Firefox 77.0b8 is still smoother when dragging the scroll bar.  The difference is neglectable though. I think this bug can be closed.