Bug 44374 – Changing cx/cy causes SVGCircleElement to regenerate path data when not needed

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Eric Seidel (no email)]

Changing cx/cy causes SVGCircleElement to regenerate path data when not needed

http://themaninblue.com/experiment/AnimationBenchmark/svg

source: http://themaninblue.com/experiment/AnimationBenchmark/svg/js/main.js

We're spending 7% of that benchmark regenerating path data for the circles which is unnecessary.  (45% is spent in RenderPath::paint().)  If instead we used a transform for the cx/cy, we could always draw the circle around 0 and not need to regen the path when cx/cy changes.

[Eric Seidel (no email)]

Layout is close to 16% of that benchmark.  We'd still need to do a layout, but the layout cost would be very small if we could avoid needing to regenerate the path.

[Dirk Schulze]

We could realy use the unite circle and just transform it according to x,y,cx,cy. But that causes some changes, first we would need to store the Path's in the elements (rect,circle,path,polygon,line,...). And we should also get rid of the current ellipse logic in Path::createEllipse and let the platforms draw the circle.

[Eric Seidel (no email)]

As noted in bug 44375, path copying is really expensive.  So all of our Path::create* functions which return a path, cause a copy.  I think we should consider using the unit circle as you suggest.

[Eric Seidel (no email)]

I downloaded the benchmark locally (http://themaninblue.com/experiment/AnimationBenchmark/benchmarks.zip) and modified the SVG benchmark to use transforms instead of setting cx/cy.  We spend 6% time under layout() instead of 16%! (basically working around this bug).  But our framerate still tops out at 30fps.  Maybe we're hitting some sort of framerate cap?

[Dirk Schulze]

Commented on the wrong bug, see https://bugs.webkit.org/show_bug.cgi?id=44375#c6

[Dirk Schulze]

(In reply to comment #4)
> I downloaded the benchmark locally (http://themaninblue.com/experiment/AnimationBenchmark/benchmarks.zip) and modified the SVG benchmark to use transforms instead of setting cx/cy.  We spend 6% time under layout() instead of 16%! 

Well, if you modify the test in that way, we don't invalidate the path. Means, you avoid the copy process from SVG*Element to SVGRenderPath as well. That's what I mean with store the path in the element in comment #2.

[Rob Buis]

Implementing https://bugs.webkit.org/show_bug.cgi?id=65236 could make this bug unneeded. We should keep themaninblue benchmark in mind though for 65236.
Cheers,

Rob.

[Dirk Schulze]

Reni, are you continuing you work on RenderSVGShape, so that this bug get unnecessary?

[Philip Rogers]

Fixed in http://trac.webkit.org/changeset/112667