Lazily allocate svg shape transforms, shrink RenderSVGShape by 44B

Lazily allocate svg shape transforms, shrink RenderSVGShape by 44B

This patch switches to lazily allocating transforms in RenderSVGShape
because local transforms are relatively rare.

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

[pdr., 2014-11-03 01:12:39]

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

[fs, 2014-11-05 13:46:47]

lgtm

https://codereview.chromium.org/701433002/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGShape.cpp (right):

https://codereview.chromium.org/701433002/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGShape.cpp:127: m_localTransform =
graphicsElement->hasAnimatedLocalTransform() ? adoptPtr(new
AffineTransform(graphicsElement->calculateAnimatedLocalTransform())) : 0;
It'd be nice to not reallocate the transform for each update, but I suppose a
micro-bench would be needed to even observe it - and we're already assuming that
the transform will rarely be there anyway so probably not worth the additional
code complexity at this point in time.

https://codereview.chromium.org/701433002/diff/1/Source/core/svg/SVGGraphicsE...
File Source/core/svg/SVGGraphicsElement.cpp (right):

https://codereview.chromium.org/701433002/diff/1/Source/core/svg/SVGGraphicsE...
Source/core/svg/SVGGraphicsElement.cpp:128: return (style &&
style->hasTransform()) || !m_transform->currentValue()->isEmpty() ||
hasSVGRareData();
Maybe there ought to be a comment here pointing to the method below - which
would serve to explain why thi predicate is correct (hasSVGRareData() will not
be directly obvious for instance).

[fs, 2014-11-05 13:49:40]

Oh, and shouldn't it be "44B" [32-bit, 40B 64-bit]? (sizeof(AffineTransform) -
sizeof(OwnPtr<...> => 48 - (4 or 8))

[f(malita), 2014-11-06 22:16:15]

lgtm++

https://codereview.chromium.org/701433002/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGShape.cpp (right):

https://codereview.chromium.org/701433002/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGShape.cpp:127: m_localTransform =
graphicsElement->hasAnimatedLocalTransform() ? adoptPtr(new
AffineTransform(graphicsElement->calculateAnimatedLocalTransform())) : 0;
On 2014/11/05 13:46:47, fs wrote:
> It'd be nice to not reallocate the transform for each update, but I suppose a
> micro-bench would be needed to even observe it - and we're already assuming
that
> the transform will rarely be there anyway so probably not worth the additional
> code complexity at this point in time.

+1 for avoiding unnecessary reallocations. Even if local transforms are rare for
shapes, we shouldn't penalize outliers.

[pdr., 2014-11-07 04:20:27]

https://codereview.chromium.org/701433002/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGShape.cpp (right):

https://codereview.chromium.org/701433002/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGShape.cpp:127: m_localTransform =
graphicsElement->hasAnimatedLocalTransform() ? adoptPtr(new
AffineTransform(graphicsElement->calculateAnimatedLocalTransform())) : 0;
On 2014/11/06 at 22:16:15, Florin Malita wrote:
> On 2014/11/05 13:46:47, fs wrote:
> > It'd be nice to not reallocate the transform for each update, but I suppose
a
> > micro-bench would be needed to even observe it - and we're already assuming
that
> > the transform will rarely be there anyway so probably not worth the
additional
> > code complexity at this point in time.
> 
> +1 for avoiding unnecessary reallocations. Even if local transforms are rare
for shapes, we shouldn't penalize outliers.

Agreed and done :)

https://codereview.chromium.org/701433002/diff/1/Source/core/svg/SVGGraphicsE...
File Source/core/svg/SVGGraphicsElement.cpp (right):

https://codereview.chromium.org/701433002/diff/1/Source/core/svg/SVGGraphicsE...
Source/core/svg/SVGGraphicsElement.cpp:128: return (style &&
style->hasTransform()) || !m_transform->currentValue()->isEmpty() ||
hasSVGRareData();
On 2014/11/05 at 13:46:47, fs wrote:
> Maybe there ought to be a comment here pointing to the method below - which
would serve to explain why thi predicate is correct (hasSVGRareData() will not
be directly obvious for instance).

Excellent point. Done

[pdr., 2014-11-07 04:21:33]

PTAL

https://codereview.chromium.org/701433002/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGShape.cpp (right):

https://codereview.chromium.org/701433002/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGShape.cpp:127: if
(graphicsElement->hasAnimatedLocalTransform()) {
I'm not a huge fan of how this looks... do either of you have suggestions for
making this better?

[f(malita), 2014-11-07 18:23:12]

https://codereview.chromium.org/701433002/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGShape.cpp (right):

https://codereview.chromium.org/701433002/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGShape.cpp:127: if
(graphicsElement->hasAnimatedLocalTransform()) {
On 2014/11/07 04:21:33, pdr wrote:
> I'm not a huge fan of how this looks... do either of you have suggestions for
> making this better?

Not necessarily better, but if you want to optimize for legibility and willing
to eat up a double-initialization on allocation we could do

    if (!m_localTransform)
        m_localTransform = adoptPtr(new AffineTransform());
    *m_localTransform = graphicsElement->calculateAnimatedLocalTransform();

Up to you, I don't feel strongly either way.

[fs, 2014-11-10 11:36:43]

https://codereview.chromium.org/701433002/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGShape.cpp (right):

https://codereview.chromium.org/701433002/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGShape.cpp:127: if
(graphicsElement->hasAnimatedLocalTransform()) {
On 2014/11/07 18:23:12, Florin Malita wrote:
> On 2014/11/07 04:21:33, pdr wrote:
> > I'm not a huge fan of how this looks... do either of you have suggestions
for
> > making this better?
> 
> Not necessarily better, but if you want to optimize for legibility and willing
> to eat up a double-initialization on allocation we could do
> 
>     if (!m_localTransform)
>         m_localTransform = adoptPtr(new AffineTransform());
>     *m_localTransform = graphicsElement->calculateAnimatedLocalTransform();
> 
> Up to you, I don't feel strongly either way.

No greater suggestions here either. You could get rid of the double-init above
with a new AffineTransform constructor though - if it's felt to be a too large
sacrifice.

https://codereview.chromium.org/701433002/diff/20001/Source/platform/transfor...
File Source/platform/transforms/AffineTransform.h (right):

https://codereview.chromium.org/701433002/diff/20001/Source/platform/transfor...
Source/platform/transforms/AffineTransform.h:159: AffineTransform& operator
=(const AffineTransform &t)
Hmm, why was this needed? Worrying about self-assignments? (I don't see that
happening with the newly added code?)

[pdr., 2014-11-11 04:45:46]

On 2014/11/10 at 11:36:43, fs wrote:
>
https://codereview.chromium.org/701433002/diff/20001/Source/platform/transfor...
> Source/platform/transforms/AffineTransform.h:159: AffineTransform& operator
=(const AffineTransform &t)
> Hmm, why was this needed? Worrying about self-assignments? (I don't see that
happening with the newly added code?)

It was used in the assignment. This may have been too c++ey so I switched this
to use AffineTransform->setTransform() instead of the = operator.

I also moved the transform update code into a helper and
RenderSVGShape::layout() looks pretty now.

[pdr., 2014-11-11 04:51:52]

Thanks for the reviews. I'm surprised we don't have a good pattern for the lazy
OwnPtr approach but I wasn't able to find other examples.


I think this is ready to go now. Please take a quick look and CQ if you like it.

[fs, 2014-11-11 10:09:28]

The CQ bit was checked by fs@opera.com

[fs, 2014-11-11 10:09:29]

lgtm

On 2014/11/11 04:45:46, pdr wrote:
> On 2014/11/10 at 11:36:43, fs wrote:
> >
>
https://codereview.chromium.org/701433002/diff/20001/Source/platform/transfor...
> > Source/platform/transforms/AffineTransform.h:159: AffineTransform& operator
> =(const AffineTransform &t)
> > Hmm, why was this needed? Worrying about self-assignments? (I don't see that
> happening with the newly added code?)
> 
> It was used in the assignment. This may have been too c++ey so I switched this
> to use AffineTransform->setTransform() instead of the = operator.

I think I was more wondering why the default (compiler-generated) version didn't
suffice. It gone now though, so nvm.
 
> I also moved the transform update code into a helper and
> RenderSVGShape::layout() looks pretty now.

Yes, much better.

On 2014/11/11 04:51:52, pdr wrote:
> Thanks for the reviews. I'm surprised we don't have a good pattern for the
lazy
> OwnPtr approach but I wasn't able to find other examples.

Right. I was thinking of suggesting "Lazy<T>", but that seemed like overkill
without some other users...
 
> I think this is ready to go now. Please take a quick look and CQ if you like
it.

We'll see how the button copes with this... ;-)

[commit-bot: I haz the power, 2014-11-11 10:09:57]

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

[commit-bot: I haz the power, 2014-11-11 10:56:59]

Committed patchset #3 (id:40001) as 185118