Bug 92011

Summary: 50% fixed position coverage slow scroll heuristic is incorrect when invalidations aren't clipped
Product: WebKit Reporter: Adrienne Walker <enne>
Component: WebCore Misc.Assignee: Adrienne Walker <enne>
Status: RESOLVED FIXED    
Severity: Normal CC: anilsson, benjamin, dglazkov, enne, noam, simon.fraser, thorton, tonikitoo, vollick, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Remove fixed object heuristic
none
Archive of layout-test-results from gce-cr-linux-02 none

Description Adrienne Walker 2012-07-23 10:57:54 PDT
The heuristic in FrameView::scrollContentsFastPath that tries to estimate the coverage of fixed position elements on the screen is incorrect if !FrameView::clipsRepaints().  This is because you could have a very large off-screen fixed position element that covers more than the viewport size.

Some thoughts:
(1) Maybe fixed position elements should be explicitly clipped to the viewport or to some ancestor clipping layer of their container.

(2) For Chromium, this heuristic is not useful at all when the view's layer is composited.  We don't shift scroll composited layers, so the slow scroll "invalidate everything" path is always going to be slower.  Maybe we could just skip this heuristic for composited layers?
Comment 1 Adrienne Walker 2012-07-23 15:00:00 PDT
Created attachment 153871 [details]
Patch
Comment 2 Adrienne Walker 2012-07-23 15:04:26 PDT
(In reply to comment #1)
> Created an attachment (id=153871) [details]
> Patch

smfr, thorton, noamr: Putting this patch up for discussion.  I'm not sure how the fast/slow scrolling works for other ports, but the FrameView slow scroll path doesn't make any sense for Chromium as there's no shift scrolling, so the extra invalidations only make things worse.
Comment 3 Noam Rosenthal 2012-07-23 15:07:43 PDT
(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=153871) [details] [details]
> > Patch
> 
> smfr, thorton, noamr: Putting this patch up for discussion.  I'm not sure how the fast/slow scrolling works for other ports, but the FrameView slow scroll path doesn't make any sense for Chromium as there's no shift scrolling, so the extra invalidations only make things worse.

Pretty similar for the Qt port, at least for WebKit2. Scrolling is done in the UI process and we're forcing fixed position layers to be composited. The only time we need slow scrolling is for non-layer fixed objects like backgrounds, in which case this patch should work fine.
Comment 4 Simon Fraser (smfr) 2012-07-23 15:28:19 PDT
Comment on attachment 153871 [details]
Patch

I think our similar-but-different scrolling models are conflicting here.

On Mac Mountain Lion we use a tile cache behind the root's layer, so contentsInCompositedLayer() is true. However, we don't have support for compositing fixed elements, so we have to repaint fixed things on scrolling. Once we put fixed things into composited layers, they'll be excluded from the regionToUpdate. But the ! isCompositedContentLayer check doesn't appropriately reflect the "content is painted into a layer which moves when you scroll and doesn't need to repaint". That seems like a behavior that the ScrollingCoordinator should export, like frameRepaintsOnScroll(FrameView*) or something.
Comment 5 Adrienne Walker 2012-07-23 16:12:37 PDT
(In reply to comment #4)
> (From update of attachment 153871 [details])
> I think our similar-but-different scrolling models are conflicting here.
> 
> On Mac Mountain Lion we use a tile cache behind the root's layer, so contentsInCompositedLayer() is true. However, we don't have support for compositing fixed elements, so we have to repaint fixed things on scrolling. Once we put fixed things into composited layers, they'll be excluded from the regionToUpdate. But the ! isCompositedContentLayer check doesn't appropriately reflect the "content is painted into a layer which moves when you scroll and doesn't need to repaint". That seems like a behavior that the ScrollingCoordinator should export, like frameRepaintsOnScroll(FrameView*) or something.

Maybe I'm misunderstanding something here.

This patch changes the logic so that the scrollContentsFastPath is always used (and not aborted by this heuristic) when the contents are in a composited layer.  scrollContentsFastPath always does invalidations for all fixed position objects that are not composited.  The difference is that scrollContentsSlowPath just invalidates the entire frame view contents rather than each fixed position object individually.  In both cases, repainting for fixed position object occurs.

It's just that the slowContentsSlowPath does more invalidations than are necessary if this heuristic is hit.  It doesn't give any other code a chance to do something more smart than doing all that extra painting.
Comment 6 Simon Fraser (smfr) 2012-07-23 16:22:17 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 153871 [details] [details])
> > I think our similar-but-different scrolling models are conflicting here.
> > 
> > On Mac Mountain Lion we use a tile cache behind the root's layer, so contentsInCompositedLayer() is true. However, we don't have support for compositing fixed elements, so we have to repaint fixed things on scrolling. Once we put fixed things into composited layers, they'll be excluded from the regionToUpdate. But the ! isCompositedContentLayer check doesn't appropriately reflect the "content is painted into a layer which moves when you scroll and doesn't need to repaint". That seems like a behavior that the ScrollingCoordinator should export, like frameRepaintsOnScroll(FrameView*) or something.
> 
> Maybe I'm misunderstanding something here.
> 
> This patch changes the logic so that the scrollContentsFastPath is always used (and not aborted by this heuristic) when the contents are in a composited layer.  

I'm not sure why being in a composited layer affects the behavior at all.
Comment 7 Adrienne Walker 2012-07-23 16:55:31 PDT
(In reply to comment #6)

> I'm not sure why being in a composited layer affects the behavior at all.

Now that I think about it, I think Chromium's non-composited paint aggregator should automatically handle "too many" invalidations already.  So, in both cases it'd be better for us to skip the heuristic and defer to our own invalidation handling code to make the choice about what to do.

So then the questions are:

(1) Is this heuristic useful for other ports for both composited and non-composited mode?

(2) If it is useful, then is ScrollingCoordinator the best place to put this switch? I'm not quite sure what this function should be called, though.
Comment 8 Adrienne Walker 2012-07-24 16:51:49 PDT
Created attachment 154175 [details]
Remove fixed object heuristic
Comment 9 WebKit Review Bot 2012-07-24 17:35:00 PDT
Comment on attachment 154175 [details]
Remove fixed object heuristic

Attachment 154175 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13347035

New failing tests:
fast/loader/loadInProgress.html
fast/inline/positionedLifetime.html
fast/loader/unload-form-post-about-blank.html
Comment 10 WebKit Review Bot 2012-07-24 17:35:05 PDT
Created attachment 154192 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 11 Benjamin Poulain 2012-07-24 17:36:05 PDT
I may be missing something, but if you composite all the fixed elements and the elements above, why do you need to go in that path at all?

Shouldn't you take a shortcut and just update the newly exposed area?
Comment 12 Adrienne Walker 2012-07-24 17:39:51 PDT
(In reply to comment #11)
> I may be missing something, but if you composite all the fixed elements and the elements above, why do you need to go in that path at all?
> 
> Shouldn't you take a shortcut and just update the newly exposed area?

Chromium doesn't automatically composite all fixed position elements (yet), so regionToUpdate still includes them.
Comment 13 Adrienne Walker 2012-07-26 12:43:21 PDT
smfr, thorton: ping, re: questions in comment 7.
Comment 14 Adrienne Walker 2012-07-31 11:55:12 PDT
(In reply to comment #13)
> smfr, thorton: ping, re: questions in comment 7.

Sorry to re-ping on this bug, but it's been a week since I posted a three line patch and nobody but noamr has been forthcoming about whether or why this heuristic is useful for their port.

Is there somebody else I should CC who works on Safari?
Comment 15 WebKit Review Bot 2012-08-07 14:33:47 PDT
Comment on attachment 154175 [details]
Remove fixed object heuristic

Clearing flags on attachment: 154175

Committed r124919: <http://trac.webkit.org/changeset/124919>
Comment 16 WebKit Review Bot 2012-08-07 14:33:53 PDT
All reviewed patches have been landed.  Closing bug.