Skip rule matching during animations.

Skip rule matching during animations.

If a style change is due to animations updating their interpolated values
only, store a clone of the RenderStyle before the animated properties are
applied to use as a base for updating RenderStyle for the next frames.

The base RenderStyle is stored on the ActiveAnimations object and cleared
when a recalc is triggered by changes that require rule matching.

The machinery for detecting if a LocalStyleChange is caused by an animation
update only was already in place, so this CL is making use of that.

Running blink_perf.animation shows a frame rate increase of 18-40% with
the exception of three tests which are unchanged.

In general this seems to shave off nearly 50% of the time used in
recalcStyle for each animation frame if no inherited properties are
animated. For instance, in "The expansion of the dictionary definition card"
from issue 377939.

R=dstockwell@chromium.org,esprehn@chromium.org
BUG=377939, 421822
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183662

[rune, 2014-10-09 13:22:23]

I've been playing around with the polymer paper element demos, and the recalc
style time seems to have gone down for animated effects. I don't have any stable
numbers, though.

[dstockwell, 2014-10-10 01:01:05]

This is really exciting!

Would it be possible to assert that the base style is correct? eg. If
ENABLE(ASSERT), still do the regular matching and just check at the end? This
would be similar to how we validate the existing optimizations using the
animation style change flag.

If I remember correctly, esprehn had some concerns that there was potential for
accidental misuse of the animation style change flag, which is why it was not
previously exposed.

[rune, 2014-10-10 13:08:35]

On 2014/10/10 at 01:01:05, dstockwell wrote:
> This is really exciting!
> 
> Would it be possible to assert that the base style is correct? eg. If
ENABLE(ASSERT), still do the regular matching and just check at the end? This
would be similar to how we validate the existing optimizations using the
animation style change flag.
> 
> If I remember correctly, esprehn had some concerns that there was potential
for accidental misuse of the animation style change flag, which is why it was
not previously exposed.

I've added an ASSERT. I've confirmed that it caused crashes in debug layout
tests when the condition was flipped. Running linux_blink_dbg with the correct
condition now.

[rune, 2014-10-10 19:28:26]

On 2014/10/10 at 13:08:35, rune wrote:
> On 2014/10/10 at 01:01:05, dstockwell wrote:
> > This is really exciting!
> > 
> > Would it be possible to assert that the base style is correct? eg. If
ENABLE(ASSERT), still do the regular matching and just check at the end? This
would be similar to how we validate the existing optimizations using the
animation style change flag.
> > 
> > If I remember correctly, esprehn had some concerns that there was potential
for accidental misuse of the animation style change flag, which is why it was
not previously exposed.
> 
> I've added an ASSERT. I've confirmed that it caused crashes in debug layout
tests when the condition was flipped. Running linux_blink_dbg with the correct
condition now.

No layout tests asserting in linux_blink_dbg, it seems.

[rune, 2014-10-13 09:45:39]

@esprehn PTAL

[esprehn, 2014-10-13 18:19:33]

This seems okay, I do wonder if we should finally bite the bullet and steal
another bit in StyleChangeType for AnimationStyleChange.

https://codereview.chromium.org/635203003/diff/50001/Source/core/animation/Ac...
File Source/core/animation/ActiveAnimations.h (right):

https://codereview.chromium.org/635203003/diff/50001/Source/core/animation/Ac...
Source/core/animation/ActiveAnimations.h:82: void
setBaseRenderStyle(PassRefPtr<RenderStyle> renderStyle)
Can we make this out of line instead? Then we don't need to include
RenderStyle.h in this and all over more files.

https://codereview.chromium.org/635203003/diff/50001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/635203003/diff/50001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:546: struct AnimationResolveData {
I don't think you need this class at all, just add a static function like
animationStartStyle(element) and then null check the result.

https://codereview.chromium.org/635203003/diff/50001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:550: , useBaseRenderStyle(false)
The useBaseRenderStyle bit doesn't seem needed, just null check the base render
style.

[rune, 2014-10-13 21:31:48]

https://codereview.chromium.org/635203003/diff/50001/Source/core/animation/Ac...
File Source/core/animation/ActiveAnimations.h (right):

https://codereview.chromium.org/635203003/diff/50001/Source/core/animation/Ac...
Source/core/animation/ActiveAnimations.h:82: void
setBaseRenderStyle(PassRefPtr<RenderStyle> renderStyle)
On 2014/10/13 at 18:19:33, esprehn wrote:
> Can we make this out of line instead? Then we don't need to include
RenderStyle.h in this and all over more files.

Done.

https://codereview.chromium.org/635203003/diff/50001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/635203003/diff/50001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:546: struct AnimationResolveData {
On 2014/10/13 at 18:19:33, esprehn wrote:
> I don't think you need this class at all, just add a static function like
animationStartStyle(element) and then null check the result.

I need to know if I should store a new baseRenderStyle if it was null, too

https://codereview.chromium.org/635203003/diff/50001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:550: , useBaseRenderStyle(false)
On 2014/10/13 at 18:19:33, esprehn wrote:
> The useBaseRenderStyle bit doesn't seem needed, just null check the base
render style.

I don't want to store two RenderStyles unless I'm animating with
isAnimationStyleChange(). So, I need to know the value of
isAnimationStyleChange(), and the baseRenderStyle if it was previously stored.
The reason I use a struct is to share the code between styleForElement and
pseudoStyleForElementInternal(). It's also a way to hide the ASSERT. I could've
used two methods with two variables instead.

[esprehn, 2014-10-13 21:44:26]

I don't like that constructing a Data object has side effects on the element you
pass in. It should take a const Element, side effects should be in explicit
methods.

https://codereview.chromium.org/635203003/diff/140001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/635203003/diff/140001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:560:
animations->setBaseRenderStyle(nullptr);
This really doesn't seem like the right place to have side effects.

[rune, 2014-10-13 22:24:49]

https://codereview.chromium.org/635203003/diff/140001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/635203003/diff/140001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:560:
animations->setBaseRenderStyle(nullptr);
On 2014/10/13 at 21:44:26, esprehn wrote:
> This really doesn't seem like the right place to have side effects.

There are a bunch of ways to do this. Do you think ActiveAnimations itself or
StyleResolver should be responsible for clearing the baseRenderStyle pointer?

We could put the logic inside ActiveAnimations and even make
isAnimationsStyleChange() private again:

RenderStyle* ActiveAnimations::baseRenderStyle() const
{
    if (isAnimationStyleChange())
        return m_baseRenderStyle;
    
    return nullptr;
}

void ActiveAnimations::updateBaseRenderStyle(RenderStyle*)
{
    if (isAnimationsStyleChange())
        m_baseRenderStyle = RenderStyle::clone(...);
    else
        m_baseRenderStyle = nullptr;
}


WDYT?

[esprehn, 2014-10-13 22:32:32]

On 2014/10/13 at 22:24:49, rune wrote:
>
https://codereview.chromium.org/635203003/diff/140001/Source/core/css/resolve...
> File Source/core/css/resolver/StyleResolver.cpp (right):
> 
>
https://codereview.chromium.org/635203003/diff/140001/Source/core/css/resolve...
> Source/core/css/resolver/StyleResolver.cpp:560:
animations->setBaseRenderStyle(nullptr);
> On 2014/10/13 at 21:44:26, esprehn wrote:
> > This really doesn't seem like the right place to have side effects.
> 
> There are a bunch of ways to do this. Do you think ActiveAnimations itself or
StyleResolver should be responsible for clearing the baseRenderStyle pointer?
> 
> We could put the logic inside ActiveAnimations and even make
isAnimationsStyleChange() private again:
> 
> RenderStyle* ActiveAnimations::baseRenderStyle() const
> {
>     if (isAnimationStyleChange())
>         return m_baseRenderStyle;
>     
>     return nullptr;
> }
> 
> void ActiveAnimations::updateBaseRenderStyle(RenderStyle*)
> {
>     if (isAnimationsStyleChange())
>         m_baseRenderStyle = RenderStyle::clone(...);
>     else
>         m_baseRenderStyle = nullptr;
> }
> 
> 
> WDYT?

updateBaseRenderStyle seems better to me, but whatever makes the code simpler I
suppose.

[rune, 2014-10-13 23:03:07]

On 2014/10/13 at 22:32:32, esprehn wrote:

> updateBaseRenderStyle seems better to me, but whatever makes the code simpler
I suppose.

New patch set up.

[esprehn, 2014-10-13 23:51:35]

lgtm

[dstockwell, 2014-10-14 00:26:29]

lgtm

[rune, 2014-10-14 06:44:23]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-10-14 06:44:49]

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

[commit-bot: I haz the power, 2014-10-14 08:18:09]

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

[commit-bot: I haz the power, 2014-10-14 08:18:15]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/31647)

[rune, 2014-10-14 09:35:22]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-10-14 09:36:15]

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

[commit-bot: I haz the power, 2014-10-14 10:13:25]

Committed patchset #5 (id:210001) as 183662