Bug 36564 – Performance regression for setting content of <text> in SVG

• Component: SVG
• Status: RESOLVED FIXED

[Beth Dakin]

Created attachment 51562 [details]
Test case

In the attached test case,  a JavaScript function will sets the content of a <text> node 50 times and popup an alert() box to report the elapsed time after clicking on the link "Sets content of the red text 50 times" in the upper-left hand corner. Before http://trac.webkit.org/changeset/52647 this test was faster. With TOT, it is about 1.6x slower.

<rdar://problem/7693963>

[Dirk Schulze]

I bet this is caused by http://trac.webkit.org/browser/trunk/WebCore/svg/graphics/SVGResourceClipper.cpp?rev=52647#L56 . (moved to rendering/RenderSVGResourceClipper.cpp)
The best way would be not to calculate the repaintRect by asking for the clipped area. That means give back FloatRect() instead of iterating through all elements and ask for the unite box of their repaintRects.

[Dirk Schulze]

Ok, I analysed the repaintRect issue. The main problem is, that RenderSVGRoot and RenderSVGContainer can't store the repaintRects at the moment. This causes multiple calls of clipperBoundingBoxForRenderer, maskerBoundingBoxForRenderer and other resources.

[Beth Dakin]

Hey Dirk!

(In reply to comment #2)
> Ok, I analysed the repaintRect issue. The main problem is, that RenderSVGRoot
> and RenderSVGContainer can't store the repaintRects at the moment. This causes
> multiple calls of clipperBoundingBoxForRenderer, maskerBoundingBoxForRenderer
> and other resources.

Why can't they store repaintRects at the moment?

[Dirk Schulze]

(In reply to comment #3)
> Hey Dirk!
> 
> (In reply to comment #2)
> > Ok, I analysed the repaintRect issue. The main problem is, that RenderSVGRoot
> > and RenderSVGContainer can't store the repaintRects at the moment. This causes
> > multiple calls of clipperBoundingBoxForRenderer, maskerBoundingBoxForRenderer
> > and other resources.
> 
> Why can't they store repaintRects at the moment?

The renderer don't know, when the repaintRects need to be recalculated. They don't know if a child or resource changes it's repaintRect. The repaintRect of a container like g is not only the unite of all child repaintRects, it also get clipped to the currently visible area. 
This saves a lot of calculations and unnecessary drawings. One test for this problem is WebCore/manual-test/svg-node-count-vs-scroll.xhtml
You can see the problem on storing the rapaintRect, if you have a big SVG, where you need to scroll.

Never the less. Even if we could store the values, because the renderer _would_ be able to congnize if they need updates, your test would still be slow the first time you press "Sets content of the red text 50 times". We still need to calculate all values at least the first time.
It looks like one problem is getRenderSVGResourceById(). This calls getElementById() what is rather inefficient. We need getRenderSVGResourceById to get the correct repaintRect. 
And we can't surrender this calls, since they can make the repaintRect bigger (eg on using masks or filters).

[Beth Dakin]

Created attachment 51779 [details]
16% improvement

Here is a patch that makes up for some of the regression, but there is still about a 40% regression left to fix. This does require updating the results of one test, but I think the new results are an improvement because they get rid of most of the console error messages, and the metrics seem more correct. Also, the pixel test is exactly the same.

[Darin Adler]

Comment on attachment 51779 [details]
16% improvement

> -        clipRect.unite(styled->renderer()->objectBoundingBox());
> +        FloatRect styledBoundingBox = styled->renderer()->objectBoundingBox();
> +        if (styledBoundingBox.isEmpty())
> +            continue;
> +        clipRect.unite(styledBoundingBox);

Why is this change needed? The unite function already checks for an empty rectangle and does nothing in that case.

[Beth Dakin]

(In reply to comment #6)
> (From update of attachment 51779 [details])
> > -        clipRect.unite(styled->renderer()->objectBoundingBox());
> > +        FloatRect styledBoundingBox = styled->renderer()->objectBoundingBox();
> > +        if (styledBoundingBox.isEmpty())
> > +            continue;
> > +        clipRect.unite(styledBoundingBox);
> 
> Why is this change needed? The unite function already checks for an empty
> rectangle and does nothing in that case.

Oops! It's not.

[Beth Dakin]

Created attachment 51781 [details]
Better 16% Improvement

[Eric Seidel (no email)]

Comment on attachment 51781 [details]
Better 16% Improvement

What test would fail if this change were wrong?

[Beth Dakin]

I'm not sure I understand the question. We have a number of tests that use clip-path and exercise this code path, and they all still pass.

[Beth Dakin]

Here's a summary of what I learned about this performance regression. This could be helpful in attempting to fix the rest of the regression.

1. clipperBoundingBoxForRenderer() was the major source of the regression. This patch optimizes that function a lot, but it could probably be optimized more. There is a lot of overhead associated with getRenderSVGResourceById(). It would be better if SVGRenderStyle stored a pointer to the resource instead of just the ID. There should be a way to make sure these are reset whenever an id on the page changes.

2. It might help to inline clipperBoundingBoxForRenderer(). And filterBoundingBoxForRenderer() and maskerBoundingBoxForRenderer() while we're at it. 

3. Being able to cache repaint rects in RenderSVGContainer and RenderSVGRoot actually would NOT speed up this test case, but it would speed up other tests. The cached repaint rect would need to be invalidated in layout, and by changing the text content, this test case lays out so frequently that there would be nothing to gain from caching repaint rects in these classes.

[Beth Dakin]

Committed with r56644. I am going to leave this bug open since there is still work to be done.

[Nikolas Zimmermann]

(In reply to comment #11)
> Here's a summary of what I learned about this performance regression. This
> could be helpful in attempting to fix the rest of the regression.
> 
> 1. clipperBoundingBoxForRenderer() was the major source of the regression. This
> patch optimizes that function a lot, but it could probably be optimized more.
> There is a lot of overhead associated with getRenderSVGResourceById(). It would
> be better if SVGRenderStyle stored a pointer to the resource instead of just
> the ID. There should be a way to make sure these are reset whenever an id on
> the page changes.
Excellent idea! Dirk and me brainstormed several ideas as well yesterday -- it's completly clear that the ID based approach is too slow. We want to finish the SVGResource -> RenderSVGResource transition first, then fix the performance issues. Dirk has the patches almost completed (except for filter, where someone else is looking into atm).

> 
> 2. It might help to inline clipperBoundingBoxForRenderer(). And
> filterBoundingBoxForRenderer() and maskerBoundingBoxForRenderer() while we're
> at it. 
> 
> 3. Being able to cache repaint rects in RenderSVGContainer and RenderSVGRoot
> actually would NOT speed up this test case, but it would speed up other tests.
> The cached repaint rect would need to be invalidated in layout, and by changing
> the text content, this test case lays out so frequently that there would be
> nothing to gain from caching repaint rects in these classes.

There's a problem with the caching, as the render tree has no idea when transformations change, they all just pull them from the SVG DOM every time on paint() (see RenderPath). I am working towards a new transform handling, that will fix that and allow us to cache those rects.

Glad you've tracked down the first performance regressions!

[Beth Dakin]

That all sounds great Niko! Awesome!

[Eric Seidel (no email)]

Attachment 51781 [details] was posted by a committer and has review+, assigning to Beth Dakin for commit.

[Beth Dakin]

That patch has been committed already. See comment #12.

[Eric Seidel (no email)]

Comment on attachment 51781 [details]
Better 16% Improvement

Obsoleting this patch since it's already been committed.

[Steven Lai]

> 3. Being able to cache repaint rects in RenderSVGContainer and RenderSVGRoot
> actually would NOT speed up this test case, but it would speed up other tests.
> The cached repaint rect would need to be invalidated in layout, and by changing
> the text content, this test case lays out so frequently that there would be
> nothing to gain from caching repaint rects in these classes.

I'm thinking that caching would still help because we're calling repaintRectInLocalCoordinates() and computeContainerBoundingBox() several times after layout is validated and during the repainting phase.

[Steven Lai]

beth is right, the repeated bounding box calcs don't seem to be the bottleneck

[Steven Lai]

Do you think that caching bounding boxes and invalidate them in setNeedsTransformUpdate() and setNeedsLayout() would be the right thing to do?

[Steven Lai]

> The renderer don't know, when the repaintRects need to be recalculated. They
> don't know if a child or resource changes it's repaintRect. The repaintRect of
> a container like g is not only the unite of all child repaintRects, it also get
> clipped to the currently visible area. 
> This saves a lot of calculations and unnecessary drawings. One test for this
> problem is WebCore/manual-test/svg-node-count-vs-scroll.xhtml
> You can see the problem on storing the rapaintRect, if you have a big SVG,
> where you need to scroll.
> 

I just tried caching the local repaintRect in RenderSVGContainer. And that seems to fix the performance regression. What still worries me is that you mentioned the render doesn't know its child or resources changes it's repaintRect. But when I test it layout() still seems to be called.

[Steven Lai]

> a container like g is not only the unite of all child repaintRects, it also get
> clipped to the currently visible area. 

By the way, I think clipping to the visible area could introduce a problem because the <g> can be used to define a pattern fill for an object which is larger than the visible area. Anyone know where the visible area clipping is performed?

[Steven Lai]

Created attachment 55310 [details]
Test to see if RenderSVGContainer::layout() is called

By the way, I was checking with a test cases:

test 1. changing the height of a rect inside a <g>
test 2. append an element inside a <g>
test 3. setting a clip path to a <g>
test 4. setting the height of the clip path inside a <g> 

In all four cases, RenderSVGContainer::layout() is called

[Steven Lai]

Created attachment 55314 [details]
Attempted patch

Attaching my attempted patch so that you guys can see if it's problematic.
It simply caches repaint rects of container render node like what other svg render nodes are doing.

[Beth Dakin]

Steven, what is the speed increase in this patch? It is best to measure with a Release build, which perhaps you already know, but I just wanted to be clear :-)

[Steven Lai]

~1500ms down to ~650ms on my macbook (it still includes the ~500ms which result from waiting setTimeout(0) to fire 50 times)

[Steven Lai]

actually the regression affects changes to any render node, i replaced changing text with changing rect fill and we would still get noticeable performance degradation. (in that case, the change ~800ms down to ~500ms)

[Dirk Schulze]

(In reply to comment #27)
> actually the regression affects changes to any render node, i replaced changing
> text with changing rect fill and we would still get noticeable performance
> degradation. (in that case, the change ~800ms down to ~500ms)

Please try scrolling to all dimensions on http://trac.webkit.org/export/58939/trunk/WebCore/manual-tests/svg-node-count-vs-scroll.xhtml . Does it still rerender and relayout?

[Steven Lai]

> Please try scrolling to all dimensions on
> http://trac.webkit.org/export/58939/trunk/WebCore/manual-tests/svg-node-count-vs-scroll.xhtml
> . Does it still rerender and relayout?

Haven't checked if it goes thru relayout and rerender code path.

But I resized several times and scrolled both vertically and horizontally several times after resizing.
The rendering seems fine.

The scrolling seems smooth now too :-)

[Dirk Schulze]

(In reply to comment #29)
> > Please try scrolling to all dimensions on
> > http://trac.webkit.org/export/58939/trunk/WebCore/manual-tests/svg-node-count-vs-scroll.xhtml
> > . Does it still rerender and relayout?
> 
> Haven't checked if it goes thru relayout and rerender code path.
> 
> But I resized several times and scrolled both vertically and horizontally
> several times after resizing.
> The rendering seems fine.
> 
> The scrolling seems smooth now too :-)

Niko, does stevens caching work because of your latest transformation changes? IIRC caching wasn't possible several weeks ago.

[Steven Lai]

> Niko, does stevens caching work because of your latest transformation changes?
> IIRC caching wasn't possible several weeks ago.

Dirk, you have the Niko's changeset so that i could take a look at it
i'm still trying to understand the layout repaint pipeline :)

[Nikolas Zimmermann]

Comment on attachment 55314 [details]
Attempted patch

Hi Steven,

patch looks partly good! In fact I wanted to try a similar approach, though some comments:
The best approach would be to actually _recalculate_ the repaintRect in layout() - through a helper function like updateCachedBoundaries() - what RenderPath is doing.
The reason for that is that you can inline repaintRectInLocalCoordinates() which should give an even more performance improvement, as the function can just return the m_cachedLocalRepaintRect.

Regarding Dirks concerns about when layout() is called:
Indeed it wasn't possible to cache these rects some weeks ago, when we still had the SVGResource vs. RenderSVGResource confusion. In case Steven doesn't know we recently converted all "resources" (masker/clipper/gradients/patterns/markers) into the render tree to avoid a special concept SVGResource/SVGPaintServer, dating back to old ksvg2 times in 2004. The issue was that relayouting wasn't handled properly. Nowadays the resources inform all clients when they have changed (eg. changing patternTransform attribute of a <pattern> notifies all clients to relayout). Furthermore the clients invalidate their resources, when they have changed (eg. rect changed size, tell it's RenderSVGResourcePattern, to update the cached data, as example..). Until ~ 2 weeks ago, RenderPath did NOT know why a layout occoured: path data changed / transform changed / boundaries changed? This is all handled properly now, and I think we're ready to move the caching up to the next level: the RenderContainer.

Some IMHO this approach should work. Can you try implementing the suggestion above, and then run "run-webkit-tests --tolerance 0 -p svg" and watch careful if anything changes. If not I'm happy to r+ your patch!
(Note: You may see some "noise", tests failing with 0.01% difference, because of CoreGraphics differences across the Mac platforms, Leopard/SnowLeopard, etc..)

.

[Steven Lai]

When I tried adding the call updateCachedBoundaries. I should place the call in RenderSVGContainer::layout() just after the call to layoutChildren, if I do so, I don't get any performance improvement. But if I do it wrongly by placing the call before layoutChildren, I get the performance improvment.

Does this sound weird?

[Steven Lai]

Created attachment 55901 [details]
Proposed Patch

Attaching my patch after making some changes as per Niko's advice last night.
(Note: Niko also has a patch that also covers this part)

[Steven Lai]

Created attachment 55902 [details]
Test case

Attached test case that shows significant difference with and without the caching patch applied.
(Included as a manual-test in the proposed patch)

[Nikolas Zimmermann]

The patch is good, but we should wait until we find the issue of the thousands of repaintRectInLocalCoordinates() calls. It's also related to text, and I'm just rewriting SVG Text support.
Will drop you a line as soon as I have some news.

[Dirk Schulze]

Comment on attachment 55901 [details]
Proposed Patch

I agree to Niko, that we need to find out where the multiple calls of layout come from. But I don't think that this realy blocks this patch. After talking with Niko, he was fine to land this patch, if we open a new bug report, where this problem is discribed and linked to this bug report.

Nevertheless I r- this patch because of the style of the ChangeLog. Ther ChangeLog misses the title of this bugReport, a link to this bugreport and maybe one or two more sentences what you're doing, and how webkit can benefit from this changes.
WebKitTools/Scripts/prepare-ChangeLog --bug 36564 can help you to create a style correct ChangeLog.

[Nikolas Zimmermann]

Created attachment 60211 [details]
Initial patch

Greatly reduced the number of repaintRectInLocalCoordinates() calls - even without caching.
Such a simple patch, with high impact.

[Eric Seidel (no email)]

Comment on attachment 60211 [details]
Initial patch

WebCore/rendering/RenderSVGContainer.h:58
 +      virtual FloatRect objectBoundingBox() const { return SVGRenderSupport::computeContainerBoundingBox(this, false); }
virtual functions can never be inlined, so this change does nothing other than hurt readability of the header. :(

[Eric Seidel (no email)]

Comment on attachment 60211 [details]
Initial patch

WebCore/rendering/SVGRenderSupport.cpp:107
 +              repaintRect = object->repaintRectInLocalCoordinates();
Should this ASSERT that the repaintRect has not been computed?  ASSERT(repaintRect == FloatRect())? or whatever the ASSERT would be.

Otherwise this looks fine.  Please fix the bogus inlining.

[Nikolas Zimmermann]

Created attachment 60215 [details]
Updated patch

Dirk wants me to remove an uneeded include in RenderPath.h - fixed Erics issue. Thanks guys!

[Nikolas Zimmermann]

(In reply to comment #40)
> (From update of attachment 60211 [details])
> WebCore/rendering/SVGRenderSupport.cpp:107
>  +              repaintRect = object->repaintRectInLocalCoordinates();
> Should this ASSERT that the repaintRect has not been computed?  ASSERT(repaintRect == FloatRect())? or whatever the ASSERT would be.
I don't think it should assert, it's pretty clear from the code, no? :-)
 
> Otherwise this looks fine.  Please fix the bogus inlining.

Fixed, and sorry - did not see your comment before uploading a new version.
But I don't think we'll need the assert though.

[Dirk Schulze]

Comment on attachment 60215 [details]
Updated patch

I agree to Niko.

[Eric Seidel (no email)]

ASSERTs are always worth while.  ASSERTs validate the assumptions of the code.  If we were initializing that rect twice in some future version of this code, you'd be sad.

[Nikolas Zimmermann]

Landed in r62238.

[Darin Adler]

(In reply to comment #39)
> (From update of attachment 60211 [details])
> WebCore/rendering/RenderSVGContainer.h:58
>  +      virtual FloatRect objectBoundingBox() const { return SVGRenderSupport::computeContainerBoundingBox(this, false); }
> virtual functions can never be inlined, so this change does nothing other than hurt readability of the header. :(

There is one important case where a virtual function body can be inlined. That’s when it’s called explicitly like this:

    RenderSVGContainer::objectBoundingBox()

That often happens inside the body of a derived class's implementation of the same virtual function. For that reason, it can make sense to have virtual functions marked inline. But even if that is the case, we can put the inline function outside the class definition to keep the class definition clean.