Bug 86383 - Scrolling is sluggish on pages with many relative elements
Summary: Scrolling is sluggish on pages with many relative elements
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Allan Sandfeld Jensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-05-14 10:36 PDT by Lars Clausen
Modified: 2020-05-21 08:00 PDT (History)
13 users (show)

See Also:


Attachments
Test case (2.06 MB, text/html)
2012-05-14 10:36 PDT, Lars Clausen
no flags Details
Patch (5.13 KB, patch)
2012-08-24 10:07 PDT, Allan Sandfeld Jensen
simon.fraser: review-
Details | Formatted Diff | Diff
Sample (1.27 MB, text/plain)
2012-08-26 10:46 PDT, Simon Fraser (smfr)
no flags Details
Patch (2.20 KB, patch)
2012-08-31 04:14 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Lars Clausen 2012-05-14 10:36:12 PDT
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.
Comment 1 Shane Stephens 2012-06-20 21:24:14 PDT
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.
Comment 2 Lars Clausen 2012-06-21 09:27:19 PDT
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.
Comment 3 Shane Stephens 2012-07-18 21:39:19 PDT
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?
Comment 4 Lars Clausen 2012-07-18 23:06:29 PDT
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.
Comment 5 Sam Weinig 2012-07-19 22:55:17 PDT
(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.
Comment 6 Allan Sandfeld Jensen 2012-08-17 06:21:56 PDT
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.
Comment 7 Allan Sandfeld Jensen 2012-08-24 08:41:50 PDT
(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.
Comment 8 Allan Sandfeld Jensen 2012-08-24 10:07:58 PDT
Created attachment 160444 [details]
Patch

Avoid recalculating the outline bounds on scroll, this fixes the biggest thorn in slow scrolling with many layers.
Comment 9 Allan Sandfeld Jensen 2012-08-26 08:26:50 PDT
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.
Comment 10 Simon Fraser (smfr) 2012-08-26 10:46:14 PDT
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.
Comment 11 Allan Sandfeld Jensen 2012-08-27 01:49:53 PDT
(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.
Comment 12 Allan Sandfeld Jensen 2012-08-27 02:12:53 PDT
(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?
Comment 13 Radar WebKit Bug Importer 2012-08-27 10:24:15 PDT
<rdar://problem/12180284>
Comment 14 Simon Fraser (smfr) 2012-08-27 12:47:37 PDT
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.
Comment 15 Allan Sandfeld Jensen 2012-08-27 15:07:44 PDT
(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 ;)
Comment 16 Allan Sandfeld Jensen 2012-08-31 04:14:22 PDT
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.
Comment 17 Simon Fraser (smfr) 2012-08-31 09:11:13 PDT
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.
Comment 18 Allan Sandfeld Jensen 2012-09-03 02:51:30 PDT
(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.
Comment 19 Julien Chaffraix 2012-09-05 18:19:21 PDT
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).
Comment 20 Simon Fraser (smfr) 2012-11-30 11:58:03 PST
You should re-measure this. I'm made a lot of improvements in this area.
Comment 21 Anders Carlsson 2014-02-05 11:14:40 PST
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.
Comment 22 Dieter Komendera 2020-05-21 08:00:16 PDT
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.