Make CSSAnimationUpdate stack-allocated

Make CSSAnimationUpdate stack-allocated

This is a CL to fix (a part of) the regressions observed in tough_animation_cases
benchmarks on oilpan builds. In short, this CL fixes the heavy allocation design
of CSSAnimationUpdate.

Currently, a lot of CSSAnimationUpdate objects are allocated on Oilpan's heap.
This is problematic because the CSSAnimationUpdate objects hold HeapVectors and HeapHashMaps,
which produce a lot of unnecessary garbage in Oilpan's heap.

This CL makes CSSAnimationUpdate stack-allocated and explicitly clears the HeapVectors and HeapHashMaps
when the CSSAnimationUpdate is destructed on stack. When we need to hold a pending CSSAnimationUpdate
on CSSAnimation, it is held as a part of object of CSSAnimation. That way we can avoid
allocating unecessary HeapVectors, HeapHashMaps etc every time a pending CSSAnimationUpdate is created.

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

[haraken, 2015-08-11 11:48:42]

haraken@chromium.org changed reviewers:
+ alancutter@chromium.org, oilpan-reviews@chromium.org, shanestephens@google.com

[haraken, 2015-08-11 11:48:49]

PTAL

[haraken, 2015-08-13 04:02:13]

alan@: Would you mind taking a look at this when you have a cycle?

[haraken, 2015-08-13 12:35:24]

haraken@chromium.org changed reviewers:
+ timloh@chromium.org

[haraken, 2015-08-13 12:35:54]

timloh@: It seems Alan is not around. Would you mind taking a look at this?

[alancutter (OOO until 2018), 2015-08-19 02:46:31]

Sorry about the delay, the Sydney office was OOO last week.

https://codereview.chromium.org/1281493004/diff/40001/Source/core/animation/c...
File Source/core/animation/css/CSSAnimationUpdate.h (right):

https://codereview.chromium.org/1281493004/diff/40001/Source/core/animation/c...
Source/core/animation/css/CSSAnimationUpdate.h:272: DISALLOW_ALLOCATION();
Can we make CSSAnimationUpdate DISALLOW_ALLOCATION instead of STACK_ALLOCATED
and avoid creating a new PendingAnimationUpdate class?

https://codereview.chromium.org/1281493004/diff/40001/Source/core/animation/c...
Source/core/animation/css/CSSAnimationUpdate.h:312: #if ENABLE(OILPAN)
Is it necessary to #if here?

https://codereview.chromium.org/1281493004/diff/40001/Source/core/animation/c...
File Source/core/animation/css/CSSAnimations.cpp (right):

https://codereview.chromium.org/1281493004/diff/40001/Source/core/animation/c...
Source/core/animation/css/CSSAnimations.cpp:203: void
CSSAnimations::calculateUpdate(const Element* animatingElement, Element&
element, StyleResolverState& state, StyleResolver* resolver)
We should avoid passing StyleResolverState around outside of the StyleResolver
and instead pass animationUpdate() in as a parameter.

[haraken, 2015-08-19 05:46:08]

Thanks for review!

https://codereview.chromium.org/1281493004/diff/40001/Source/core/animation/c...
File Source/core/animation/css/CSSAnimationUpdate.h (right):

https://codereview.chromium.org/1281493004/diff/40001/Source/core/animation/c...
Source/core/animation/css/CSSAnimationUpdate.h:272: DISALLOW_ALLOCATION();
On 2015/08/19 02:46:30, alancutter wrote:
> Can we make CSSAnimationUpdate DISALLOW_ALLOCATION instead of STACK_ALLOCATED
> and avoid creating a new PendingAnimationUpdate class?

That is a great suggestion :) Done.

https://codereview.chromium.org/1281493004/diff/40001/Source/core/animation/c...
Source/core/animation/css/CSSAnimationUpdate.h:312: #if ENABLE(OILPAN)
On 2015/08/19 02:46:30, alancutter wrote:
> Is it necessary to #if here?

Unfortunately yes. The current oilpan cannot handle vistor->trace(complicated
collections) in non-oilpan mode.

https://codereview.chromium.org/1281493004/diff/40001/Source/core/animation/c...
File Source/core/animation/css/CSSAnimations.cpp (right):

https://codereview.chromium.org/1281493004/diff/40001/Source/core/animation/c...
Source/core/animation/css/CSSAnimations.cpp:203: void
CSSAnimations::calculateUpdate(const Element* animatingElement, Element&
element, StyleResolverState& state, StyleResolver* resolver)
On 2015/08/19 02:46:30, alancutter wrote:
> We should avoid passing StyleResolverState around outside of the StyleResolver
> and instead pass animationUpdate() in as a parameter.

Done.

[alancutter (OOO until 2018), 2015-08-19 07:26:43]

https://codereview.chromium.org/1281493004/diff/80001/Source/core/animation/c...
File Source/core/animation/css/CSSAnimationUpdate.h (right):

https://codereview.chromium.org/1281493004/diff/80001/Source/core/animation/c...
Source/core/animation/css/CSSAnimationUpdate.h:148: void setPendingUpdate(const
CSSAnimationUpdate& update)
s/setPendingUpdate/copy/ ?

https://codereview.chromium.org/1281493004/diff/80001/Source/core/animation/c...
Source/core/animation/css/CSSAnimationUpdate.h:179: bool isEmpty() const {
return m_isEmpty; }
This function should remain as it was otherwise the logic for whether we skip
applying animation updates will change. The way it's written in this patch
really means isPendingUpdate() which doesn't represent whether any member
vectors are populated.

https://codereview.chromium.org/1281493004/diff/80001/Source/core/animation/c...
Source/core/animation/css/CSSAnimationUpdate.h:287: friend class
PendingAnimationUpdate;
No need for friend class.

[haraken, 2015-08-19 12:08:11]

https://codereview.chromium.org/1281493004/diff/80001/Source/core/animation/c...
File Source/core/animation/css/CSSAnimationUpdate.h (right):

https://codereview.chromium.org/1281493004/diff/80001/Source/core/animation/c...
Source/core/animation/css/CSSAnimationUpdate.h:148: void setPendingUpdate(const
CSSAnimationUpdate& update)
On 2015/08/19 07:26:43, alancutter wrote:
> s/setPendingUpdate/copy/ ?

Done.

https://codereview.chromium.org/1281493004/diff/80001/Source/core/animation/c...
Source/core/animation/css/CSSAnimationUpdate.h:179: bool isEmpty() const {
return m_isEmpty; }
On 2015/08/19 07:26:43, alancutter wrote:
> This function should remain as it was otherwise the logic for whether we skip
> applying animation updates will change. The way it's written in this patch
> really means isPendingUpdate() which doesn't represent whether any member
> vectors are populated.

Done.

https://codereview.chromium.org/1281493004/diff/80001/Source/core/animation/c...
Source/core/animation/css/CSSAnimationUpdate.h:287: friend class
PendingAnimationUpdate;
On 2015/08/19 07:26:43, alancutter wrote:
> No need for friend class.

Done.

[alancutter (OOO until 2018), 2015-08-20 02:06:58]

lgtm with nits.

https://codereview.chromium.org/1281493004/diff/120001/Source/core/animation/...
File Source/core/animation/css/CSSAnimationUpdate.h (right):

https://codereview.chromium.org/1281493004/diff/120001/Source/core/animation/...
Source/core/animation/css/CSSAnimationUpdate.h:157: #endif
I'm not sure it's worthwhile adding 4 ifdefs for a relatively minor assertion. I
think it's fine to just assert on isEmpty() instead.

https://codereview.chromium.org/1281493004/diff/120001/Source/core/animation/...
File Source/core/animation/css/CSSAnimations.h (right):

https://codereview.chromium.org/1281493004/diff/120001/Source/core/animation/...
Source/core/animation/css/CSSAnimations.h:66: void copy(const
CSSAnimationUpdate& update)
I'd still keep this method named setPendingUpdate().

[haraken, 2015-08-20 03:01:25]

Thanks for review!

https://codereview.chromium.org/1281493004/diff/120001/Source/core/animation/...
File Source/core/animation/css/CSSAnimationUpdate.h (right):

https://codereview.chromium.org/1281493004/diff/120001/Source/core/animation/...
Source/core/animation/css/CSSAnimationUpdate.h:157: #endif
On 2015/08/20 02:06:58, alancutter wrote:
> I'm not sure it's worthwhile adding 4 ifdefs for a relatively minor assertion.
I
> think it's fine to just assert on isEmpty() instead.

Done.

https://codereview.chromium.org/1281493004/diff/120001/Source/core/animation/...
File Source/core/animation/css/CSSAnimations.h (right):

https://codereview.chromium.org/1281493004/diff/120001/Source/core/animation/...
Source/core/animation/css/CSSAnimations.h:66: void copy(const
CSSAnimationUpdate& update)
On 2015/08/20 02:06:58, alancutter wrote:
> I'd still keep this method named setPendingUpdate().

Done.

[haraken, 2015-08-20 03:02:01]

The CQ bit was checked by haraken@chromium.org

[haraken, 2015-08-20 03:02:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/1281493004/#ps140001
(title: " ")

[commit-bot: I haz the power, 2015-08-20 03:02:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1281493004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1281493004/140001

[alancutter (OOO until 2018), 2015-08-20 03:05:50]

Thanks for working on our animation performance! I'm curious if you have any
perf numbers that led to making this change.

[haraken, 2015-08-20 03:17:02]

On 2015/08/20 03:05:50, alancutter wrote:
> Thanks for working on our animation performance! I'm curious if you have any
> perf numbers that led to making this change.

Yes, we're (hopefully) in the final phase of the performance fix for oilpan +
core/animations/. We'll share the result once it's ready :)

[commit-bot: I haz the power, 2015-08-20 03:33:06]

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

[commit-bot: I haz the power, 2015-08-20 03:33:07]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[haraken, 2015-08-20 08:01:40]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2015-08-20 08:01:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1281493004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1281493004/140001

[commit-bot: I haz the power, 2015-08-20 08:24:29]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200885