Bug 44375 – Copying Paths is expensive. toPathData() should avoid causing a copy.

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: SVG

[Eric Seidel (no email)]

Copying Paths is expensive.  toPathData() should avoid causing a copy.

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

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

We're spending 3% of that benchmark in Path::operator=  Crazy!  (45% is spent in RenderPath::paint().)  toPathData() always causes a copy of the Path which is expensive (at least for CoreGraphics as it's a malloc).

We either should make Path, COW, or we should find a way to avoid this copy in this return.

[Eric Seidel (no email)]

It's actually closer to 4%.

[Eric Seidel (no email)]

This will stop showing up on this benchmark if bug 44374 is fixed, since we could avoid regenerating path data all together.

[Eric Seidel (no email)]

Created attachment 65026 [details]
one possible approach

[Eric Seidel (no email)]

Comment on attachment 65026 [details]
one possible approach

This doesn't really change our performance on http://themaninblue.com/experiment/AnimationBenchmark/svg/

I think a better approach would be the one mentioned in 44374 of using the unit circle and transforming it.

This might speed up some painting ever so slightly, since we're not copying Paths as much.  All of the uses of Path::create* were causing a Path copy.

In the shark sample, this brought our time spent under SVGCircleElement::toPathData up to 8.9% after the change, but before the change, toPathData + Path::operator= + CFRelease (which were all related to copying paths) was over 12% in total.

I'm not sure if I should finish this land this patch or not, but I'm putting it here for comments anyway.

[Eric Seidel (no email)]

It's possible this could be a tiny win on the PLT, if the PLT uses any of the GraphicsContext code paths which were previously copying paths.

I'm not able to run the PLT, so I can't tell you.

[Dirk Schulze]

(In reply to comment #4)
> (From update of attachment 65026 [details])
> This doesn't really change our performance on http://themaninblue.com/experiment/AnimationBenchmark/svg/
> 
> I think a better approach would be the one mentioned in 44374 of using the unit circle and transforming it.
> 
> This might speed up some painting ever so slightly, since we're not copying Paths as much.  All of the uses of Path::create* were causing a Path copy.
> 
> In the shark sample, this brought our time spent under SVGCircleElement::toPathData up to 8.9% after the change, but before the change, toPathData + Path::operator= + CFRelease (which were all related to copying paths) was over 12% in total.
> 
> I'm not sure if I should finish this land this patch or not, but I'm putting it here for comments anyway.

I think I had perf problems with path transformations in CG in the past. IIRC CG copys the patch on transformations anyway. So it might not bring a better performance. I Discussed a similar issue with Oliver in Canvas. Oliver should know better.

[Kenneth Russell]

Comment on attachment 65026 [details]
one possible approach

I think that the general direction of making Path RefCounted (https://bugs.webkit.org/show_bug.cgi?id=44402) and reusing the unit circle's path (https://bugs.webkit.org/show_bug.cgi?id=44374) are better than making these changes simply to avoid calling Path's copy constructor and/or assignment operator. Note that with these changes there is still no caching of Paths going on inside of routines like RenderMathMLRoot::paint, GraphicsContext::addRoundedRectClip, etc., and no clear opportunity to do so.

If it seems that making Path RefCounted would be a loss because of additional mallocs/frees during temporary Path construction and destruction, perhaps we could consider making a cross-platform internal data structure in Path which holds the PlatformPathPtr and itself is reference counted.

[Dirk Schulze]

Andreas is working on a solution for SVG on bug 46052.

*** This bug has been marked as a duplicate of bug 46052 ***