Bug 82511 – Avoid painting non-foreground phases in RenderSVGRoot

• Component: SVG
• Status: RESOLVED INVALID

[Florin Malita]

As suggested in https://bugs.webkit.org/show_bug.cgi?id=82059 :

> (From update of attachment 134135 [details])
> The only renderer that used to care about phases like HitTestFloat or HotTestChildBlockBackgrounds is RenderSVGFo. Now that this dependency is gone, we can completely avoid painting() the whole SVG subtree in non-foreground phases (still exceptions are needed for PaintPhaseSelection I think) - that would save a lot of time!

[Florin Malita]

Looks like we also need to paint during PaintPhaseOutline & PaintPhaseSelfOutline - otherwise svg/custom/focus-ring.svg is failing.

[Florin Malita]

Created attachment 135328 [details]
Patch

[Nikolas Zimmermann]

(In reply to comment #1)
> Looks like we also need to paint during PaintPhaseOutline & PaintPhaseSelfOutline - otherwise svg/custom/focus-ring.svg is failing.
That's sad and true :-)

It brings up an old idea: tracking the PaintPhases that children actually care about in their containing blocks / containers. This can be used to speed up HTML as well.
Unfortunately my work-in-progress patch for that is lost, I can't find it anymore.

Currently most renderers paint() methods work like this, eg.:
if (paintPhase == outline && outlineWidth > 0) paintOutline()..
I thought about introducing "shouldPaintPhase(PaintPhase)" to each renderer, which returns a OR'ed combination of the paint phases the renderer cares about. During layout, all children of a container, then register their phases in the container. Next time the container paints, it knows which phases need to be propagated to the children, and which not.

Do you think its worth to try that? Or would you still like to see this patch go in? If yes, I'd propose to add a new performance test for that, with a large SVG document, consisting of lots of containers (deep nested <g>'s etc.). I expect this patch to be a speed-up on documents like this.

[Florin Malita]

(In reply to comment #3)
> I thought about introducing "shouldPaintPhase(PaintPhase)" to each renderer, which returns a OR'ed combination of the paint phases the renderer cares about. During layout, all children of a container, then register their phases in the container. Next time the container paints, it knows which phases need to be propagated to the children, and which not.

That sounds like a great idea!

Are you thinking about maintaining per-child masks or just an aggregate (ORed) for all the children? The first approach may have a noticeable memory impact (and some added complexity), but the latter misses some optimization opportunities. It's probably worth exploring both and getting some perf numbers.


> Do you think its worth to try that? Or would you still like to see this patch go in? If yes, I'd propose to add a new performance test for that, with a large SVG document, consisting of lots of containers (deep nested <g>'s etc.). I expect this patch to be a speed-up on documents like this.

I'd prefer getting this small change in first (even if temporarily), then start working on the more general patch.

I'll look at putting together a perf test to verify the improvement. From a quick scan of existing stuff in PerformanceTests it's a bit unclear how to benchmark the paint() time (most tests seem focused on layout and DOM manipulation), but I'll dig deeper.

[Florin Malita]

Try as I might have, I couldn't isolate any performance variance with this patch.

Some debugging revealed that most paint phases are already filtered out in RenderReplaced::shouldPaint():

    if (paintInfo.phase != PaintPhaseForeground && paintInfo.phase != PaintPhaseOutline && paintInfo.phase != PaintPhaseSelfOutline 
            && paintInfo.phase != PaintPhaseSelection && paintInfo.phase != PaintPhaseMask)
        return false;

So, unfortunately, this patch is not really buying us anything.

[Nikolas Zimmermann]

(In reply to comment #5)
> Try as I might have, I couldn't isolate any performance variance with this patch.
> 
> Some debugging revealed that most paint phases are already filtered out in RenderReplaced::shouldPaint():
> 
>     if (paintInfo.phase != PaintPhaseForeground && paintInfo.phase != PaintPhaseOutline && paintInfo.phase != PaintPhaseSelfOutline 
>             && paintInfo.phase != PaintPhaseSelection && paintInfo.phase != PaintPhaseMask)
>         return false;
> 
> So, unfortunately, this patch is not really buying us anything.

Hah, I completely forgot about that, when we switched RenderSVGRoot to inherit from RenderReplaced. So we actually don't suffer from paint() overhead anymore, right? Can we close this bug?

[Florin Malita]

(In reply to comment #6)
> we actually don't suffer from paint() overhead anymore, right?

Right, RenderSVGRoot only paints during the phases it needs to. Your more general optimization idea still makes sense though.