Bug 44374 - Changing cx/cy causes SVGCircleElement to regenerate path data when not needed
Summary: Changing cx/cy causes SVGCircleElement to regenerate path data when not needed
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: Nobody
URL:
Keywords:
Depends on:
Blocks: 44402 44375 65236
  Show dependency treegraph
 
Reported: 2010-08-20 19:46 PDT by Eric Seidel (no email)
Modified: 2012-06-23 13:39 PDT (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2010-08-20 19:46:42 PDT
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.
Comment 1 Eric Seidel (no email) 2010-08-20 19:59:14 PDT
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.
Comment 2 Dirk Schulze 2010-08-20 21:58:19 PDT
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.
Comment 3 Eric Seidel (no email) 2010-08-21 07:33:49 PDT
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.
Comment 4 Eric Seidel (no email) 2010-08-21 09:41:54 PDT
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?
Comment 5 Dirk Schulze 2010-08-21 09:45:11 PDT
Commented on the wrong bug, see https://bugs.webkit.org/show_bug.cgi?id=44375#c6
Comment 6 Dirk Schulze 2010-08-21 10:01:37 PDT
(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.
Comment 7 Rob Buis 2011-07-28 10:12:55 PDT
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.
Comment 8 Dirk Schulze 2012-02-01 21:49:46 PST
Reni, are you continuing you work on RenderSVGShape, so that this bug get unnecessary?
Comment 9 Philip Rogers 2012-06-23 13:39:33 PDT
Fixed in http://trac.webkit.org/changeset/112667