Oilpan: Unship oilpan from temporary animation objects

Oilpan: Unship oilpan from temporary animation objects

We decided to unship oilpan from temporary animation objects
(i.e., AnimatableValues, InterpoableValues, Interpolations and its associated objects) for the following reasons:

- The balls_css_properties_* benchmarks allocate an incredible number of AnimatableValues & InterpolableValues.
If we put those temporary objects on Oilpan's heap, they add a significant pressure on the heap.

- There is no advantage in putting those temporary objects on Oilpan's heap because their lifetime is very clear.

- I have tried a bunch of approaches to resolve the regression but concluded that we won't be able to resolve the regression
without introducing an incremental marking or/and a generational GC to Oilpan. I don't think that
the engineering cost and the complexity outweighs the benefit. At the very least, it won't be a task of Oilpan v1.

c.f., we already shipped oilpan for other animation objects (https://codereview.chromium.org/1318543009/).

For more details, see this thread: 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=201698

[haraken, 2015-08-10 03:51:32]

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

[haraken, 2015-08-10 03:51:32]

Not for review.

I'm investigating which object(s) are keys to fix the regression of
tough_animation_cases in oilpan.

[haraken, 2015-08-10 08:10:08]

In the patch set 3, I unshipped oilpan from AnimatableValue, InterpolableValue,
InterpolationValue, PrimitiveInterpolation, Keyframe and
PropertySpecificKeyframe. I think I'm unshipping oilpan from too many things,
but before optimizing the unship list, I want to find a point where the
regression observed in frame_times is completely gone.

Without the patch set 3, each pause time in
tough_animation_cases.css_properties_40 is 13 ms. With the patch set 2, each
pause time is 8 ms. This is a great improvement but 8 ms still sounds a bit too
large. I need to identify what objects are causing the 8 ms pause.

[haraken, 2015-08-13 12:12:31]

On 2015/08/10 08:10:08, haraken wrote:
> In the patch set 3, I unshipped oilpan from AnimatableValue,
InterpolableValue,
> InterpolationValue, PrimitiveInterpolation, Keyframe and
> PropertySpecificKeyframe. I think I'm unshipping oilpan from too many things,
> but before optimizing the unship list, I want to find a point where the
> regression observed in frame_times is completely gone.
> 
> Without the patch set 3, each pause time in
> tough_animation_cases.css_properties_40 is 13 ms. With the patch set 2, each
> pause time is 8 ms. This is a great improvement but 8 ms still sounds a bit
too
> large. I need to identify what objects are causing the 8 ms pause.

The patch set made CSSAnimationUpdate STACK_ALLOCATED and PendingAnimationUpdate
DISALLOW_ALLOCATION. This dramatically reduces the number of objects we have to
allocate on the heap.

(The CL is not intending for review.)

[haraken, 2015-08-24 06:21:04]

Nit: The patch set 10 is rebased with ToT.

[haraken, 2015-09-02 12:05:30]

haraken@chromium.org changed reviewers:
+ sigbjornf@opera.com

[haraken, 2015-09-02 12:05:32]

Sigbjorn: Would you mind taking a look at this CL before asking animation
experts for formal review?

[sof, 2015-09-02 13:02:12]

It looks alright wrt Oilpan details, but do make sure that enable_oilpan=1
doesn't break due to these type changes.

The changes saddens me a bit though (but I'll get over it.) I hope none of the
added Persistent<>s introduce a leak (can't readily see a problem due to those
though.)

https://codereview.chromium.org/1276183004/diff/340001/Source/core/animation/...
File Source/core/animation/AnimationStack.cpp (right):

https://codereview.chromium.org/1276183004/diff/340001/Source/core/animation/...
Source/core/animation/AnimationStack.cpp:61:
OwnPtr<Vector<RefPtr<Interpolation>>> sample = nullptr;
(it's fine to leave behind the explicit initialization here, imho.)

https://codereview.chromium.org/1276183004/diff/340001/Source/core/animation/...
File Source/core/animation/KeyframeEffectModel.cpp (right):

https://codereview.chromium.org/1276183004/diff/340001/Source/core/animation/...
Source/core/animation/KeyframeEffectModel.cpp:227:
DEFINE_TRACE(KeyframeEffectModelBase)
Can just elide this one?

https://codereview.chromium.org/1276183004/diff/340001/Source/core/animation/...
File Source/core/animation/KeyframeEffectModel.h (right):

https://codereview.chromium.org/1276183004/diff/340001/Source/core/animation/...
Source/core/animation/KeyframeEffectModel.h:138: using KeyframeGroupMap =
WillBeHeapHashMap<PropertyHandle,
OwnPtrWillBeMember<PropertySpecificKeyframeGroup>>;
The value type doesn't look correct now. OwnPtr<> or just a part object?

https://codereview.chromium.org/1276183004/diff/340001/Source/core/animation/...
File Source/core/animation/KeyframeEffectModelTest.cpp (right):

https://codereview.chromium.org/1276183004/diff/340001/Source/core/animation/...
Source/core/animation/KeyframeEffectModelTest.cpp:103:
OwnPtr<Vector<RefPtr<Interpolation>>> values = nullptr;
Not due to your CL, but this type might deserve an alias/typedef.

https://codereview.chromium.org/1276183004/diff/340001/Source/core/animation/...
File Source/core/animation/css/CSSAnimationUpdate.h (left):

https://codereview.chromium.org/1276183004/diff/340001/Source/core/animation/...
Source/core/animation/css/CSSAnimationUpdate.h:98: DISALLOW_ALLOCATION();
Won't do any harm to keep?

https://codereview.chromium.org/1276183004/diff/340001/Source/platform/heap/T...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1276183004/diff/340001/Source/platform/heap/T...
Source/platform/heap/ThreadState.cpp:673: return judgeGCThreshold(32 * 1024 *
1024, 1.2);
Change doesn't belong here (and not something you want to land)?

https://codereview.chromium.org/1276183004/diff/340001/Source/platform/heap/T...
Source/platform/heap/ThreadState.cpp:929: Heap::reportMemoryUsageForTracing();
Unrelated also?

[haraken, 2015-09-03 00:12:55]

Thanks for review! Addressed the comments.

https://codereview.chromium.org/1276183004/diff/340001/Source/core/animation/...
File Source/core/animation/KeyframeEffectModelTest.cpp (right):

https://codereview.chromium.org/1276183004/diff/340001/Source/core/animation/...
Source/core/animation/KeyframeEffectModelTest.cpp:103:
OwnPtr<Vector<RefPtr<Interpolation>>> values = nullptr;
On 2015/09/02 13:02:12, sof wrote:
> Not due to your CL, but this type might deserve an alias/typedef.

Will fix in a follow-up.

[haraken, 2015-09-03 00:13:42]

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

[haraken, 2015-09-03 00:13:43]

Alan: This CL is ready from the oilpan perspective. Would you mind taking a look
when you have a cycle?

[alancutter (OOO until 2018), 2015-09-03 02:28:10]

lgtm for everything outside AnimatableFilterOperations.

https://codereview.chromium.org/1276183004/diff/360001/Source/core/animation/...
File Source/core/animation/animatable/AnimatableFilterOperations.h (right):

https://codereview.chromium.org/1276183004/diff/360001/Source/core/animation/...
Source/core/animation/animatable/AnimatableFilterOperations.h:75:
Persistent<FilterOperationsWrapper> m_operationWrapper;
Why do we need to wrap the non Oilpanned FilterOperations in an Oilpanned object
that gets pointed to by a non Oilpanned object?

[haraken, 2015-09-03 02:40:12]

Thanks for reviewing the huge CL!

https://codereview.chromium.org/1276183004/diff/360001/Source/core/animation/...
File Source/core/animation/animatable/AnimatableFilterOperations.h (right):

https://codereview.chromium.org/1276183004/diff/360001/Source/core/animation/...
Source/core/animation/animatable/AnimatableFilterOperations.h:75:
Persistent<FilterOperationsWrapper> m_operationWrapper;
On 2015/09/03 02:28:09, alancutter wrote:
> Why do we need to wrap the non Oilpanned FilterOperations in an Oilpanned
object
> that gets pointed to by a non Oilpanned object?

Your point is valid and I'm planning to remove the complexity in the future.

- FilterOperations is GarbageCollected (i.e., allocated with 'new').

- We don't want to allow GarbageCollected objects to get allocated as a compound
object because the compound object doesn't have an object header. If someone
makes a programming mistake and stores the pointer to the compound object into
Member<FilterOperations>, it can cause security issues when Oilpan traces the
Member (because the FilterOperations object doesn't have a valid header). Thus
we want to forbid GarbageCollected objects to get allocated as a compound
object. This is why we use FilterOperationsWrapper.

- However, I've already added asserts to Member's constructor etc to verify that
an object stored in the Member has a valid header. Assuming that the asserts are
sufficient, we can remove the restriction I mentioned above.

- Actually I'm not yet sure if we should force the restriction (for safety) or
remove the restriction (for convenience). It seems that the number of places
where we want to use GarbageCollected objects as compound classes is pretty
limited, I'm leaning toward to forcing the restriction. But I need more study.

(Either way, I'd like to keep the FilterOperationsWrapper at least in this CL.)

[alancutter (OOO until 2018), 2015-09-03 03:21:56]

On 2015/09/03 at 02:40:12, haraken wrote:
> 
> - FilterOperations is GarbageCollected (i.e., allocated with 'new').

This part is confusing to me, looking at it I can't see any sign that it's
garbage collected.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Its FilterOperationVector member is a HeapVector, is that what you mean by
FilterOperations is garbage collected?

[haraken, 2015-09-03 04:06:55]

On 2015/09/03 03:21:56, alancutter wrote:
> On 2015/09/03 at 02:40:12, haraken wrote:
> > 
> > - FilterOperations is GarbageCollected (i.e., allocated with 'new').
> 
> This part is confusing to me, looking at it I can't see any sign that it's
> garbage collected.
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Its FilterOperationVector member is a HeapVector, is that what you mean by
> FilterOperations is garbage collected?

Sorry, I was misunderstanding the problem.

The problem is that FilterOperations is used as a compound object in both
GarbageCollected and not-GarbageCollected objects.

Imagine that we're in oilpan builds.

class AnimatableFilterOperations : public RefCounted {
  FilterOperations m_operations;
};

class StyleFilterData : public GarbageCollected {
  FilterOperations m_operations;
};

class FilterOperations {
  PersistentHeapVector<Member<FilterOperation>> m_vector; // This works for
AnimatableFilterOperations but doesn't work for StyleFilterData.
  HeapVector<Member<FilterOperation>> m_vector; // This works for
StyleFilterData but doesn't work for AnimatableFilterOperations.
};

To avoid the problem, I made AnimatableFilterOperations use
Persistent<FilterOperationsWrapper>.

[alancutter (OOO until 2018), 2015-09-03 04:09:36]

On 2015/09/03 at 04:06:55, haraken wrote:
> On 2015/09/03 03:21:56, alancutter wrote:
> > On 2015/09/03 at 02:40:12, haraken wrote:
> > > 
> > > - FilterOperations is GarbageCollected (i.e., allocated with 'new').
> > 
> > This part is confusing to me, looking at it I can't see any sign that it's
> > garbage collected.
> >
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > Its FilterOperationVector member is a HeapVector, is that what you mean by
> > FilterOperations is garbage collected?
> 
> Sorry, I was misunderstanding the problem.
> 
> The problem is that FilterOperations is used as a compound object in both
GarbageCollected and not-GarbageCollected objects.
> 
> Imagine that we're in oilpan builds.
> 
> class AnimatableFilterOperations : public RefCounted {
>   FilterOperations m_operations;
> };
> 
> class StyleFilterData : public GarbageCollected {
>   FilterOperations m_operations;
> };
> 
> class FilterOperations {
>   PersistentHeapVector<Member<FilterOperation>> m_vector; // This works for
AnimatableFilterOperations but doesn't work for StyleFilterData.
>   HeapVector<Member<FilterOperation>> m_vector; // This works for
StyleFilterData but doesn't work for AnimatableFilterOperations.
> };
> 
> To avoid the problem, I made AnimatableFilterOperations use
Persistent<FilterOperationsWrapper>.

Thanks for explaining, lgtm. :)

[haraken, 2015-09-03 04:10:00]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2015-09-03 04:10:05]

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

[commit-bot: I haz the power, 2015-09-03 05:13:36]

Committed patchset #19 (id:360001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201698

[sof, 2015-09-03 09:20:53]

Oilpan bots are not too happy with this one,

 
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...
 
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...
 
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...

[haraken, 2015-09-03 10:23:51]

On 2015/09/03 09:20:53, sof wrote:
> Oilpan bots are not too happy with this one,
> 
>  
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...
>  
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...
>  
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...

Started looking.

[haraken, 2015-09-03 11:14:06]

On 2015/09/03 10:23:51, haraken wrote:
> On 2015/09/03 09:20:53, sof wrote:
> > Oilpan bots are not too happy with this one,
> > 
> >  
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...
> >  
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...
> >  
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...
> 
> Started looking.

Fixed in https://codereview.chromium.org/1302233010/.

[sof, 2015-09-03 13:44:30]

On 2015/09/03 11:14:06, haraken wrote:
> On 2015/09/03 10:23:51, haraken wrote:
> > On 2015/09/03 09:20:53, sof wrote:
> > > Oilpan bots are not too happy with this one,
> > > 
> > >  
> > >
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...
> > >  
> > >
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...
> > >  
> > >
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...
> > 
> > Started looking.
> 
> Fixed in https://codereview.chromium.org/1302233010/.

Thanks, looks better. The leaks are still present,


http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...

[haraken, 2015-09-03 14:57:48]

On 2015/09/03 13:44:30, sof wrote:
> On 2015/09/03 11:14:06, haraken wrote:
> > On 2015/09/03 10:23:51, haraken wrote:
> > > On 2015/09/03 09:20:53, sof wrote:
> > > > Oilpan bots are not too happy with this one,
> > > > 
> > > >  
> > > >
> > >
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...
> > > >  
> > > >
> > >
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...
> > > >  
> > > >
> > >
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...
> > > 
> > > Started looking.
> > 
> > Fixed in https://codereview.chromium.org/1302233010/.
> 
> Thanks, looks better. The leaks are still present,
> 
> 
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...

The leak bot is great... Let me take care of it tomorrow morning.

[haraken, 2015-09-04 01:11:56]

On 2015/09/03 14:57:48, haraken wrote:
> On 2015/09/03 13:44:30, sof wrote:
> > On 2015/09/03 11:14:06, haraken wrote:
> > > On 2015/09/03 10:23:51, haraken wrote:
> > > > On 2015/09/03 09:20:53, sof wrote:
> > > > > Oilpan bots are not too happy with this one,
> > > > > 
> > > > >  
> > > > >
> > > >
> > >
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...
> > > > >  
> > > > >
> > > >
> > >
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...
> > > > >  
> > > > >
> > > >
> > >
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...
> > > > 
> > > > Started looking.
> > > 
> > > Fixed in https://codereview.chromium.org/1302233010/.
> > 
> > Thanks, looks better. The leaks are still present,
> > 
> > 
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...
> 
> The leak bot is great... Let me take care of it tomorrow morning.

Fix uploaded: https://codereview.chromium.org/1325433005/