Optimize performance of RenderLayer::clipCrossesPaintingBoundary
Created attachment 439446 [details] Patch
Tested the case in bug: 229802 locally, this patch will reduce the time cost of RenderLayer::parentClipRects from 8.3% to 2.5%.
Created attachment 439448 [details] screenshot-before-opt
Created attachment 439449 [details] screenshot-after-opt
Comment on attachment 439446 [details] Patch Hi, I think this patch is ready for review. Please take a look, thanks!
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.
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!
Created attachment 439720 [details] Patch-for-review
Created attachment 439741 [details] screenshot-of-original-clipCrossesPaintingBoundary
Created attachment 439742 [details] screenshot-of-optimized-clipCrossesPaintingBoundary
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%
Created attachment 439801 [details] Patch
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].
<rdar://problem/83747512>
Re-opened since this is blocked by bug 234273
Created attachment 447587 [details] Patch
Created attachment 447588 [details] Patch
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.
Created attachment 447597 [details] not-reproducible-with-simulate-wheel-event-expected.html Add a fake expected.html
Created attachment 447598 [details] Patch
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.
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.
(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!
Created attachment 447690 [details] Patch
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?
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.
Created attachment 447932 [details] Patch
(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.
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
Created attachment 447934 [details] Patch
Created attachment 447935 [details] Patch
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.
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.
Created attachment 448726 [details] Patch
Created attachment 449022 [details] Patch