Oilpan: Partially ship Oilpan for core/animations

Oilpan: Partially ship Oilpan for core/animations

This CL ships Oilpan for core/animations except for temporary animation objects.
We confirmed that this CL won't regress any animation benchmark in telemetry.

Look at the discussion in the following thread for more details
and performance results: https://groups.google.com/a/chromium.org/d/topic/oilpan-reviews/V7d-7o4AbeA/discussion

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

[haraken, 2015-08-26 02:11:20]

haraken@chromium.org changed reviewers:
+ keishi@chromium.org, oilpan-reviews@chromium.org

[haraken, 2015-08-26 02:11:21]

keishi@: This is a CL to ship Oilpan for a part of core/animations/ objects (see
the following list). Feel free to use this CL to bisect the performance
regression.

AnimationEffect
EventDelegate
AnimationEffectTiming
AnimationTimeline
PlatformTiming
EffectModel
ElementAnimations
SampledEffect
RunningAnimation

[alancutter (OOO until 2018), 2015-09-01 08:00:41]

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

[alancutter (OOO until 2018), 2015-09-01 08:00:42]

lgtm though I would like to highlight the increase in size of ElementRareData
for anyone else following.

https://codereview.chromium.org/1318543009/diff/60001/Source/core/animation/A...
File Source/core/animation/Animation.h (right):

https://codereview.chromium.org/1318543009/diff/60001/Source/core/animation/A...
Source/core/animation/Animation.h:55: : public
RefCountedGarbageCollectedEventTargetWithInlineData<Animation>
Not sure why this class still needs to be RefCounted, should a TODO be added
here for something to change to make it not RefCountedGarbageCollected?

https://codereview.chromium.org/1318543009/diff/60001/Source/core/animation/C...
File Source/core/animation/CompositorAnimationsTest.cpp (left):

https://codereview.chromium.org/1318543009/diff/60001/Source/core/animation/C...
Source/core/animation/CompositorAnimationsTest.cpp:1219:
RefPtrWillBePersistent<Animation> animation1 =
m_timeline->play(keyframeEffect1.get());
Not sure why this was RefPtrWillBePersistent.

https://codereview.chromium.org/1318543009/diff/60001/Source/core/animation/S...
File Source/core/animation/SampledEffect.h (right):

https://codereview.chromium.org/1318543009/diff/60001/Source/core/animation/S...
Source/core/animation/SampledEffect.h:19: class SampledEffect : public
GarbageCollectedFinalized<SampledEffect> {
Not sure why this needs finalising.

[sof, 2015-09-01 08:13:06]

https://codereview.chromium.org/1318543009/diff/60001/Source/core/animation/S...
File Source/core/animation/SampledEffect.h (right):

https://codereview.chromium.org/1318543009/diff/60001/Source/core/animation/S...
Source/core/animation/SampledEffect.h:19: class SampledEffect : public
GarbageCollectedFinalized<SampledEffect> {
On 2015/09/01 08:00:42, alancutter wrote:
> Not sure why this needs finalising.

The OwnPtrWillBeMember<> field brings that about -- i.e., it won't need
finalization when Oilpan is always on.

(We've sometimes left behind a TODO to remind ourselves not to forget dropping
*Finalized upon fully shipping.)

[haraken, 2015-09-01 08:53:58]

Thanks for the careful review!

> I would like to highlight the increase in size of ElementRareData
for anyone else following.

Yeah, sizeof(Persistent) is 16 byte. The size increase will be gone once we ship
oilpan for everything (then we can use Member, which is 8 byte).

FWIW, as far as we measure Blink's memory usage in memory-infra, sizeof(Node) is
not a problem (which implies that sizeof(ElementRareData) is not a problem)
though.

https://codereview.chromium.org/1318543009/diff/60001/Source/core/animation/A...
File Source/core/animation/Animation.h (right):

https://codereview.chromium.org/1318543009/diff/60001/Source/core/animation/A...
Source/core/animation/Animation.h:55: : public
RefCountedGarbageCollectedEventTargetWithInlineData<Animation>
On 2015/09/01 08:00:42, alancutter wrote:
> Not sure why this class still needs to be RefCounted, should a TODO be added
> here for something to change to make it not RefCountedGarbageCollected?

RefCountedGarbageCollectedEventTarget is a class allocated on Oilpan's heap but
provides ref()/deref(). The ref()/deref() is needed because we haven't yet moved
EventTarget to Oilpan's heap. (To move EventTarget to the heap, we need to move
the Node hierarchy to the heap at the same time.)

Sorry about the complexity -- the complexity will be gone when we ship oilpan
for everything.

https://codereview.chromium.org/1318543009/diff/60001/Source/core/animation/C...
File Source/core/animation/CompositorAnimationsTest.cpp (left):

https://codereview.chromium.org/1318543009/diff/60001/Source/core/animation/C...
Source/core/animation/CompositorAnimationsTest.cpp:1219:
RefPtrWillBePersistent<Animation> animation1 =
m_timeline->play(keyframeEffect1.get());
On 2015/09/01 08:00:42, alancutter wrote:
> Not sure why this was RefPtrWillBePersistent.

I guess there was no reason.

https://codereview.chromium.org/1318543009/diff/60001/Source/core/animation/S...
File Source/core/animation/SampledEffect.h (right):

https://codereview.chromium.org/1318543009/diff/60001/Source/core/animation/S...
Source/core/animation/SampledEffect.h:19: class SampledEffect : public
GarbageCollectedFinalized<SampledEffect> {
On 2015/09/01 08:13:06, sof wrote:
> On 2015/09/01 08:00:42, alancutter wrote:
> > Not sure why this needs finalising.
> 
> The OwnPtrWillBeMember<> field brings that about -- i.e., it won't need
> finalization when Oilpan is always on.
> 
> (We've sometimes left behind a TODO to remind ourselves not to forget dropping
> *Finalized upon fully shipping.)

Added a TODO.

[haraken, 2015-09-01 08:55:51]

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

[haraken, 2015-09-01 08:55:51]

+tkent-san: Would you approve or object to the 8 byte increase to
sizeof(ElementRareData)? See comment #6 for more details.

[sof, 2015-09-01 11:43:22]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2015-09-01 11:43:22]

lgtm oilpan details.

Persistent<> is 16 bytes larger with ENABLE(ASSERT), so you need to adjust for
that wrt ElementRareData and Debug builds. But some of the release bots are
complaining about sizeof mismatch also - perhaps they've got asserts enabled
also(?)

https://codereview.chromium.org/1318543009/diff/80001/Source/core/animation/A...
File Source/core/animation/AnimationEffectTiming.h (right):

https://codereview.chromium.org/1318543009/diff/80001/Source/core/animation/A...
Source/core/animation/AnimationEffectTiming.h:18: class CORE_EXPORT
AnimationEffectTiming : public GarbageCollectedFinalized<AnimationEffectTiming>,
public ScriptWrappable {
nit: "Finalized" is not needed.

[tkent, 2015-09-01 22:57:08]

On 2015/09/01 08:55:51, haraken wrote:
> +tkent-san: Would you approve or object to the 8 byte increase to
> sizeof(ElementRareData)? See comment #6 for more details.

It's ok.  RareData is not so size-sensitive, and the increase is temporary.

[tkent, 2015-09-01 22:59:22]

Please check if we can remove #includes of RefPtr.h, PassRefPtr.h, and
RefCounted.h.

https://codereview.chromium.org/1318543009/diff/80001/Source/core/animation/S...
File Source/core/animation/SampledEffect.h (right):

https://codereview.chromium.org/1318543009/diff/80001/Source/core/animation/S...
Source/core/animation/SampledEffect.h:19: // TODO(haraken): Drop Finalized once
we ship Oilpan and the OwnPtrWillBeMember
We should introduce GarbageCollectedFinalizedWillBeGarbageCollected :0

[haraken, 2015-09-01 23:50:48]

On 2015/09/01 22:59:22, tkent wrote:
> Please check if we can remove #includes of RefPtr.h, PassRefPtr.h, and
> RefCounted.h.

Done.

[haraken, 2015-09-01 23:51:17]

The CQ bit was checked by haraken@chromium.org

[haraken, 2015-09-01 23:51:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
alancutter@chromium.org, sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/1318543009/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-09-01 23:51:27]

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

[commit-bot: I haz the power, 2015-09-02 01:04:29]

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

[commit-bot: I haz the power, 2015-09-02 01:04:30]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[haraken, 2015-09-02 01:14:04]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2015-09-02 01:14:14]

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

[commit-bot: I haz the power, 2015-09-02 01:54:29]

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

[commit-bot: I haz the power, 2015-09-02 01:54:30]

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

[haraken, 2015-09-02 04:50:04]

The CQ bit was checked by haraken@chromium.org

[haraken, 2015-09-02 04:50:05]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com,
alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/1318543009/#ps120001
(title: " ")

[commit-bot: I haz the power, 2015-09-02 04:50:12]

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

[commit-bot: I haz the power, 2015-09-02 09:20:19]

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

[commit-bot: I haz the power, 2015-09-02 09:20:21]

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

[haraken, 2015-09-02 09:20:53]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2015-09-02 09:21:05]

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

[commit-bot: I haz the power, 2015-09-02 10:34:33]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201633