Bug 230885 – [Performance] Optimize RenderLayer::clipCrossesPaintingBoundary

• Component: Layout and Rendering
• Status: REOPENED

[cathiechen]

Optimize performance of RenderLayer::clipCrossesPaintingBoundary

[cathiechen]

Created attachment 439446 [details]
Patch

[cathiechen]

Tested the case in bug: 229802 locally, this patch will reduce the time cost of RenderLayer::parentClipRects from 8.3% to 2.5%.

[cathiechen]

Created attachment 439448 [details]
screenshot-before-opt

[cathiechen]

Created attachment 439449 [details]
screenshot-after-opt

[cathiechen]

Comment on attachment 439446 [details]
Patch

Hi,
I think this patch is ready for review. Please take a look, thanks!

[Simon Fraser (smfr)]

Comment on attachment 439446 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:4455
> +    if (parentLayer->enclosingPaginationLayer(IncludeCompositedPaginatedLayers) != enclosingPaginationLayer(IncludeCompositedPaginatedLayers))
> +        return true;
> +
> +    if (compositedWithOwnBackingStore(*this))
> +        return true;
> +
> +    if (paintsIntoProvidedBacking() && backingProviderLayer())
> +        return true;
> +
> +    const RenderLayer* paintParent = paintOrderParent();
> +    if (parentLayer == paintParent)
> +        return false;
> +
> +    return paintParent != parentLayer->paintOrderParent();

It's unfortunate that this effectively copies code from enclosingCompositingLayerForRepaint(), and puts in a separate file, so fixes to enclosingCompositingLayerForRepaint() are unlikely to be replicated here.

Think it would be better to add `bool RenderLayer::sharesCompositingLayerForRepaint(RenderLayer& otherLayer)` and put it next to RenderLayer::enclosingCompositingLayerForRepaint() so the code is close.

Pagination is really rare, so move the pagination check last also.

[cathiechen]

Comment on attachment 439446 [details]
Patch

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

>> Source/WebCore/rendering/RenderLayer.cpp:4455
>> +    return paintParent != parentLayer->paintOrderParent();
> 
> It's unfortunate that this effectively copies code from enclosingCompositingLayerForRepaint(), and puts in a separate file, so fixes to enclosingCompositingLayerForRepaint() are unlikely to be replicated here.
> 
> Think it would be better to add `bool RenderLayer::sharesCompositingLayerForRepaint(RenderLayer& otherLayer)` and put it next to RenderLayer::enclosingCompositingLayerForRepaint() so the code is close.
> 
> Pagination is really rare, so move the pagination check last also.

Done, thanks!

[cathiechen]

Created attachment 439720 [details]
Patch-for-review

[cathiechen]

Created attachment 439741 [details]
screenshot-of-original-clipCrossesPaintingBoundary

[cathiechen]

Created attachment 439742 [details]
screenshot-of-optimized-clipCrossesPaintingBoundary

[cathiechen]

Here are my locally test results. The clipCrossesPaintingBoundary percentage:
* Original: 7.1%, 7.8%, 7.0%
* With patch in bug 231016: 6.0%, 5.9%, 5.7%
* With this patch + patch in bug 231016: 1.6%, 1.4%, 1.2%

[cathiechen]

Created attachment 439801 [details]
Patch

[EWS]

Committed r283354 (242363@main): <https://commits.webkit.org/242363@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 439801 [details].

[Radar WebKit Bug Importer]

<rdar://problem/83747512>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 234273

[cathiechen]

Created attachment 447587 [details]
Patch

[cathiechen]

Created attachment 447588 [details]
Patch

[cathiechen]

Created attachment 447596 [details]
not-reproducible-with-simulate-wheel-event.html

Add a test with wheel event simulation, but the issue is not reproducible in run-webkit-tests. It is reproducible by manually scrolling the content.

[cathiechen]

Created attachment 447597 [details]
not-reproducible-with-simulate-wheel-event-expected.html

Add a fake expected.html

[cathiechen]

Created attachment 447598 [details]
Patch

[cathiechen]

Comment on attachment 447598 [details]
Patch

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

Hi,
This patch also addressed the regression in bug 234269. Let me explain more in the code.

> Source/WebCore/rendering/RenderLayer.cpp:1966
> +    if (auto* repaintTarget = repaintTargetForLayer(*this); repaintTarget != repaintTargetForLayer(otherLayer))

This change addressed the regression.
In the case, the layers "second" and "third" share backing layer, they are not clipCrossesPaintingBoundary, so as the "four" layer, which can get the right clip rects from its parent then.

[Tim Nguyen (:ntim)]

Comment on attachment 447598 [details]
Patch

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

>> Source/WebCore/rendering/RenderLayer.cpp:1966
>> +    if (auto* repaintTarget = repaintTargetForLayer(*this); repaintTarget != repaintTargetForLayer(otherLayer))
> 
> This change addressed the regression.
> In the case, the layers "second" and "third" share backing layer, they are not clipCrossesPaintingBoundary, so as the "four" layer, which can get the right clip rects from its parent then.

Could you add the reftest for the regression?

Also, could you please unskip https://webkit-search.igalia.com/webkit/rev/600f4eb35ff0057349a063904f4a89bd0e15abdb/LayoutTests/TestExpectations#5061? Since the revert of the previous version of this patch caused the issue.

[cathiechen]

(In reply to Tim Nguyen (:ntim) from comment #22)

Hi Tim, thanks for the review!

> Could you add the reftest for the regression?
> 
Yeah, I tried to add one. But it seems the regression can not be captured by run-webkit-tests. See the attachment: not-reproducible-with-simulate-wheel-event.html 

> Also, could you please unskip
> https://webkit-search.igalia.com/webkit/rev/
> 600f4eb35ff0057349a063904f4a89bd0e15abdb/LayoutTests/TestExpectations#5061?
> Since the revert of the previous version of this patch caused the issue.

Will do, thanks!

[cathiechen]

Created attachment 447690 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 447690 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:1938
> +static RenderLayer* repaintTargetForLayer(const RenderLayer& layer)

This function has a confusing name now that you've pulled it out from the lambda.

You might expect it to return the same thing as enclosingCompositingLayerForRepaint() but all it does is check itself and its backing provider.

> Source/WebCore/rendering/RenderLayer.cpp:1973
> +    const RenderLayer* paintParent = paintOrderParent();
> +    if (&otherLayer == paintParent)
> +        return true;
> +    auto* otherPaintParent = otherLayer.paintOrderParent();
> +    return paintParent == otherPaintParent || this == otherPaintParent;

This code isn't doing any compositing checks. "Paint order" here is really just about z-order, so it's not clear to me that this is correct. For example, what if otherLayer's paintsIntoProvidedBacking() is true?

[cathiechen]

Comment on attachment 447690 [details]
Patch

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

Hi Simon,
Thanks for the review!

>> Source/WebCore/rendering/RenderLayer.cpp:1938
>> +static RenderLayer* repaintTargetForLayer(const RenderLayer& layer)
> 
> This function has a confusing name now that you've pulled it out from the lambda.
> 
> You might expect it to return the same thing as enclosingCompositingLayerForRepaint() but all it does is check itself and its backing provider.

Yeah, agree, I will not reuse this function in the new patch.

>> Source/WebCore/rendering/RenderLayer.cpp:1973
>> +    return paintParent == otherPaintParent || this == otherPaintParent;
> 
> This code isn't doing any compositing checks. "Paint order" here is really just about z-order, so it's not clear to me that this is correct. For example, what if otherLayer's paintsIntoProvidedBacking() is true?

Yeah, the approach is not right here, when otherLayer's paintsIntoProvidedBacking() is true. Will fix this is the new patch.

[cathiechen]

Created attachment 447932 [details]
Patch

[cathiechen]

(In reply to cathiechen from comment #23)
> (In reply to Tim Nguyen (:ntim) from comment #22)
> 
> Hi Tim, thanks for the review!
> 
> > Could you add the reftest for the regression?
> > 
> Yeah, I tried to add one. But it seems the regression can not be captured by
> run-webkit-tests. See the attachment:
> not-reproducible-with-simulate-wheel-event.html 
> 
> > Also, could you please unskip
> > https://webkit-search.igalia.com/webkit/rev/
> > 600f4eb35ff0057349a063904f4a89bd0e15abdb/LayoutTests/TestExpectations#5061?
> > Since the revert of the previous version of this patch caused the issue.
> 
> Will do, thanks!

Hi Tim! Looks like the crash bug was hidden again by the regression of the previous patch. I'll try to explain it in bug 234315.

[cathiechen]

Hi, I just added a perf test to the patch. It reveals the improvement of this patch to the pages with lots of deeply nested self-painting layers.

The result is improved from 916ms to 714ms.

More detailed data:


The result with this patch:
:Time -> [553, 539.0000000000005, 540, 714, 736, 716, 712.0000000000009, 718, 716, 716, 714, 715, 712, 711.0000000000018, 715, 711.9999999999982, 713, 719.9999999999982, 719.0000000000018, 715.9999999999982] ms
    mean: 690.3499999999998 ms
    median: 714.5 ms
    stdev: 63.343902626851104 ms
    min: 539.0000000000005 ms
    max: 736 ms

The result without this patch:
:Time -> [750, 996, 924, 914, 918, 946, 916, 918, 922, 924, 913, 948, 911, 909, 915, 908, 918, 917, 915, 915] ms
    mean: 914.85 ms
    median: 916.5 ms
    stdev: 43.64000458295116 ms
    min: 750 ms
    max: 996 ms

[cathiechen]

Created attachment 447934 [details]
Patch

[cathiechen]

Created attachment 447935 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 447935 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:1971
> +    if (compositedWithOwnBackingStore(*layer))
> +        return false;
> +
> +    if (!layer->paintsIntoProvidedBacking()) {

The way this code uses `layer` makes it look like it's going to loop, but it doesn't. It would be clearer to use `this` in place of `layer`.

> Source/WebCore/rendering/RenderLayer.cpp:1975
> +        if (layer == parentLayer)

How is this ever possible?

> Source/WebCore/rendering/RenderLayer.cpp:1979
> +        if (compositedWithOwnBackingStore(*parentLayer))
> +            return false;

But this layer can paint into parentLayer's backing, right? This will be pretty common.

[cathiechen]

Comment on attachment 447935 [details]
Patch

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

Hi Simon,
Sorry for the confusion. I explained a little, please take a look, thanks!

>> Source/WebCore/rendering/RenderLayer.cpp:1971
>> +    if (!layer->paintsIntoProvidedBacking()) {
> 
> The way this code uses `layer` makes it look like it's going to loop, but it doesn't. It would be clearer to use `this` in place of `layer`.

`layer` could be `this` or `paintOrderParent()`. Inside this constrain, `layer` is set to `paintOrderParent()`.

> Source/WebCore/rendering/RenderLayer.cpp:1972
> +        layer = paintOrderParent();

Maybe it becomes a little clear, if it uses `layer = layer->paintOrderParent();` here?

>> Source/WebCore/rendering/RenderLayer.cpp:1975
>> +        if (layer == parentLayer)
> 
> How is this ever possible?

`layer` is `paintOrderParent()` now, so it could be `parentLayer`.

>> Source/WebCore/rendering/RenderLayer.cpp:1979
>> +            return false;
> 
> But this layer can paint into parentLayer's backing, right? This will be pretty common.

Here, `this->paintOrderParent()` is not equal to `parentLayer`, so this layer won't paint into parentLayer's backing.

[cathiechen]

Created attachment 448726 [details]
Patch

[cathiechen]

Created attachment 449022 [details]
Patch