Bug 73754

Summary: Avoid calling calculateRects in RenderLayer::paintLayer when the rectangles are not needed
Product: WebKit Reporter: Julien Chaffraix <jchaffraix>
Component: Layout and RenderingAssignee: Julien Chaffraix <jchaffraix>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, fishd, jamesr, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://dglazkov.github.com/performance-tests/biggrid.html
Bug Depends on:    
Bug Blocks: 73714    
Attachments:
Description Flags
Proposed change: limit the call to calculateRects for when we need it.
none
Proposed change 2: hopefully the |shouldPaint| logic should be clearer, also changed RenderLayerBacking.
none
Patch for landing none

Description Julien Chaffraix 2011-12-03 15:46:44 PST
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).
Comment 1 Julien Chaffraix 2011-12-03 16:05:35 PST
Created attachment 117774 [details]
Proposed change: limit the call to calculateRects for when we need it.
Comment 2 Simon Fraser (smfr) 2011-12-05 08:58:22 PST
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.
Comment 3 Julien Chaffraix 2011-12-05 13:40:30 PST
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).
Comment 4 Julien Chaffraix 2011-12-05 18:08:43 PST
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.
Comment 5 Julien Chaffraix 2011-12-05 18:17:16 PST
(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")
Comment 6 Julien Chaffraix 2011-12-05 19:51:56 PST
Created attachment 117981 [details]
Proposed change 2: hopefully the |shouldPaint| logic should be clearer, also changed RenderLayerBacking.
Comment 7 James Robinson 2011-12-05 23:31:58 PST
What's the perf impact of this patch on that testcase?
Comment 8 Simon Fraser (smfr) 2011-12-06 10:49:44 PST
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.
Comment 9 Julien Chaffraix 2011-12-06 11:03:23 PST
(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.
Comment 10 Julien Chaffraix 2011-12-06 12:34:35 PST
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.
Comment 11 Julien Chaffraix 2011-12-06 16:46:06 PST
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.
Comment 12 Julien Chaffraix 2011-12-06 16:49:59 PST
Created attachment 118140 [details]
Patch for landing
Comment 13 WebKit Review Bot 2011-12-06 20:53:27 PST
Comment on attachment 118140 [details]
Patch for landing

Clearing flags on attachment: 118140

Committed r102217: <http://trac.webkit.org/changeset/102217>
Comment 14 WebKit Review Bot 2011-12-06 20:53:32 PST
All reviewed patches have been landed.  Closing bug.