Web Animations API: Reuse cached InterpolableValue during interpolation

Web Animations API: Reuse cached InterpolableValue during interpolation

This change avoids allocating new InterpolableValues on every sample by
updating the existing InterpolableValue cache in Interpolations.

TEST=AnimationInterpolableValueTest

[shans, 2014-04-15 11:53:43]

On 2014/04/15 08:42:42, alancutter wrote:

I want to think about this a bit more. At a minimum, we need to change the name
from 'interpolate' to 'setToResultOfInterpolation' or something similar but less
verbose :)

I wish we had our performance test infrastructure ready to go ... this adds
cognitive effort to understanding the code, but would be worth it if the
performance benefits were tangible.

[shans, 2014-04-15 11:55:13]

On 2014/04/15 11:53:43, shans wrote:
> On 2014/04/15 08:42:42, alancutter wrote:
> 
> I want to think about this a bit more. At a minimum, we need to change the
name
> from 'interpolate' to 'setToResultOfInterpolation' or something similar but
less
> verbose :)
> 
> I wish we had our performance test infrastructure ready to go ... this adds
> cognitive effort to understanding the code, but would be worth it if the
> performance benefits were tangible.

So actually, I think if we do this we want both 'interpolate' and
'setToResultOfInterpolation'. Then we first populate the cache with interpolate,
and update the in-place value on subsequent frames using the other one.

[alancutter (OOO until 2018), 2014-04-16 00:33:33]

On 2014/04/15 11:53:43, shans wrote:
> On 2014/04/15 08:42:42, alancutter wrote:
> 
> I want to think about this a bit more. At a minimum, we need to change the
name
> from 'interpolate' to 'setToResultOfInterpolation' or something similar but
less
> verbose :)
> 
> I wish we had our performance test infrastructure ready to go ... this adds
> cognitive effort to understanding the code, but would be worth it if the
> performance benefits were tangible.

I'm happy to block this on tangible performance gains.

[alancutter (OOO until 2018), 2014-05-02 07:06:11]

On 2014/04/15 11:53:43, shans wrote:
> I want to think about this a bit more. At a minimum, we need to change the
name
> from 'interpolate' to 'setToResultOfInterpolation' or something similar but
less
> verbose :)

Renamed to assignInterpolation().

On 2014/04/16 00:33:33, alancutter wrote:
> I'm happy to block this on tangible performance gains.

Measured this change to have ~2.3% drop in the number of vsyncs per frame for
the CSS transition all in smoothness.tough_animation_cases.

[alancutter (OOO until 2018), 2014-05-02 07:11:24]

On 2014/05/02 07:06:11, alancutter wrote:
> Measured this change to have ~2.3% drop in the number of vsyncs per frame for
> the CSS transition all in smoothness.tough_animation_cases.

This was on a Nexus 10 device.

[Stephen Chennney, 2014-11-10 18:20:21]

schenney@chromium.org changed reviewers:
+ schenney@chromium.org

[Stephen Chennney, 2014-11-10 18:20:22]

I was going with adding a result to the interpolation method, which has the same
effect. I'm fine with this approach.

There's no way I can see this hurting performance. And it will definitely help
Oilpan. No reason to delay as far as I am concerned.

Once this is done, I was thinking of moving the type specialization up to
Interpolator, and get rid of InterpolableValue entirely. We'd then have even
less memory allocation per animation. It seems this might run into problems with
the legacy methods.

[shans, 2014-11-10 19:24:00]

On 2014/11/10 18:20:22, Stephen Chenney wrote:
> I was going with adding a result to the interpolation method, which has the
same
> effect. I'm fine with this approach.
> 
> There's no way I can see this hurting performance. And it will definitely help
> Oilpan. No reason to delay as far as I am concerned.
> 
> Once this is done, I was thinking of moving the type specialization up to
> Interpolator, and get rid of InterpolableValue entirely. We'd then have even
> less memory allocation per animation. It seems this might run into problems
with
> the legacy methods.

Actually, I think adding a result to the interpolation method would be cleaner
and easier to understand.
Do you want to prepare a patch that does that? Otherwise I'm happy to do it, or
I'm sure Alan would too.

I'm not sure about whether moving the type specialization up would have concrete
impact. Once the interpolation result is reused, there is no allocation during
animation (only during setup).

[Stephen Chennney, 2014-11-10 21:19:13]

On 2014/11/10 19:24:00, shans wrote:
> On 2014/11/10 18:20:22, Stephen Chenney wrote:
> > I was going with adding a result to the interpolation method, which has the
> same
> > effect. I'm fine with this approach.
> > 
> > There's no way I can see this hurting performance. And it will definitely
help
> > Oilpan. No reason to delay as far as I am concerned.
> > 
> > Once this is done, I was thinking of moving the type specialization up to
> > Interpolator, and get rid of InterpolableValue entirely. We'd then have even
> > less memory allocation per animation. It seems this might run into problems
> with
> > the legacy methods.
> 
> Actually, I think adding a result to the interpolation method would be cleaner
> and easier to understand.
> Do you want to prepare a patch that does that? Otherwise I'm happy to do it,
or
> I'm sure Alan would too.

I had a patch but was way too proactive in deleting it. So I don't have anything
right now.

Honestly I don't think it's any cleaner or more understandable (less method name
changing, however). No matter what you do you end up with methods that do 2 type
casts, an interpolation and a write to a primitive value. And the list case is
messy.

> I'm not sure about whether moving the type specialization up would have
concrete
> impact. Once the interpolation result is reused, there is no allocation during
> animation (only during setup).

It would cut down on type casts and you could store the value's double, bool,
etc in the Interpolator with fewer pointer chasing operations. Might or might
not be a perf win.

[alancutter (OOO until 2018), 2014-11-10 22:10:45]

On 2014/11/10 19:24:00, shans wrote:
> On 2014/11/10 18:20:22, Stephen Chenney wrote:
> > I was going with adding a result to the interpolation method, which has the
> same
> > effect. I'm fine with this approach.
> > 
> > There's no way I can see this hurting performance. And it will definitely
help
> > Oilpan. No reason to delay as far as I am concerned.
> > 
> > Once this is done, I was thinking of moving the type specialization up to
> > Interpolator, and get rid of InterpolableValue entirely. We'd then have even
> > less memory allocation per animation. It seems this might run into problems
> with
> > the legacy methods.
> 
> Actually, I think adding a result to the interpolation method would be cleaner
> and easier to understand.
> Do you want to prepare a patch that does that? Otherwise I'm happy to do it,
or
> I'm sure Alan would too.

Adding a result parameter to InterpolableValue::interpolate() seems like it
would no longer take advantage of virtual method dispatch the way
assignInterpolation() does currently. I'm not sure that would end up being any
cleaner but I'm happy to be proven wrong.

[shans, 2014-11-10 23:03:48]

On 2014/11/10 21:19:13, Stephen Chenney wrote:
> On 2014/11/10 19:24:00, shans wrote:
> > On 2014/11/10 18:20:22, Stephen Chenney wrote:
> > > I was going with adding a result to the interpolation method, which has
the
> > same
> > > effect. I'm fine with this approach.
> > > 
> > > There's no way I can see this hurting performance. And it will definitely
> help
> > > Oilpan. No reason to delay as far as I am concerned.
> > > 
> > > Once this is done, I was thinking of moving the type specialization up to
> > > Interpolator, and get rid of InterpolableValue entirely. We'd then have
even
> > > less memory allocation per animation. It seems this might run into
problems
> > with
> > > the legacy methods.
> > 
> > Actually, I think adding a result to the interpolation method would be
cleaner
> > and easier to understand.
> > Do you want to prepare a patch that does that? Otherwise I'm happy to do it,
> or
> > I'm sure Alan would too.
> 
> I had a patch but was way too proactive in deleting it. So I don't have
anything
> right now.
> 
> Honestly I don't think it's any cleaner or more understandable (less method
name
> changing, however). No matter what you do you end up with methods that do 2
type
> casts, an interpolation and a write to a primitive value. And the list case is
> messy.

Doug and I just looked at the two approaches side-by-side for
InterpolableNumber.
Keeping an interpolate with a provided output is a lot easier to read, and
preserves
method const-ness. It's a pretty clear win.

The list case is protected by an earlier assert - basically the shape is known
in advanced
and fixed in place by a clone. I don't think it is too messy?

> > I'm not sure about whether moving the type specialization up would have
> concrete
> > impact. Once the interpolation result is reused, there is no allocation
during
> > animation (only during setup).
> 
> It would cut down on type casts and you could store the value's double, bool,
> etc in the Interpolator with fewer pointer chasing operations. Might or might
> not be a perf win.

You can't trivially store the full value type in the Interpolator as it's
hierarchical in the list case.
For example, box-shadow will end up interpolating as
[bool, [number, number, number, number], [number, number, number, number]].
The interpolable values mean we don't need different interpolation code for
every property.

[shans, 2014-11-10 23:04:42]

On 2014/11/10 22:10:45, alancutter wrote:
> On 2014/11/10 19:24:00, shans wrote:
> > On 2014/11/10 18:20:22, Stephen Chenney wrote:
> > > I was going with adding a result to the interpolation method, which has
the
> > same
> > > effect. I'm fine with this approach.
> > > 
> > > There's no way I can see this hurting performance. And it will definitely
> help
> > > Oilpan. No reason to delay as far as I am concerned.
> > > 
> > > Once this is done, I was thinking of moving the type specialization up to
> > > Interpolator, and get rid of InterpolableValue entirely. We'd then have
even
> > > less memory allocation per animation. It seems this might run into
problems
> > with
> > > the legacy methods.
> > 
> > Actually, I think adding a result to the interpolation method would be
cleaner
> > and easier to understand.
> > Do you want to prepare a patch that does that? Otherwise I'm happy to do it,
> or
> > I'm sure Alan would too.
> 
> Adding a result parameter to InterpolableValue::interpolate() seems like it
> would no longer take advantage of virtual method dispatch the way
> assignInterpolation() does currently. I'm not sure that would end up being any
> cleaner but I'm happy to be proven wrong.

Why not? It's just a matter of which value you're traversing. Currently the
start
value is traversed, your patch changes that to the output value. Either way,
virtual dispatch is used to decide which interpolation to call.

[shans, 2014-11-10 23:28:33]

On 2014/11/10 23:04:42, shans wrote:
> On 2014/11/10 22:10:45, alancutter wrote:
> > On 2014/11/10 19:24:00, shans wrote:
> > > On 2014/11/10 18:20:22, Stephen Chenney wrote:
> > > > I was going with adding a result to the interpolation method, which has
> the
> > > same
> > > > effect. I'm fine with this approach.
> > > > 
> > > > There's no way I can see this hurting performance. And it will
definitely
> > help
> > > > Oilpan. No reason to delay as far as I am concerned.
> > > > 
> > > > Once this is done, I was thinking of moving the type specialization up
to
> > > > Interpolator, and get rid of InterpolableValue entirely. We'd then have
> even
> > > > less memory allocation per animation. It seems this might run into
> problems
> > > with
> > > > the legacy methods.
> > > 
> > > Actually, I think adding a result to the interpolation method would be
> cleaner
> > > and easier to understand.
> > > Do you want to prepare a patch that does that? Otherwise I'm happy to do
it,
> > or
> > > I'm sure Alan would too.
> > 
> > Adding a result parameter to InterpolableValue::interpolate() seems like it
> > would no longer take advantage of virtual method dispatch the way
> > assignInterpolation() does currently. I'm not sure that would end up being
any
> > cleaner but I'm happy to be proven wrong.
> 
> Why not? It's just a matter of which value you're traversing. Currently the
> start
> value is traversed, your patch changes that to the output value. Either way,
> virtual dispatch is used to decide which interpolation to call.

See https://codereview.chromium.org/712143003