Bug 240728

Created attachment 459627 [details] Patch

Comment on attachment 459627 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=459627&action=review > Source/WebCore/rendering/RenderElement.cpp:448 > + if (hasLayer()) > + downcast<RenderLayerModelObject>(*this).layer()->setRepaintStatus(NeedsFullRepaint); Just call setLayerNeedsFullRepaint()? Why not do this in the RendererAndDescendantsRenderersWithLayers clause above too? Does this cause more extensive repainting than we had before (e.g. when background color changes and the layer has sticking-out children)? > Source/WebCore/rendering/RenderLayer.cpp:2008 > +RenderLayer* RenderLayer::enclosingCompositingLayerForRepaint(IncludeSelfOrNot includeSelf, bool* enclosingLayerNeedsFullRepaint) const Probably nicer to return a std::pair<> or a struct. > Source/WebCore/rendering/RenderLayer.cpp:2028 > + if (enclosingLayerNeedsFullRepaint) > + *enclosingLayerNeedsFullRepaint = *enclosingLayerNeedsFullRepaint || curr->needsFullRepaint(); You don't cut off the ancestor walk here if enclosingLayerNeedsFullRepaint becomes true, but I think you could if you know the caller is just going to bail on this repaint. > Source/WebCore/rendering/RenderObject.cpp:986 > + if (!repaintContainer.renderer) > + repaintContainer = { fullRepaintIsScheduled(*this), &view }; You're adding a second ancestor walk here, but can't you still use repaintContainer.fullRepaintIsScheduled? It should have the correct answer for all layers traversed, possibly other than for the RenderView's layer.

Created attachment 459644 [details] Patch

Created attachment 459652 [details] Patch

This is 10% progression on Design (overall 1.2%) https://perf.safari.apple.com/v3/#/analysis/task/10120

(In reply to Simon Fraser (smfr) from comment #2) > Comment on attachment 459627 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=459627&action=review > > > Source/WebCore/rendering/RenderElement.cpp:448 > > + if (hasLayer()) > > + downcast<RenderLayerModelObject>(*this).layer()->setRepaintStatus(NeedsFullRepaint); > > Just call setLayerNeedsFullRepaint()? > > Why not do this in the RendererAndDescendantsRenderersWithLayers clause > above too? > > Does this cause more extensive repainting than we had before (e.g. when > background color changes and the layer has sticking-out children)? > Removed. > > Source/WebCore/rendering/RenderLayer.cpp:2008 > > +RenderLayer* RenderLayer::enclosingCompositingLayerForRepaint(IncludeSelfOrNot includeSelf, bool* enclosingLayerNeedsFullRepaint) const > > Probably nicer to return a std::pair<> or a struct. Done. > > > Source/WebCore/rendering/RenderLayer.cpp:2028 > > + if (enclosingLayerNeedsFullRepaint) > > + *enclosingLayerNeedsFullRepaint = *enclosingLayerNeedsFullRepaint || curr->needsFullRepaint(); > > You don't cut off the ancestor walk here if enclosingLayerNeedsFullRepaint > becomes true, but I think you could if you know the caller is just going to > bail on this repaint. I don't mind adding a dedicated function which bails out early when enclosingLayerNeedsFullRepaint becomes true, but currently some of the clients of this function need the actual enclosing layer regardless of the repaint status. > > Source/WebCore/rendering/RenderObject.cpp:986 > > + if (!repaintContainer.renderer) > > + repaintContainer = { fullRepaintIsScheduled(*this), &view }; > > You're adding a second ancestor walk here, but can't you still use > repaintContainer.fullRepaintIsScheduled? It should have the correct answer > for all layers traversed, possibly other than for the RenderView's layer. This is (mostly) for the non-composited case when we don't even start walking the layer tree (see view().usesCompositing() in RenderObject::containerForRepaint).

Comment on attachment 459652 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=459652&action=review > Source/WebCore/rendering/RenderLayer.cpp:2050 > + return { false, nullptr }; Could just return { } > Source/WebCore/rendering/RenderLayer.h:594 > + bool fullRepaintIsScheduled { false }; Maybe fullRepaintAlreadyScheduled or dueForFullRepaint?

Created attachment 459730 [details] Patch

Created attachment 459769 [details] Patch

Committed r294902 (251025@main): <https://commits.webkit.org/251025@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 459769 [details].

<rdar://problem/93998698>