Bug 73754 – Avoid calling calculateRects in RenderLayer::paintLayer when the rectangles are not needed

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Julien Chaffraix]

RenderLayer::paintLayer is called a lot in one of our benchmarks (I will link to it once it is available online) - easily in the order of 1.5 million calls on a sizable table with td { overflow: hidden } after scrolling for several seconds.

Currently we compute all the rectangles (clip rectangles, bounds, background...) even if we don't use them. Skipping this computation can save a lot of time due to the mere number of calls (it would be better to investigate limiting those calls in the first place but this is a separate issue).

[Julien Chaffraix]

Created attachment 117774 [details]
Proposed change: limit the call to calculateRects for when we need it.

[Simon Fraser (smfr)]

Comment on attachment 117774 [details]
Proposed change: limit the call to calculateRects for when we need it.

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

> Source/WebCore/rendering/RenderLayer.cpp:2807
> +    if (isSelfPaintingLayer && !paintingOverlayScrollbars)

Can you not test m_hasVisibleContent here too?

The different tests in the different places are a bit messy. I'd prefer the logic be cleaned up.

You should also look at RenderLayerBacking::paintLayer() which shares some of the same code.

> Source/WebCore/rendering/RenderLayer.cpp:2876
> +    // FIXME: Shouldn't we check against shouldPaint here too to match the other branches?

I think you should try it and see if any pixel test start to fail.

[Julien Chaffraix]

Comment on attachment 117774 [details]
Proposed change: limit the call to calculateRects for when we need it.

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

>> Source/WebCore/rendering/RenderLayer.cpp:2807
>> +    if (isSelfPaintingLayer && !paintingOverlayScrollbars)
> 
> Can you not test m_hasVisibleContent here too?
> 
> The different tests in the different places are a bit messy. I'd prefer the logic be cleaned up.
> 
> You should also look at RenderLayerBacking::paintLayer() which shares some of the same code.

> The different tests in the different places are a bit messy. I'd prefer the logic be cleaned up.

Makes sense. I will start consolidating our checks to be more coherent.

> You should also look at RenderLayerBacking::paintLayer() which shares some of the same code.

I completely missed this one. Let me see what can be done.

>> Source/WebCore/rendering/RenderLayer.cpp:2876
>> +    // FIXME: Shouldn't we check against shouldPaint here too to match the other branches?
> 
> I think you should try it and see if any pixel test start to fail.

Good point and it does (which makes sense as our content can be outside the damage rect when our outline is not).

[Julien Chaffraix]

Spending more time looking at the existing code, it looks like there is several logics overlapping in paintLayer which is why it is confusing (overlay scrollbars vs the rest for example). I would like to split paintLayer into more specialized functions but this would add a lot of code. Thus I will postpone that to a follow-up bug and will clean the logic in place for now.

[Julien Chaffraix]

(In reply to comment #0)
> RenderLayer::paintLayer is called a lot in one of our benchmarks (I will link to it once it is available online) - easily in the order of 1.5 million calls on a sizable table with td { overflow: hidden } after scrolling for several seconds.

Thanks to Dimitri the benchmark is online: http://dglazkov.github.com/performance-tests/biggrid.html (compare scrolling with and without overflow: hidden using "Render At Once")

[Julien Chaffraix]

Created attachment 117981 [details]
Proposed change 2: hopefully the |shouldPaint| logic should be clearer, also changed RenderLayerBacking.

[James Robinson]

What's the perf impact of this patch on that testcase?

[Simon Fraser (smfr)]

Comment on attachment 117981 [details]
Proposed change 2: hopefully the |shouldPaint| logic should be clearer, also changed RenderLayerBacking.

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

> Source/WebCore/rendering/RenderLayer.cpp:2802
> +    bool paintingOverlayScrollbars = paintFlags & PaintLayerPaintingOverlayScrollbars;

Maybe shouldPaintOverlayScrollbars for consistency. Or remove the 'should' from all of these.

> Source/WebCore/rendering/RenderLayer.cpp:2805
> +    bool shouldPaintRenderer = m_hasVisibleContent && isSelfPaintingLayer && !paintingOverlayScrollbars;

I'd call this shouldPaintContent

> Source/WebCore/rendering/RenderLayer.cpp:2812
> +        // FIXME: We compute all the rectangles when paintingOverlayScrollbars but we use only |damageRect|.

This FIXME is only interesting if calculateRects does a lot of useless work in this case.

> Source/WebCore/rendering/RenderLayerBacking.cpp:1100
> +    bool shouldPaintRenderer = (m_owningLayer->hasVisibleContent() || m_owningLayer->hasVisibleDescendant()) && m_owningLayer->isSelfPaintingLayer();

shouldPaintContent or paintContent.

[Julien Chaffraix]

(In reply to comment #7)
> What's the perf impact of this patch on that testcase?

I measured through the WebInspector's timeline + Chromium FPS overlay on a 10,000 x 25 table with overflow: hidden:
* Baseline (ToT Chromium):
 - average paint time when scrolling: 135 ms
 - FPS: ~17 FPS
* Baseline + the latest patch:
 - average paint time when scrolling: 85 ms
 - FPS: ~19 FPS (a bit less jerky but still unacceptably jerky)

The WebInspector has an overhead (which I did not try to capture) thus the values should be considered rough. However the difference seems sufficient to think that there is an underlying improvement.

[Julien Chaffraix]

Comment on attachment 117981 [details]
Proposed change 2: hopefully the |shouldPaint| logic should be clearer, also changed RenderLayerBacking.

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

>> Source/WebCore/rendering/RenderLayer.cpp:2802
>> +    bool paintingOverlayScrollbars = paintFlags & PaintLayerPaintingOverlayScrollbars;
> 
> Maybe shouldPaintOverlayScrollbars for consistency. Or remove the 'should' from all of these.

I would rather keep the 'should' as it matches our style more closely. I don't feel shouldPaintOverlayScrollbars is right here as it's not really a command, more a statement: maybe isPaintingOverlayScrollbars which matches isSelfPaintingLayer?

>> Source/WebCore/rendering/RenderLayer.cpp:2805
>> +    bool shouldPaintRenderer = m_hasVisibleContent && isSelfPaintingLayer && !paintingOverlayScrollbars;
> 
> I'd call this shouldPaintContent

That was one of my 2 choices too (I thought Renderer was a bit better). Let's just renaming it!

>> Source/WebCore/rendering/RenderLayer.cpp:2812
>> +        // FIXME: We compute all the rectangles when paintingOverlayScrollbars but we use only |damageRect|.
> 
> This FIXME is only interesting if calculateRects does a lot of useless work in this case.

I don't know how much unneeded work it really does but it is not the first case where we do compute rectangles that we don't need so it may be a good idea to start marking the code paths. I don't feel strongly about this FIXME though.

[Julien Chaffraix]

Comment on attachment 117981 [details]
Proposed change 2: hopefully the |shouldPaint| logic should be clearer, also changed RenderLayerBacking.

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

>>> Source/WebCore/rendering/RenderLayer.cpp:2802
>>> +    bool paintingOverlayScrollbars = paintFlags & PaintLayerPaintingOverlayScrollbars;
>> 
>> Maybe shouldPaintOverlayScrollbars for consistency. Or remove the 'should' from all of these.
> 
> I would rather keep the 'should' as it matches our style more closely. I don't feel shouldPaintOverlayScrollbars is right here as it's not really a command, more a statement: maybe isPaintingOverlayScrollbars which matches isSelfPaintingLayer?

I decided to go with isSelfPaintingLayer for now (I will split this logic in its own function so the variable will soon disappear).

>>> Source/WebCore/rendering/RenderLayer.cpp:2812
>>> +        // FIXME: We compute all the rectangles when paintingOverlayScrollbars but we use only |damageRect|.
>> 
>> This FIXME is only interesting if calculateRects does a lot of useless work in this case.
> 
> I don't know how much unneeded work it really does but it is not the first case where we do compute rectangles that we don't need so it may be a good idea to start marking the code paths. I don't feel strongly about this FIXME though.

Removed the FIXME for now as it needs more investigation.

[Julien Chaffraix]

Created attachment 118140 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 118140 [details]
Patch for landing

Clearing flags on attachment: 118140

Committed r102217: <http://trac.webkit.org/changeset/102217>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.