[webkit-reviews] review requested: [Bug 94743] Automatically use composited scrolling : [Attachment 177564] Patch

[bugzilla-daemon at webkit.org]

vollick at chromium.org has asked	for review:
Bug 94743: Automatically use composited scrolling
https://bugs.webkit.org/show_bug.cgi?id=94743

Attachment 177564: Patch
https://bugs.webkit.org/attachment.cgi?id=177564&action=review

------- Additional Comments from vollick at chromium.org
Some notes about performance. I profiled the behavior of two heavy pages, with
and without the flag to enable opting in. I should also mention my profiling
methodology. We have scrolling benchmark that loads various pages and simulates
scrolls, during which it gathers performance data. Here's what I saw:

With opting in turned on:
  www.gmail.com:
    Avg # layers = 47.4
    Amount of time spent in RenderLayerCompositor::updateCompositingLayers =
1576.1 ms (13.8%)
  www.theverge.com: (It's worth noting that this page did *not* opt into
composited scrolling)
    Avg # layers = 3.0
    Amount of time spent in RenderLayerCompositor::updateCompositingLayers =
272.6 ms

With opting in turned off:
  www.gmail.com:
    Avg # layers = 2.0
    Amount of time spent in RenderLayerCompositor::updateCompositingLayers =
196.8 ms
  www.theverge.com:
    Avg # layers = 3.0
    Amount of time spent in RenderLayerCompositor::updateCompositingLayers =
243.7 ms

Interestingly, if I comment out the setShouldReevaluateCompositingAfterLayout
calls in dirtyZOrderLists and dirtyNormalFlowList, I measured that we spent
1598.436ms. Also, the time spent in
RenderLayer::updateCanSafelyEstablishStackingContext is only 0.83ms! Moreover,
if I measure the cost of updateCompositingLayers _before_ the scroll, it's
about 144ms. This implies that the cost is almost entirely due to overlap
testing during scroll. I certainly want to look at that problem, but it seems
clear that it was not caused by this patch, and I don't think it makes sense to
squeeze in an overlap testing optimization here, either.


(In reply to comment #53)
> (From update of attachment 176731 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=176731&action=review
>
> Thanks Ian, I think this looks great. It would be interesting to see what
percentage of overflow scrolling elements this applies to. We could add some
metrics for that later.
For sure, we're working on that already ;)
>
> > Source/WebCore/rendering/RenderLayer.cpp:1793
> > +	 bool forceUseCompositedScrolling =
acceleratedCompositingForOverflowScrollEnabled()  &&
canSafelyEstablishAStackingContext();
>
> Nit: extra space before "&&".
Done.
>
> > Source/WebCore/testing/InternalSettings.idl:-72
> > -
>
> Was this an intentional change?
Nope, removed.