Reduce how often LayoutSVGShape::updateShapeFromElement is called

Reduce how often LayoutSVGShape::updateShapeFromElement is called

LayoutSVGShape::updateShapeFromElement is used by path elements to
create a Path from the 'd' attribute which is relatively expensive.
For non-path subclasses of LayoutSVGShape, updateShapeFromElement
is used to update non-path alternatives (e.g., bounds of an ellipse).

This patch factors out the shape update code (updateShapeFromElement)
from updating the bounding boxes which is now handled in
updateStrokeAndFillBoundingBoxes.

With shape updating and bounding box updating separated, it is possible
to skip the expensive path calculation in updateShapeFromElement when
only the bounding boxes need updating [1].

A small change was needed in LayoutSVG{Rect,Ellipse}::styleDidChange
where a shape update is forced for shape-dependent properties.

[1] http://pr.gg/spinners.html, http://crbug.com/491422

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

[pdr., 2015-05-31 01:37:14]

pdr@chromium.org changed reviewers:
+ esprehn@chromium.org, fs@opera.com

[fs, 2015-06-01 09:09:20]

https://codereview.chromium.org/1158583003/diff/1/Source/core/layout/svg/Layo...
File Source/core/layout/svg/LayoutSVGShape.cpp (right):

https://codereview.chromium.org/1158583003/diff/1/Source/core/layout/svg/Layo...
Source/core/layout/svg/LayoutSVGShape.cpp:159: updateShapeFromElement();
This also updates m_strokeBoundingBox, so we'll need to split that part out
first (and call it below) before this can work. (This appears to be the reason
for some of the expectation changes.)

https://codereview.chromium.org/1158583003/diff/1/Source/core/svg/SVGGeometry...
File Source/core/svg/SVGGeometryElement.cpp (right):

https://codereview.chromium.org/1158583003/diff/1/Source/core/svg/SVGGeometry...
Source/core/svg/SVGGeometryElement.cpp:50: if (attrName ==
SVGNames::vector_effectAttr) {
Isn't this a presentation attribute?

[pdr., 2015-06-15 05:53:03]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-15 05:53:13]

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

[commit-bot: I haz the power, 2015-06-15 07:26:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-15 07:26:17]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)

[pdr., 2015-06-15 20:29:39]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-15 20:30:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1158583003/40001

[pdr., 2015-06-15 20:31:16]

On 2015/06/01 at 09:09:20, fs wrote:

PTAL

[commit-bot: I haz the power, 2015-06-15 21:58:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-15 21:58:39]

Dry run: This issue passed the CQ dry run.

[fs, 2015-06-16 10:54:02]

https://codereview.chromium.org/1158583003/diff/40001/Source/core/layout/svg/...
File Source/core/layout/svg/LayoutSVGEllipse.cpp (right):

https://codereview.chromium.org/1158583003/diff/40001/Source/core/layout/svg/...
Source/core/layout/svg/LayoutSVGEllipse.cpp:98: m_usePathFallback = false;
This is a bit unpleasant - it'd be nicer to not mutate the "geometry state" in
this method. Not a huge deal though (the path should be simple). Maybe something
to consider cleaning up later. (And in the <rect> case it's a bit more involved
because of how the geometry "reuses" the bbox for storage.)

https://codereview.chromium.org/1158583003/diff/40001/Source/core/layout/svg/...
File Source/core/layout/svg/LayoutSVGEllipse.h (right):

https://codereview.chromium.org/1158583003/diff/40001/Source/core/layout/svg/...
Source/core/layout/svg/LayoutSVGEllipse.h:44: void updateShapeFromElement()
override final;
Nit: This class is final

https://codereview.chromium.org/1158583003/diff/40001/Source/core/layout/svg/...
File Source/core/layout/svg/LayoutSVGPath.cpp (right):

https://codereview.chromium.org/1158583003/diff/40001/Source/core/layout/svg/...
Source/core/layout/svg/LayoutSVGPath.cpp:60: // TODO(pdr): It would be ideal to
only call this in updateShapeFromElement
I think the first part (line) of this makes sense. Not so sure about the second
one though - if the/a marker changes we'd want to update the bounds, but not
update the shape since the position ought to be the same still.

https://codereview.chromium.org/1158583003/diff/40001/Source/core/layout/svg/...
File Source/core/layout/svg/LayoutSVGPath.h (right):

https://codereview.chromium.org/1158583003/diff/40001/Source/core/layout/svg/...
Source/core/layout/svg/LayoutSVGPath.h:46: void updateShapeFromElement()
override final;
Nit: This class is final.

https://codereview.chromium.org/1158583003/diff/40001/Source/core/layout/svg/...
File Source/core/layout/svg/LayoutSVGRect.h (right):

https://codereview.chromium.org/1158583003/diff/40001/Source/core/layout/svg/...
Source/core/layout/svg/LayoutSVGRect.h:45: void updateShapeFromElement()
override final;
Nit: final

https://codereview.chromium.org/1158583003/diff/40001/Source/core/layout/svg/...
File Source/core/layout/svg/LayoutSVGShape.cpp (right):

https://codereview.chromium.org/1158583003/diff/40001/Source/core/layout/svg/...
Source/core/layout/svg/LayoutSVGShape.cpp:180: if (m_needsBoundariesUpdate &&
toSVGGeometryElement(element())->selfHasRelativeLengths())
I think this needs to consider the "new" geometric properties ('x', 'y, etc) -
maybe just blanket-compare SVGComputedStyle::layout in
LayoutSVGShape::styleDidChange? (The we'd almost be to drop this bit too -
right?)

[pdr., 2015-06-17 00:05:08]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-17 00:05:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1158583003/60001

[pdr., 2015-06-17 03:50:49]

Thanks for the detailed review. PTAL

https://codereview.chromium.org/1158583003/diff/40001/Source/core/layout/svg/...
File Source/core/layout/svg/LayoutSVGEllipse.cpp (right):

https://codereview.chromium.org/1158583003/diff/40001/Source/core/layout/svg/...
Source/core/layout/svg/LayoutSVGEllipse.cpp:98: m_usePathFallback = false;
On 2015/06/16 at 10:54:02, fs wrote:
> This is a bit unpleasant - it'd be nicer to not mutate the "geometry state" in
this method. Not a huge deal though (the path should be simple). Maybe something
to consider cleaning up later. (And in the <rect> case it's a bit more involved
because of how the geometry "reuses" the bbox for storage.)

Now that we're not rebuilding paths on all changes, it may be a perf and
complexity win to remove LayoutSVGRect and LayoutSVGEllipse entirely. WDYT?

https://codereview.chromium.org/1158583003/diff/40001/Source/core/layout/svg/...
File Source/core/layout/svg/LayoutSVGEllipse.h (right):

https://codereview.chromium.org/1158583003/diff/40001/Source/core/layout/svg/...
Source/core/layout/svg/LayoutSVGEllipse.h:44: void updateShapeFromElement()
override final;
On 2015/06/16 at 10:54:02, fs wrote:
> Nit: This class is final

Done

https://codereview.chromium.org/1158583003/diff/40001/Source/core/layout/svg/...
File Source/core/layout/svg/LayoutSVGPath.cpp (right):

https://codereview.chromium.org/1158583003/diff/40001/Source/core/layout/svg/...
Source/core/layout/svg/LayoutSVGPath.cpp:60: // TODO(pdr): It would be ideal to
only call this in updateShapeFromElement
On 2015/06/16 at 10:54:02, fs wrote:
> I think the first part (line) of this makes sense. Not so sure about the
second one though - if the/a marker changes we'd want to update the bounds, but
not update the shape since the position ought to be the same still.

Done

https://codereview.chromium.org/1158583003/diff/40001/Source/core/layout/svg/...
File Source/core/layout/svg/LayoutSVGPath.h (right):

https://codereview.chromium.org/1158583003/diff/40001/Source/core/layout/svg/...
Source/core/layout/svg/LayoutSVGPath.h:46: void updateShapeFromElement()
override final;
On 2015/06/16 at 10:54:02, fs wrote:
> Nit: This class is final.

Done

https://codereview.chromium.org/1158583003/diff/40001/Source/core/layout/svg/...
File Source/core/layout/svg/LayoutSVGRect.h (right):

https://codereview.chromium.org/1158583003/diff/40001/Source/core/layout/svg/...
Source/core/layout/svg/LayoutSVGRect.h:45: void updateShapeFromElement()
override final;
On 2015/06/16 at 10:54:02, fs wrote:
> Nit: final

Done

https://codereview.chromium.org/1158583003/diff/40001/Source/core/layout/svg/...
File Source/core/layout/svg/LayoutSVGShape.cpp (right):

https://codereview.chromium.org/1158583003/diff/40001/Source/core/layout/svg/...
Source/core/layout/svg/LayoutSVGShape.cpp:180: if (m_needsBoundariesUpdate &&
toSVGGeometryElement(element())->selfHasRelativeLengths())
On 2015/06/16 at 10:54:02, fs wrote:
> I think this needs to consider the "new" geometric properties ('x', 'y, etc) -
maybe just blanket-compare SVGComputedStyle::layout in
LayoutSVGShape::styleDidChange? (The we'd almost be to drop this bit too -
right?)

I think we can just check for the exact property changes we need deeper in
LayoutSVG{Rect,Ellipse}::styleDidChange. I've done this in the latest patchset
and removed this if statement.

[fs, 2015-06-17 11:15:29]

lgtm

https://codereview.chromium.org/1158583003/diff/40001/Source/core/layout/svg/...
File Source/core/layout/svg/LayoutSVGEllipse.cpp (right):

https://codereview.chromium.org/1158583003/diff/40001/Source/core/layout/svg/...
Source/core/layout/svg/LayoutSVGEllipse.cpp:98: m_usePathFallback = false;
On 2015/06/17 03:50:49, pdr wrote:
> On 2015/06/16 at 10:54:02, fs wrote:
> > This is a bit unpleasant - it'd be nicer to not mutate the "geometry state"
in
> this method. Not a huge deal though (the path should be simple). Maybe
something
> to consider cleaning up later. (And in the <rect> case it's a bit more
involved
> because of how the geometry "reuses" the bbox for storage.)
> 
> Now that we're not rebuilding paths on all changes, it may be a perf and
> complexity win to remove LayoutSVGRect and LayoutSVGEllipse entirely. WDYT?

If by that you're implying only using something akin of LayoutSVGPath - and only
storing the Path - I'd be all for that. I'd have some reservations wrt to memory
usage though (ISTR that the default "blocksize" for SkPath is more than the 16
or so bytes we're currently using. There's also the question of if there'll be
overhead analysing the (Sk)Path to get a simpler primitive back (convexity etc.)
Probably worth to check with the Skia guys about that.

[pdr., 2015-06-17 14:38:17]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2015-06-17 14:38:26]

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

[commit-bot: I haz the power, 2015-06-17 15:55:27]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197267