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>
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.
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.
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?
(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).
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 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.
(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.
Created attachment 51781 [details] Better 16% Improvement
Comment on attachment 51781 [details] Better 16% Improvement What test would fail if this change were wrong?
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.
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.
Committed with r56644. I am going to leave this bug open since there is still work to be done.
(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!
That all sounds great Niko! Awesome!
Attachment 51781 [details] was posted by a committer and has review+, assigning to Beth Dakin for commit.
That patch has been committed already. See comment #12.
Comment on attachment 51781 [details] Better 16% Improvement Obsoleting this patch since it's already been committed.
> 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.
beth is right, the repeated bounding box calcs don't seem to be the bottleneck
Do you think that caching bounding boxes and invalidate them in setNeedsTransformUpdate() and setNeedsLayout() would be the right thing to do?
> 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.
> 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?
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
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.
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 :-)
~1500ms down to ~650ms on my macbook (it still includes the ~500ms which result from waiting setTimeout(0) to fire 50 times)
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)
(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?
> 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 :-)
(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.
> 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 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..) .
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?
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)
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)
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 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.
Created attachment 60211 [details] Initial patch Greatly reduced the number of repaintRectInLocalCoordinates() calls - even without caching. Such a simple patch, with high impact.
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 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.
Created attachment 60215 [details] Updated patch Dirk wants me to remove an uneeded include in RenderPath.h - fixed Erics issue. Thanks guys!
(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 on attachment 60215 [details] Updated patch I agree to Niko.
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.
Landed in r62238.
(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.