Rework the SVGPathConsumer interface

Rework the SVGPathConsumer interface

The per-command interface is replaced with a segment interface. The new
interface in practice only consists of the

  emitSegment(const PathSegment&)

method, but incrementPathSegmentCount() and continueConsuming() need to
be kept around for the sake of SVGPathTraversalStateBuilder.

The two users of the interface - SVGPathParser and SVGPathBlender - are
converted to use the new interface.
For SVGPathParser this is fairly
straightforward (no additional code-changes).
In SVGPathBlender, the "blend switch" is moved into a separate function
and each blend* method is converted to return the blended segment
instead of calling the consumer themselves. Some local variables are
removed in various blend* methods, but otherwise the conversion consist
of assigning blended values to a PathSegmentData, rather than feeding
them to a consumer.

BUG=467592
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192589

[fs, 2015-03-24 18:06:31]

fs@opera.com changed reviewers:
+ fmalita@chromium.org, kouhei@chromium.org, pdr@chromium.org,
schenney@chromium.org

[Stephen Chennney, 2015-03-25 15:05:25]

LGTM. But please modify the FIXME to be a TODO before committing. As I
understand it the TODO is referring to my other comment.

https://codereview.chromium.org/1037463002/diff/1/Source/core/svg/SVGPathStri...
File Source/core/svg/SVGPathStringBuilder.cpp (right):

https://codereview.chromium.org/1037463002/diff/1/Source/core/svg/SVGPathStri...
Source/core/svg/SVGPathStringBuilder.cpp:57: // FIXME: Centralized location for
this (SVGPathSeg.h?)
FIXMEs now must be TODO(name). See thread on blink-dev.

https://codereview.chromium.org/1037463002/diff/1/Source/core/svg/SVGPathStri...
Source/core/svg/SVGPathStringBuilder.cpp:57: // FIXME: Centralized location for
this (SVGPathSeg.h?)
Is this a repeat of some other block? I recall seeing it elsewhere.

[f(malita), 2015-03-25 15:08:15]

The new approach certainly feels cleaner.

LGTM % nit/perf question.

https://codereview.chromium.org/1037463002/diff/1/Source/core/svg/SVGPathBlen...
File Source/core/svg/SVGPathBlender.cpp (right):

https://codereview.chromium.org/1037463002/diff/1/Source/core/svg/SVGPathBlen...
Source/core/svg/SVGPathBlender.cpp:213: blendedSegment.command =
m_isInFirstHalfOfAnimation ? fromSeg.command : toSeg.command;
This is common logic within all these methods - would it look nicer if we
instead handle it in blendSegments() (and pass the result blendedSegment as an
out-param)?

(maybe trading functional encapsulation for DRYness is not worth it - up to you)

https://codereview.chromium.org/1037463002/diff/1/Source/core/svg/SVGPathBlen...
Source/core/svg/SVGPathBlender.cpp:277: m_consumer->emitSegment(blendedSegment);
Do you know if we have any perf coverage for this code? Any chance to run some
tests and make sure we're not regressing?

I'm just a tad worried because now we're going through this intermediate segment
representation instead of the previously more direct consumer calls.

[fs, 2015-03-25 17:08:14]

https://codereview.chromium.org/1037463002/diff/1/Source/core/svg/SVGPathBlen...
File Source/core/svg/SVGPathBlender.cpp (right):

https://codereview.chromium.org/1037463002/diff/1/Source/core/svg/SVGPathBlen...
Source/core/svg/SVGPathBlender.cpp:213: blendedSegment.command =
m_isInFirstHalfOfAnimation ? fromSeg.command : toSeg.command;
On 2015/03/25 15:08:15, f(malita) wrote:
> This is common logic within all these methods - would it look nicer if we
> instead handle it in blendSegments() (and pass the result blendedSegment as an
> out-param)?
> 
> (maybe trading functional encapsulation for DRYness is not worth it - up to
you)

Yes, I think I have a follow-up that "commons up" this code a bit. There's more
common ground here. Expect something soonish.

https://codereview.chromium.org/1037463002/diff/1/Source/core/svg/SVGPathBlen...
Source/core/svg/SVGPathBlender.cpp:277: m_consumer->emitSegment(blendedSegment);
On 2015/03/25 15:08:15, f(malita) wrote:
> Do you know if we have any perf coverage for this code? Any chance to run some
> tests and make sure we're not regressing?
> 
> I'm just a tad worried because now we're going through this intermediate
segment
> representation instead of the previously more direct consumer calls.

I'm not aware of any tests like that, so I suppose I'd have to ad-hoc something
together. Since I measured a general improvement for the parser case I'm not
terribly worried about the  "dispatch cost" - PathSegmentData is small (< 32
bytes, so should fit in a single cacheline on anything we care about), and
otherwise the cost should be the same - so it's mainly the cost of the switch
above I think. I'll see if I can dig up some animated path that is heavy enough
(hopefully I have one on file already somewhere...)

https://codereview.chromium.org/1037463002/diff/1/Source/core/svg/SVGPathStri...
File Source/core/svg/SVGPathStringBuilder.cpp (right):

https://codereview.chromium.org/1037463002/diff/1/Source/core/svg/SVGPathStri...
Source/core/svg/SVGPathStringBuilder.cpp:57: // FIXME: Centralized location for
this (SVGPathSeg.h?)
On 2015/03/25 15:05:25, Stephen Chenney wrote:
> Is this a repeat of some other block? I recall seeing it elsewhere.

I don't _think_ this exact block exists somewhere else. SVGPathStringSource
obviously have the reverse mapping (via a switch), and
SVGPathSeg::pathSegTypeAsLetter - the latter of which I hope can be joined with
this one in some way.

[fs, 2015-03-25 17:56:11]

https://codereview.chromium.org/1037463002/diff/1/Source/core/svg/SVGPathBlen...
File Source/core/svg/SVGPathBlender.cpp (right):

https://codereview.chromium.org/1037463002/diff/1/Source/core/svg/SVGPathBlen...
Source/core/svg/SVGPathBlender.cpp:277: m_consumer->emitSegment(blendedSegment);
On 2015/03/25 17:08:13, fs wrote:
> On 2015/03/25 15:08:15, f(malita) wrote:
> > Do you know if we have any perf coverage for this code? Any chance to run
some
> > tests and make sure we're not regressing?
> > 
> > I'm just a tad worried because now we're going through this intermediate
> segment
> > representation instead of the previously more direct consumer calls.
> 
> I'm not aware of any tests like that, so I suppose I'd have to ad-hoc
something
> together. Since I measured a general improvement for the parser case I'm not
> terribly worried about the  "dispatch cost" - PathSegmentData is small (< 32
> bytes, so should fit in a single cacheline on anything we care about), and
> otherwise the cost should be the same - so it's mainly the cost of the switch
> above I think. I'll see if I can dig up some animated path that is heavy
enough
> (hopefully I have one on file already somewhere...)

I extracted some path animations from some SVGs I had lying around - producing
[1], and then multiplied those by a few hundred (lost count, but ~600 I think).
SVGPathBlender barely registered on the profile. The emitSegment() consumer
showed up with ~CoalescingBuffer (inlined) as the hotspot (unsurprisingly, and
nothing to do with this CL really). Otherwise it seems the (SMIL) animation code
is too inefficient when it comes to path animations (I suspect it reparses the
values too often.) So any further investigation would need to be of the
microbench kind to not get shadowed by the other bottlenecks...

FWIW I'll try to look into this some more if possible (I've been wanting to
squash the ~CoalescingBuffer inefficiency mentioned above...)

[1] http://jsfiddle.net/e33ybbdh/

[f(malita), 2015-03-25 18:18:27]

On 2015/03/25 17:56:11, fs wrote:
> I extracted some path animations from some SVGs I had lying around - producing
> [1], and then multiplied those by a few hundred (lost count, but ~600 I
think).
> SVGPathBlender barely registered on the profile. The emitSegment() consumer
> showed up with ~CoalescingBuffer (inlined) as the hotspot (unsurprisingly, and
> nothing to do with this CL really). Otherwise it seems the (SMIL) animation
code
> is too inefficient when it comes to path animations (I suspect it reparses the
> values too often.) So any further investigation would need to be of the
> microbench kind to not get shadowed by the other bottlenecks...
> 
> FWIW I'll try to look into this some more if possible (I've been wanting to
> squash the ~CoalescingBuffer inefficiency mentioned above...)
> 
> [1] http://jsfiddle.net/e33ybbdh/

Thanks for checking! lgtm++

[fs, 2015-03-26 08:39:47]

The CQ bit was checked by fs@opera.com

[fs, 2015-03-26 08:39:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
schenney@chromium.org
Link to the patchset: https://codereview.chromium.org/1037463002/#ps20001
(title: "Rebase; FIXME -> TODO.")

[commit-bot: I haz the power, 2015-03-26 08:40:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1037463002/20001

[commit-bot: I haz the power, 2015-03-26 08:43:35]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192589