Bug 36564 - Performance regression for setting content of <text> in SVG
Summary: Performance regression for setting content of <text> in SVG
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords: InRadar, Regression
Depends on: 37575
Blocks: 38820
  Show dependency treegraph
 
Reported: 2010-03-24 16:09 PDT by Beth Dakin
Modified: 2010-07-01 11:12 PDT (History)
9 users (show)

See Also:


Attachments
Test case (28.63 KB, application/xhtml+xml)
2010-03-24 16:09 PDT, Beth Dakin
no flags Details
16% improvement (5.71 KB, patch)
2010-03-26 14:44 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Better 16% Improvement (1.73 KB, patch)
2010-03-26 14:55 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Test to see if RenderSVGContainer::layout() is called (1.90 KB, application/xhtml+xml)
2010-05-06 15:40 PDT, Steven Lai
no flags Details
Attempted patch (2.97 KB, patch)
2010-05-06 16:01 PDT, Steven Lai
zimmermann: review-
Details | Formatted Diff | Diff
Proposed Patch (32.86 KB, patch)
2010-05-12 14:24 PDT, Steven Lai
krit: review-
krit: commit-queue-
Details | Formatted Diff | Diff
Test case (29.00 KB, application/xhtml+xml)
2010-05-12 14:29 PDT, Steven Lai
no flags Details
Initial patch (18.96 KB, patch)
2010-07-01 01:04 PDT, Nikolas Zimmermann
eric: review+
Details | Formatted Diff | Diff
Updated patch (17.97 KB, patch)
2010-07-01 01:26 PDT, Nikolas Zimmermann
krit: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2010-03-24 16:09:41 PDT
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>
Comment 1 Dirk Schulze 2010-03-24 23:16:07 PDT
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.
Comment 2 Dirk Schulze 2010-03-25 01:50:23 PDT
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.
Comment 3 Beth Dakin 2010-03-25 11:39:25 PDT
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?
Comment 4 Dirk Schulze 2010-03-25 14:06:04 PDT
(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).
Comment 5 Beth Dakin 2010-03-26 14:44:44 PDT
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.
Comment 6 Darin Adler 2010-03-26 14:47:14 PDT
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.
Comment 7 Beth Dakin 2010-03-26 14:50:25 PDT
(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.
Comment 8 Beth Dakin 2010-03-26 14:55:05 PDT
Created attachment 51781 [details]
Better 16% Improvement
Comment 9 Eric Seidel (no email) 2010-03-26 15:02:17 PDT
Comment on attachment 51781 [details]
Better 16% Improvement

What test would fail if this change were wrong?
Comment 10 Beth Dakin 2010-03-26 15:04:43 PDT
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.
Comment 11 Beth Dakin 2010-03-26 15:06:04 PDT
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.
Comment 12 Beth Dakin 2010-03-26 15:13:25 PDT
Committed with r56644. I am going to leave this bug open since there is still work to be done.
Comment 13 Nikolas Zimmermann 2010-03-27 01:02:57 PDT
(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!
Comment 14 Beth Dakin 2010-03-29 09:09:24 PDT
That all sounds great Niko! Awesome!
Comment 15 Eric Seidel (no email) 2010-03-29 11:40:00 PDT
Attachment 51781 [details] was posted by a committer and has review+, assigning to Beth Dakin for commit.
Comment 16 Beth Dakin 2010-03-29 11:41:47 PDT
That patch has been committed already. See comment #12.
Comment 17 Eric Seidel (no email) 2010-03-29 11:44:02 PDT
Comment on attachment 51781 [details]
Better 16% Improvement

Obsoleting this patch since it's already been committed.
Comment 18 Steven Lai 2010-05-04 19:42:37 PDT
> 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.
Comment 19 Steven Lai 2010-05-05 00:21:00 PDT
beth is right, the repeated bounding box calcs don't seem to be the bottleneck
Comment 20 Steven Lai 2010-05-05 16:37:52 PDT
Do you think that caching bounding boxes and invalidate them in setNeedsTransformUpdate() and setNeedsLayout() would be the right thing to do?
Comment 21 Steven Lai 2010-05-06 13:46:15 PDT
> 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.
Comment 22 Steven Lai 2010-05-06 13:50:41 PDT
> 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?
Comment 23 Steven Lai 2010-05-06 15:40:02 PDT
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
Comment 24 Steven Lai 2010-05-06 16:01:48 PDT
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.
Comment 25 Beth Dakin 2010-05-06 16:11:25 PDT
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 :-)
Comment 26 Steven Lai 2010-05-06 16:15:42 PDT
~1500ms down to ~650ms on my macbook (it still includes the ~500ms which result from waiting setTimeout(0) to fire 50 times)
Comment 27 Steven Lai 2010-05-06 16:21:05 PDT
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)
Comment 28 Dirk Schulze 2010-05-06 23:27:10 PDT
(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?
Comment 29 Steven Lai 2010-05-06 23:39:15 PDT
> 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 :-)
Comment 30 Dirk Schulze 2010-05-07 00:33:02 PDT
(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.
Comment 31 Steven Lai 2010-05-07 00:38:28 PDT
> 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 :)
Comment 32 Nikolas Zimmermann 2010-05-07 04:34:42 PDT
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..)

.
Comment 33 Steven Lai 2010-05-10 13:54:08 PDT
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?
Comment 34 Steven Lai 2010-05-12 14:24:15 PDT
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)
Comment 35 Steven Lai 2010-05-12 14:29:13 PDT
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)
Comment 36 Nikolas Zimmermann 2010-05-26 02:38:35 PDT
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.
Comment 37 Dirk Schulze 2010-06-01 05:53:48 PDT
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.
Comment 38 Nikolas Zimmermann 2010-07-01 01:04:06 PDT
Created attachment 60211 [details]
Initial patch

Greatly reduced the number of repaintRectInLocalCoordinates() calls - even without caching.
Such a simple patch, with high impact.
Comment 39 Eric Seidel (no email) 2010-07-01 01:20:36 PDT
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. :(
Comment 40 Eric Seidel (no email) 2010-07-01 01:24:17 PDT
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.
Comment 41 Nikolas Zimmermann 2010-07-01 01:26:56 PDT
Created attachment 60215 [details]
Updated patch

Dirk wants me to remove an uneeded include in RenderPath.h - fixed Erics issue. Thanks guys!
Comment 42 Nikolas Zimmermann 2010-07-01 01:30:15 PDT
(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.
Comment 43 Dirk Schulze 2010-07-01 01:33:41 PDT
Comment on attachment 60215 [details]
Updated patch

I agree to Niko.
Comment 44 Eric Seidel (no email) 2010-07-01 01:35:22 PDT
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.
Comment 45 Nikolas Zimmermann 2010-07-01 01:38:20 PDT
Landed in r62238.
Comment 46 Darin Adler 2010-07-01 11:12:18 PDT
(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.