Bug 229802 – [Performance] Self painting layers spend too much time in RenderLayer::collectFragments

• Status: NEW
• Component: Layout and Rendering

[cathiechen]

RenderLayer::collectFragments costs around 20% time in RenderLayer::collectFragments for this test.

[cathiechen]

Created attachment 437140 [details]
screen-shot-collectFragments-cost

[cathiechen]

Created attachment 437142 [details]
test-change-first-child-height.html

[cathiechen]

Created attachment 437162 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 437162 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:4591
> +    if (!paintDirtyRect.intersects(m_repaintRects.outlineBoundsRect))

This needs to check m_repaintRectsValid

[cathiechen]

Comment on attachment 437162 [details]
Patch

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

>> Source/WebCore/rendering/RenderLayer.cpp:4591
>> +    if (!paintDirtyRect.intersects(m_repaintRects.outlineBoundsRect))
> 
> This needs to check m_repaintRectsValid

Done, thanks!

[cathiechen]

Created attachment 437376 [details]
Patch

[cathiechen]

Created attachment 437606 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/82921321>

[cathiechen]

Created attachment 437754 [details]
Patch

[cathiechen]

Created attachment 437822 [details]
Patch

[Rob Buis]

Comment on attachment 437822 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:3553
> +        return true;

With a transform in place there could still be no intersection I think? If so it is probably best to keep this check out of hasIntersections. Or rename to something like hasSimpleIntersections.

[cathiechen]

Comment on attachment 437822 [details]
Patch

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

>> Source/WebCore/rendering/RenderLayer.cpp:3553
>> +        return true;
> 
> With a transform in place there could still be no intersection I think? If so it is probably best to keep this check out of hasIntersections. Or rename to something like hasSimpleIntersections.

Thanks, looks like this can be removed.

[cathiechen]

Created attachment 437952 [details]
Patch

[cathiechen]

Created attachment 437954 [details]
screenshot-of-optimized-collectFragments

[cathiechen]

Created attachment 437955 [details]
screenshot-of-original-collectFragments

[cathiechen]

Hi,

I think the patch is ready for review.

According to the tests on my local Mac.
The function RenderLayer::collectFragments costs about 20% less (from 27% to 6.2%) while loading the test page.

A performance test has been added, which is based on requestAnimationFrame. The result is about 10% better.

Please take a look. Thanks:)

[cathiechen]

Created attachment 438231 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 438231 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:3552
> +    if (auto* box = renderBox(); box && box->hasRenderOverflow()) {

If a box has overflow, why can't we just check against m_repaintRects.clippedOverflowRect?

[cathiechen]

Comment on attachment 438231 [details]
Patch

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

>> Source/WebCore/rendering/RenderLayer.cpp:3552
>> +    if (auto* box = renderBox(); box && box->hasRenderOverflow()) {
> 
> If a box has overflow, why can't we just check against m_repaintRects.clippedOverflowRect?

Yes, we can remove this. I was trying to collect all the rects, including the descendants' which is not needed.

And it seems this approach will change the return value of collectFragments when there is backgroundClipRect.
I will try to upload a new patch that should not change the return value.

[cathiechen]

Created attachment 439162 [details]
Patch

[cathiechen]

The time is reduced from 27% to 15.2% with the new optimization.

[cathiechen]

Created attachment 439328 [details]
screenshot-of-optimized-collectFragments

[cathiechen]

Created attachment 439330 [details]
Patch

[cathiechen]

Comment on attachment 439330 [details]
Patch

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

[cathiechen]

The intersection check and the parentClipRects part are not very relevant, so I've split the parentClipRects changes to a new bug. (230885)

[cathiechen]

Created attachment 439451 [details]
Patch-for-review

[cathiechen]

Created attachment 440179 [details]
Patch

[Rob Buis]

Comment on attachment 439451 [details]
Patch-for-review

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

> Source/WebCore/rendering/RenderLayer.cpp:4625
> +                foregroundRect = LayoutRect();

It seems at this point we can just return? Also is it possible to add a single block to handle the !intersectsBackgroundRect, rather than in three areas?

[cathiechen]

Comment on attachment 439451 [details]
Patch-for-review

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

>> Source/WebCore/rendering/RenderLayer.cpp:4625
>> +                foregroundRect = LayoutRect();
> 
> It seems at this point we can just return? Also is it possible to add a single block to handle the !intersectsBackgroundRect, rather than in three areas?

Yeah, indeed, we can return here.
BTW, we moved this fix to bug: 231169