Re: [Oilpan] Migrate most classes under core/animations to Oilpan heap. (issue 1120003002 by peria@chromium.org)

[pe...@chromium.org]

Maybe now it's ready for review.
Please take a look.

https://codereview.chromium.org/1120003002/

[har...@chromium.org]

On 2015/05/27 06:28:08, peria wrote:
> Maybe now it's ready for review.
> Please take a look.

This sounds great!

Can you get performance & memory numbers on mobile working with yuta-san?


https://codereview.chromium.org/1120003002/

[pe...@chromium.org]

Yuta-san,
Could you measure performance of following tests with and without applying
this
CL(PS 18) on Nexus 4&7?
- blink_perf.animation
- blink_perf.layout
- smoothness.tough_animation_case
- memory.top_7_stress
- memory.top_7_stress_slimming_paint


https://codereview.chromium.org/1120003002/

[har...@chromium.org]

On 2015/05/28 02:37:18, peria wrote:
> Yuta-san,
> Could you measure performance of following tests with and without applying
this
> CL(PS 18) on Nexus 4&7?
> - blink_perf.animation
> - blink_perf.layout
> - smoothness.tough_animation_case
> - memory.top_7_stress
> - memory.top_7_stress_slimming_paint

Will it be better to run those benchmarks in your Linux desktop and see the
performance data before investigating mobile in details?


https://codereview.chromium.org/1120003002/

[pe...@chromium.org]

Yes, I ran them for few times on my Linux.
Only smoothness.tough_animation_cases:responsive-biggest_jank_thread_time
seems to regress ~10%, but the data is unreliable.
Other tests seem to keep as-is or regress < 5%.



https://codereview.chromium.org/1120003002/

[sigb...@opera.com]

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/Animation.h
File Source/core/animation/Animation.h (right):

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/Animation.h#newcode58
Source/core/animation/Animation.h:58: USING_PRE_FINALIZER(Animation,
disposeAnimation);
No need to add a TODO, but eagerly finalizing Animation objects will be
an alternative.

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/AnimationStack.cpp
File Source/core/animation/AnimationStack.cpp (left):

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/AnimationStack.cpp#oldcode91
Source/core/animation/AnimationStack.cpp:91: // std::sort doesn't work
with OwnPtrs
nit: s/OwnPtr/Member/ (or remove the comment, doesn't add much.)

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/AnimationTimeline.cpp
File Source/core/animation/AnimationTimeline.cpp (right):

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/AnimationTimeline.cpp#newcode138
Source/core/animation/AnimationTimeline.cpp:138:
HeapVector<Member<Animation>> animations;
Use copyToVector() instead?

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/AnimationTimeline.h
File Source/core/animation/AnimationTimeline.h (right):

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/AnimationTimeline.h#newcode55
Source/core/animation/AnimationTimeline.h:55: class PlatformTiming :
public GarbageCollectedFinalized<PlatformTiming> {
Unusual abstract interface, it also deriving from a GC base class.

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/KeyframeEffect.h
File Source/core/animation/KeyframeEffect.h (right):

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/KeyframeEffect.h#newcode67
Source/core/animation/KeyframeEffect.h:67: const EffectModel* model()
const { return m_model.get(); }
nit: redundant get()

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/LengthBoxStyleInterpolation.cpp
File Source/core/animation/LengthBoxStyleInterpolation.cpp (right):

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/LengthBoxStyleInterpolation.cpp#newcode107
Source/core/animation/LengthBoxStyleInterpolation.cpp:107:
StyleInterpolation::trace(visitor);
nit: move this to the end?

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/SampledEffect.h
File Source/core/animation/SampledEffect.h (right):

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/SampledEffect.h#newcode29
Source/core/animation/SampledEffect.h:29:
HeapVector<Member<Interpolation>>* mutableInterpolations() { return
m_interpolations.get(); }
get() needed?

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/StringKeyframe.cpp
File Source/core/animation/StringKeyframe.cpp (right):

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/StringKeyframe.cpp#newcode277
Source/core/animation/StringKeyframe.cpp:277: break;
Could you fix indentation of this "break"?

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/animatable/AnimatableLengthPoint.h
File Source/core/animation/animatable/AnimatableLengthPoint.h (right):

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/animatable/AnimatableLengthPoint.h#newcode45
Source/core/animation/animatable/AnimatableLengthPoint.h:45: const
AnimatableValue* x() const { return m_x.get(); }
redundant get()s.

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/animatable/AnimatableLengthPoint3D.h
File Source/core/animation/animatable/AnimatableLengthPoint3D.h (right):

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/animatable/AnimatableLengthPoint3D.h#newcode45
Source/core/animation/animatable/AnimatableLengthPoint3D.h:45: const
AnimatableValue* x() const { return m_x.get(); }
yet more redundant get()s

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/animatable/AnimatableLengthSize.h
File Source/core/animation/animatable/AnimatableLengthSize.h (right):

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/animatable/AnimatableLengthSize.h#newcode45
Source/core/animation/animatable/AnimatableLengthSize.h:45: const
AnimatableValue* width() const { return m_width.get(); }
get() redundant - could you take a pass through the header files touched
here and remove them where possible?

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

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/css/CSSAnimations.h#newcode82
Source/core/animation/css/CSSAnimations.h:82: ~RunningAnimation() { }
Define it out-of-line?

https://codereview.chromium.org/1120003002/

[pe...@chromium.org]

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/Animation.h
File Source/core/animation/Animation.h (right):

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/Animation.h#newcode58
Source/core/animation/Animation.h:58: USING_PRE_FINALIZER(Animation,
disposeAnimation);

On 2015/05/30 11:34:04, sof wrote:
> No need to add a TODO, but eagerly finalizing Animation objects will
be an
> alternative.

Done.

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/AnimationStack.cpp
File Source/core/animation/AnimationStack.cpp (left):

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/AnimationStack.cpp#oldcode91
Source/core/animation/AnimationStack.cpp:91: // std::sort doesn't work
with OwnPtrs

On 2015/05/30 11:34:04, sof wrote:
> nit: s/OwnPtr/Member/ (or remove the comment, doesn't add much.)

Done.

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/AnimationTimeline.cpp
File Source/core/animation/AnimationTimeline.cpp (right):

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/AnimationTimeline.cpp#newcode138
Source/core/animation/AnimationTimeline.cpp:138:
HeapVector<Member<Animation>> animations;

On 2015/05/30 11:34:04, sof wrote:
> Use copyToVector() instead?

Done.

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/KeyframeEffect.h
File Source/core/animation/KeyframeEffect.h (right):

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/KeyframeEffect.h#newcode67
Source/core/animation/KeyframeEffect.h:67: const EffectModel* model()
const { return m_model.get(); }

On 2015/05/30 11:34:04, sof wrote:
> nit: redundant get()

Done.

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/LengthBoxStyleInterpolation.cpp
File Source/core/animation/LengthBoxStyleInterpolation.cpp (right):

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/LengthBoxStyleInterpolation.cpp#newcode107
Source/core/animation/LengthBoxStyleInterpolation.cpp:107:
StyleInterpolation::trace(visitor);

On 2015/05/30 11:34:04, sof wrote:
> nit: move this to the end?

Done.

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/SampledEffect.h
File Source/core/animation/SampledEffect.h (right):

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/SampledEffect.h#newcode29
Source/core/animation/SampledEffect.h:29:
HeapVector<Member<Interpolation>>* mutableInterpolations() { return
m_interpolations.get(); }

On 2015/05/30 11:34:05, sof wrote:
> get() needed?

Done.

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/SampledEffect.h#newcode29
Source/core/animation/SampledEffect.h:29:
HeapVector<Member<Interpolation>>* mutableInterpolations() { return
m_interpolations.get(); }

On 2015/05/30 11:34:05, sof wrote:
> get() needed?

not needed.

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/StringKeyframe.cpp
File Source/core/animation/StringKeyframe.cpp (right):

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/StringKeyframe.cpp#newcode277
Source/core/animation/StringKeyframe.cpp:277: break;

On 2015/05/30 11:34:05, sof wrote:
> Could you fix indentation of this "break"?

Thank you for catching up this mistake!

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/animatable/AnimatableLengthPoint.h
File Source/core/animation/animatable/AnimatableLengthPoint.h (right):

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/animatable/AnimatableLengthPoint.h#newcode45
Source/core/animation/animatable/AnimatableLengthPoint.h:45: const
AnimatableValue* x() const { return m_x.get(); }

On 2015/05/30 11:34:05, sof wrote:
> redundant get()s.

Done.

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/animatable/AnimatableLengthPoint3D.h
File Source/core/animation/animatable/AnimatableLengthPoint3D.h (right):

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/animatable/AnimatableLengthPoint3D.h#newcode45
Source/core/animation/animatable/AnimatableLengthPoint3D.h:45: const
AnimatableValue* x() const { return m_x.get(); }

On 2015/05/30 11:34:05, sof wrote:
> yet more redundant get()s

Done.

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/animatable/AnimatableLengthSize.h
File Source/core/animation/animatable/AnimatableLengthSize.h (right):

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/animatable/AnimatableLengthSize.h#newcode45
Source/core/animation/animatable/AnimatableLengthSize.h:45: const
AnimatableValue* width() const { return m_width.get(); }

On 2015/05/30 11:34:05, sof wrote:
> get() redundant - could you take a pass through the header files
touched here
> and remove them where possible?

Done.

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

https://codereview.chromium.org/1120003002/diff/320001/Source/core/animation/css/CSSAnimations.h#newcode82
Source/core/animation/css/CSSAnimations.h:82: ~RunningAnimation() { }

On 2015/05/30 11:34:05, sof wrote:
> Define it out-of-line?

Done.

https://codereview.chromium.org/1120003002/

[pe...@chromium.org]

Yuta-san,
Could you concatenate the result of my experiences on Linux?
https://drive.google.com/a/chromium.org/file/d/0B4QUVw-AB8wPZTBqbmlCdXlpbG8/view?usp=sharing

thank you in advance.

https://codereview.chromium.org/1120003002/

[yu...@chromium.org]

Sorry for taking long to get back to you,

(I had to deal with some issue with my tool to be able to
handle null values in the results...)

Here's the results:
https://www.googledrive.com/host/0B6XhrzUCq5lhUjloNWR3b0xxRXM/2015/moveanim.html

I've only skimmed the results, but consistent regressions in
queueing durations seemed worrisome to me.

https://codereview.chromium.org/1120003002/

[pe...@chromium.org]

On 2015/06/02 05:51:44, Yuta Kitamura wrote:
> Sorry for taking long to get back to you,

> (I had to deal with some issue with my tool to be able to
> handle null values in the results...)

> Here's the results:

https://www.googledrive.com/host/0B6XhrzUCq5lhUjloNWR3b0xxRXM/2015/moveanim.html

> I've only skimmed the results, but consistent regressions in
> queueing durations seemed worrisome to me.

As we discussed offline, we have to measure performance again with
enabling lazy sweep and idle GC.
Here is my experiments' result. Could you concatenate it again?
https://drive.google.com/file/d/0B4QUVw-AB8wPZFYtWVBVZGdLTVk/view?usp=sharing

https://codereview.chromium.org/1120003002/

[yu...@chromium.org]

Here's the revised results, along with your data:
https://www.googledrive.com/host/0B6XhrzUCq5lhUjloNWR3b0xxRXM/2015/moveanim2.html

https://codereview.chromium.org/1120003002/

[pe...@chromium.org]

On 2015/06/05 05:21:49, Yuta Kitamura wrote:
> Here's the revised results, along with your data:

https://www.googledrive.com/host/0B6XhrzUCq5lhUjloNWR3b0xxRXM/2015/moveanim2.html

Thank you very much for the cooperation.

It's interesting that a new metric, which means it was not included in past
report,
"responsive-total_big_jank_thread_time" regresses largely in many pages,
especially on Nexus 7.

https://codereview.chromium.org/1120003002/

[har...@chromium.org]

Thanks for the results!

- Would you summarize the results?
- Would you start a thread in oilpan-reviews including Ross and Sami and ask
what the responsive-total_big_jank_thread_time is and how we should take
care of
the metric?



https://codereview.chromium.org/1120003002/

[pe...@chromium.org]

On 2015/06/08 02:15:06, haraken wrote:

> Thanks for the results!

> - Would you summarize the results?

I'm writing a doc.
Will share it soon.

> - Would you start a thread in oilpan-reviews including Ross and Sami and
> ask
> what the responsive-total_big_jank_thread_time is and how we should take
> care
of
> the metric?

will do.
JFYI, the metric has an explanation in code.
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/telemetry/web_perf/metrics/mainthread_jank_stats.py&q=total_big_jank_thread_time&sq=package:chromium&type=cs&l=58

https://codereview.chromium.org/1120003002/