Bug 240728 – [Repaint] Do not issue repaint when the ancestor layer has already been scheduled for one

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[zalan]

ssia

[zalan]

Created attachment 459627 [details]
Patch

[Simon Fraser (smfr)]

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.

[zalan]

Created attachment 459644 [details]
Patch

[zalan]

Created attachment 459652 [details]
Patch

[zalan]

This is 10% progression on Design (overall 1.2%)

https://perf.safari.apple.com/v3/#/analysis/task/10120

[zalan]

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

[Simon Fraser (smfr)]

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?

[zalan]

Created attachment 459730 [details]
Patch

[zalan]

Created attachment 459769 [details]
Patch

[EWS]

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

[Radar WebKit Bug Importer]

<rdar://problem/93998698>