Oilpan: enable eager tracing of common animation objects.

Oilpan: enable eager tracing of common animation objects.

Using the ability to declare class types as eagerly traceable during
marking, annotate common Web Animation classes as such.

This halves the marking time for three blink_perf.animation tests on
Linux + Windows:

 - api-balls-keyframe-animations.html
 - api-set-current-time-balls-keyframe-animations.html
 - balls-keyframe-animations.html

No regressions (or signficant improvements) on other tests.

R=kouhei,haraken
BUG=420515
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186410

[sof, 2014-11-27 21:02:36]

Please take a look.

Depends on the mechanism added in https://codereview.chromium.org/765673004/

[sof, 2014-11-27 21:02:57]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2014-11-27 21:02:57]

+oilpan-reviews

[kouhei (in TOK), 2014-11-28 01:02:08]

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

[kouhei (in TOK), 2014-11-28 01:02:09]

lgtm

[haraken, 2014-11-28 05:28:52]

What's a difference between 'public EagerTracing' and WILL_BE_EAGERLY_TRACED() ?

[sof, 2014-11-28 06:23:11]

On 2014/11/28 05:28:52, haraken wrote:
> What's a difference between 'public EagerTracing' and WILL_BE_EAGERLY_TRACED()
?

TraceEagerly is the name of the class.

The following currently holds and perhaps explains just as quickly?

class T1 : public TraceEagerly { };
class T2 : public T1 { };

class T3 { };
WILL_BE_EAGERLY_TRACED(T3);

class T4 : public T3 { };

COMPILE_ASSERT(TraceEagerlyTrait<T1>::value, ShouldBeTrue);
COMPILE_ASSERT(TraceEagerlyTrait<T2>::value, ShouldBeTrue);
COMPILE_ASSERT(TraceEagerlyTrait<T3>::value, ShouldBeTrue);
COMPILE_ASSERT(!TraceEagerlyTrait<T4>::value, ShouldBeTrue);

i.e., the trait specialization that WILL_BE_EAGERLY_TRACED(T) provides applies
to the type T only (and not classes that derive from it.)

I suspect some EnableIf()'ery can be used to allow the trait specialization to
handle subclasses of T also.

[haraken, 2014-11-28 06:26:26]

On 2014/11/28 06:23:11, sof wrote:
> On 2014/11/28 05:28:52, haraken wrote:
> > What's a difference between 'public EagerTracing' and
WILL_BE_EAGERLY_TRACED()
> ?
> 
> TraceEagerly is the name of the class.
> 
> The following currently holds and perhaps explains just as quickly?
> 
> class T1 : public TraceEagerly { };
> class T2 : public T1 { };
> 
> class T3 { };
> WILL_BE_EAGERLY_TRACED(T3);
> 
> class T4 : public T3 { };
> 
> COMPILE_ASSERT(TraceEagerlyTrait<T1>::value, ShouldBeTrue);
> COMPILE_ASSERT(TraceEagerlyTrait<T2>::value, ShouldBeTrue);
> COMPILE_ASSERT(TraceEagerlyTrait<T3>::value, ShouldBeTrue);
> COMPILE_ASSERT(!TraceEagerlyTrait<T4>::value, ShouldBeTrue);
> 
> i.e., the trait specialization that WILL_BE_EAGERLY_TRACED(T) provides applies
> to the type T only (and not classes that derive from it.)
> 
> I suspect some EnableIf()'ery can be used to allow the trait specialization to
> handle subclasses of T also.

Thanks for the clarification.

> COMPILE_ASSERT(TraceEagerlyTrait<T1>::value, ShouldBeTrue);
> COMPILE_ASSERT(TraceEagerlyTrait<T2>::value, ShouldBeTrue);
> COMPILE_ASSERT(TraceEagerlyTrait<T3>::value, ShouldBeTrue);
> COMPILE_ASSERT(!TraceEagerlyTrait<T4>::value, ShouldBeTrue);

COMPILE_ASSERT(!TraceEagerlyTrait<T4>::value, ShouldBeFalse); ?

[sof, 2014-11-28 06:32:45]

On 2014/11/28 06:26:26, haraken wrote:
...
> 
> > COMPILE_ASSERT(TraceEagerlyTrait<T1>::value, ShouldBeTrue);
> > COMPILE_ASSERT(TraceEagerlyTrait<T2>::value, ShouldBeTrue);
> > COMPILE_ASSERT(TraceEagerlyTrait<T3>::value, ShouldBeTrue);
> > COMPILE_ASSERT(!TraceEagerlyTrait<T4>::value, ShouldBeTrue);
> 
> COMPILE_ASSERT(!TraceEagerlyTrait<T4>::value, ShouldBeFalse); ?

That would be preferable, yes. The use of the empty TraceEagerly class could be
phased out, if so.

[haraken, 2014-11-28 06:46:47]

On 2014/11/28 06:32:45, sof wrote:
> On 2014/11/28 06:26:26, haraken wrote:
> ...
> > 
> > > COMPILE_ASSERT(TraceEagerlyTrait<T1>::value, ShouldBeTrue);
> > > COMPILE_ASSERT(TraceEagerlyTrait<T2>::value, ShouldBeTrue);
> > > COMPILE_ASSERT(TraceEagerlyTrait<T3>::value, ShouldBeTrue);
> > > COMPILE_ASSERT(!TraceEagerlyTrait<T4>::value, ShouldBeTrue);
> > 
> > COMPILE_ASSERT(!TraceEagerlyTrait<T4>::value, ShouldBeFalse); ?
> 
> That would be preferable, yes. The use of the empty TraceEagerly class could
be
> phased out, if so.

Hmm, I wonder why we need WILL_BE_EAGERLY_TRACED. Is there any case where we
want to trace a parent class eagerly but don't want to trace subclasses eagerly?
(If there is such as case, it seems to me that we're going a wrong way. The rule
seems a bit too complicated.)

[sof, 2014-11-28 07:05:44]

On 2014/11/28 06:46:47, haraken wrote:
> On 2014/11/28 06:32:45, sof wrote:
> > On 2014/11/28 06:26:26, haraken wrote:
> > ...
> > > 
> > > > COMPILE_ASSERT(TraceEagerlyTrait<T1>::value, ShouldBeTrue);
> > > > COMPILE_ASSERT(TraceEagerlyTrait<T2>::value, ShouldBeTrue);
> > > > COMPILE_ASSERT(TraceEagerlyTrait<T3>::value, ShouldBeTrue);
> > > > COMPILE_ASSERT(!TraceEagerlyTrait<T4>::value, ShouldBeTrue);
> > > 
> > > COMPILE_ASSERT(!TraceEagerlyTrait<T4>::value, ShouldBeFalse); ?
> > 
> > That would be preferable, yes. The use of the empty TraceEagerly class could
> be
> > phased out, if so.
> 
> Hmm, I wonder why we need WILL_BE_EAGERLY_TRACED. Is there any case where we
> want to trace a parent class eagerly but don't want to trace subclasses
eagerly?
> (If there is such as case, it seems to me that we're going a wrong way. The
rule
> seems a bit too complicated.)

I don't think you should preclude that possibility when providing a mechanism.

i.e., the subclass T1 might have long chains of T1s available via Member<>s. I
think you want to be circumspect about whether or not T1 should be eagerly
traced if the parent P is marked as being eager.

[haraken, 2014-11-28 07:26:51]

On 2014/11/28 07:05:44, sof wrote:
> On 2014/11/28 06:46:47, haraken wrote:
> > On 2014/11/28 06:32:45, sof wrote:
> > > On 2014/11/28 06:26:26, haraken wrote:
> > > ...
> > > > 
> > > > > COMPILE_ASSERT(TraceEagerlyTrait<T1>::value, ShouldBeTrue);
> > > > > COMPILE_ASSERT(TraceEagerlyTrait<T2>::value, ShouldBeTrue);
> > > > > COMPILE_ASSERT(TraceEagerlyTrait<T3>::value, ShouldBeTrue);
> > > > > COMPILE_ASSERT(!TraceEagerlyTrait<T4>::value, ShouldBeTrue);
> > > > 
> > > > COMPILE_ASSERT(!TraceEagerlyTrait<T4>::value, ShouldBeFalse); ?
> > > 
> > > That would be preferable, yes. The use of the empty TraceEagerly class
could
> > be
> > > phased out, if so.
> > 
> > Hmm, I wonder why we need WILL_BE_EAGERLY_TRACED. Is there any case where we
> > want to trace a parent class eagerly but don't want to trace subclasses
> eagerly?
> > (If there is such as case, it seems to me that we're going a wrong way. The
> rule
> > seems a bit too complicated.)
> 
> I don't think you should preclude that possibility when providing a mechanism.
> 
> i.e., the subclass T1 might have long chains of T1s available via Member<>s. I
> think you want to be circumspect about whether or not T1 should be eagerly
> traced if the parent P is marked as being eager.

OK, makes sense. I think the rule of WILL_BE_EAGERLY_TRACED and EagerTracing
would be too complicated to introduce to trunk, but we can worry about it when
enabling type-specific eager tracing on trunk.

LGTM.

[sof, 2014-11-28 12:14:03]

On 2014/11/28 07:26:51, haraken wrote:
> On 2014/11/28 07:05:44, sof wrote:
> > On 2014/11/28 06:46:47, haraken wrote:
> > > On 2014/11/28 06:32:45, sof wrote:
> > > > On 2014/11/28 06:26:26, haraken wrote:
> > > > ...
> > > > > 
> > > > > > COMPILE_ASSERT(TraceEagerlyTrait<T1>::value, ShouldBeTrue);
> > > > > > COMPILE_ASSERT(TraceEagerlyTrait<T2>::value, ShouldBeTrue);
> > > > > > COMPILE_ASSERT(TraceEagerlyTrait<T3>::value, ShouldBeTrue);
> > > > > > COMPILE_ASSERT(!TraceEagerlyTrait<T4>::value, ShouldBeTrue);
> > > > > 
> > > > > COMPILE_ASSERT(!TraceEagerlyTrait<T4>::value, ShouldBeFalse); ?
> > > > 
> > > > That would be preferable, yes. The use of the empty TraceEagerly class
> could
> > > be
> > > > phased out, if so.
> > > 
> > > Hmm, I wonder why we need WILL_BE_EAGERLY_TRACED. Is there any case where
we
> > > want to trace a parent class eagerly but don't want to trace subclasses
> > eagerly?
> > > (If there is such as case, it seems to me that we're going a wrong way.
The
> > rule
> > > seems a bit too complicated.)
> > 
> > I don't think you should preclude that possibility when providing a
mechanism.
> > 
> > i.e., the subclass T1 might have long chains of T1s available via Member<>s.
I
> > think you want to be circumspect about whether or not T1 should be eagerly
> > traced if the parent P is marked as being eager.
> 
> OK, makes sense. I think the rule of WILL_BE_EAGERLY_TRACED and EagerTracing
> would be too complicated to introduce to trunk, but we can worry about it when
> enabling type-specific eager tracing on trunk.
> 
> LGTM.

I've altered TraceEagerlyTrait<> to support declaration of eager tracing over a
class & its subclasses, which renders TraceEagerly of little value. Hence,
removed.

So, the rule is now

- Use WILL_BE_EAGERLY_TRACED(Type); if you want to enable eager trace() calls
over Type and at any of its subclasses.
- Use WILL_BE_EAGERLY_TRACED_CLASS(Type) if you only want to enable eager
trace() calls at type T (but not at its subclasses.)

Similarly for the negative versions of the trait macros, should you need that
when/if always eager mode is enabled.

[haraken, 2014-11-28 12:34:35]

On 2014/11/28 12:14:03, sof wrote:
> On 2014/11/28 07:26:51, haraken wrote:
> > On 2014/11/28 07:05:44, sof wrote:
> > > On 2014/11/28 06:46:47, haraken wrote:
> > > > On 2014/11/28 06:32:45, sof wrote:
> > > > > On 2014/11/28 06:26:26, haraken wrote:
> > > > > ...
> > > > > > 
> > > > > > > COMPILE_ASSERT(TraceEagerlyTrait<T1>::value, ShouldBeTrue);
> > > > > > > COMPILE_ASSERT(TraceEagerlyTrait<T2>::value, ShouldBeTrue);
> > > > > > > COMPILE_ASSERT(TraceEagerlyTrait<T3>::value, ShouldBeTrue);
> > > > > > > COMPILE_ASSERT(!TraceEagerlyTrait<T4>::value, ShouldBeTrue);
> > > > > > 
> > > > > > COMPILE_ASSERT(!TraceEagerlyTrait<T4>::value, ShouldBeFalse); ?
> > > > > 
> > > > > That would be preferable, yes. The use of the empty TraceEagerly class
> > could
> > > > be
> > > > > phased out, if so.
> > > > 
> > > > Hmm, I wonder why we need WILL_BE_EAGERLY_TRACED. Is there any case
where
> we
> > > > want to trace a parent class eagerly but don't want to trace subclasses
> > > eagerly?
> > > > (If there is such as case, it seems to me that we're going a wrong way.
> The
> > > rule
> > > > seems a bit too complicated.)
> > > 
> > > I don't think you should preclude that possibility when providing a
> mechanism.
> > > 
> > > i.e., the subclass T1 might have long chains of T1s available via
Member<>s.
> I
> > > think you want to be circumspect about whether or not T1 should be eagerly
> > > traced if the parent P is marked as being eager.
> > 
> > OK, makes sense. I think the rule of WILL_BE_EAGERLY_TRACED and EagerTracing
> > would be too complicated to introduce to trunk, but we can worry about it
when
> > enabling type-specific eager tracing on trunk.
> > 
> > LGTM.
> 
> I've altered TraceEagerlyTrait<> to support declaration of eager tracing over
a
> class & its subclasses, which renders TraceEagerly of little value. Hence,
> removed.
> 
> So, the rule is now
> 
> - Use WILL_BE_EAGERLY_TRACED(Type); if you want to enable eager trace() calls
> over Type and at any of its subclasses.
> - Use WILL_BE_EAGERLY_TRACED_CLASS(Type) if you only want to enable eager
> trace() calls at type T (but not at its subclasses.)
> 
> Similarly for the negative versions of the trait macros, should you need that
> when/if always eager mode is enabled.

Thanks for the tidy-up, looks much nicer. LGTM.

[sof, 2014-12-03 10:39:33]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-12-03 10:40:19]

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

[sof, 2014-12-03 10:48:39]

animators and others: we're landing this CL, which adds a small amount of
Oilpan-specific trait declarations (via macros) to help guide the Oilpan GC when
marking.

We're still considering approaches here (and evaluating the perf win), but if
you consider the declarations as adding details that really shouldn't be there,
do let us know.

[commit-bot: I haz the power, 2014-12-03 11:58:16]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186410