Bug 73754

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

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 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).

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.

(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")

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

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

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.

(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 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 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.

Created attachment 118140 [details] Patch for landing

Comment on attachment 118140 [details] Patch for landing Clearing flags on attachment: 118140 Committed r102217: <http://trac.webkit.org/changeset/102217>

All reviewed patches have been landed. Closing bug.