Bug 84210 – [chromium] Add tracing for active composited animations

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nat Duca]

[chromium] Add tracing for active composited animations

[Nat Duca]

Created attachment 137632 [details]
Patch

I think this code might start a lot of unfinished async traces. Animations created on the main thread will start an async trace in their constructor, but after they're cloned they'll no longer be the controlling instance and won't end the trace in their destructor. I think this would be simpler if isControllingInstance was a constructor parameter, perhaps only available via a 2nd, private constructor. This would also let the clone method remain const.

Group id + target property (formerly known as the animation signature) uniquely identify an animation on a thread (the clone has the same signature). Could this be used instead of an instance id?

[Nat Duca]

(In reply to comment #2)
> I think this code might start a lot of unfinished async traces. Animations created on the main thread will start an async trace in their constructor, but after they're cloned they'll no longer be the controlling instance and won't end the trace in their destructor.

Correct, but then there's another active animation that will take over and that one's destructor should eventually run issuing the close, right? Where did I get confused? :)

> Group id + target property (formerly known as the animation signature) uniquely identify an animation on a thread (the clone has the same signature). Could this be used instead of an instance id?

Our id system requires a single integer or pointer. :'(

(In reply to comment #3)
> (In reply to comment #2)
> > I think this code might start a lot of unfinished async traces. Animations created on the main thread will start an async trace in their constructor, but after they're cloned they'll no longer be the controlling instance and won't end the trace in their destructor.
> 
> Correct, but then there's another active animation that will take over and that one's destructor should eventually run issuing the close, right? Where did I get confused? :)

Ah, right. The main thread's animation totally does get closed on the impl thread (since the id gets passed across), but now I think it's the impl thread animation that won't get its trace closed. The main and impl thread animations will both start traces with different id's in their constructors, but I think that only one of those traces will get closed. Is that right?

> 
> > Group id + target property (formerly known as the animation signature) uniquely identify an animation on a thread (the clone has the same signature). Could this be used instead of an instance id?
> 
> Our id system requires a single integer or pointer. :'(
kk.

[Nat Duca]

Whoops! You're right.

[Nat Duca]

Created attachment 137756 [details]
Fixes

[Nat Duca]

Created attachment 137759 [details]
Fixes

[James Robinson]

Comment on attachment 137759 [details]
Fixes

View in context: https://bugs.webkit.org/attachment.cgi?id=137759&action=review

R=me

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.h:137
> +    // Animations live dual lives. When they are first created, they live on the main thread and there isi

typo "isi" -> "is"

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.h:146
> +    // been transfered to the impl thread, the controlling instance is the main
> +    // thread.

controlling instance is the main thread what? the main thread animation?

[Nat Duca]

Created attachment 138750 [details]
Patch for landing

[Nat Duca]

Created attachment 138751 [details]
One that builds, hopefully

Created attachment 158603 [details]
Patch

This patch controls the tracing in the layer animation controller rather than in
the active animation. In addition to tracing the duration of the animations (on
both threads), it traces when animations change state.

[James Robinson]

Comment on attachment 158603 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158603&action=review

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:97
> +    virtual bool isImpl() const OVERRIDE { return false; }

This is gross to add just for debugging.  Can you find another way?

[Nat Duca]

Comment on attachment 158603 [details]
Patch

Wow, thats nice. Maybe just fix the impl thing with a construction-time member var?

Created attachment 158628 [details]
Patch

(In reply to comment #12)
> (From update of attachment 158603 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158603&action=review
>
> > Source/WebCore/platform/graphics/chromium/LayerChromium.h:97
> > +    virtual bool isImpl() const OVERRIDE { return false; }
>
> This is gross to add just for debugging.  Can you find another way?

Yeah, it is gross. I tried what Nat suggested and now CCActiveAnimations know if
they are the 'controlling instance' (defined in CCActiveAnimation.h). Does that
seem any better?

[James Robinson]

Comment on attachment 158628 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158628&action=review

I like the controlling instance concept much better.

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:146
> +    bool forImplThread = false;
> +    return clone(forImplThread);

Looks like what you really want is for clone to take a 2-state enum parameter, not a bool

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:427
> +const char * getTargetPropertyName(CCActiveAnimation::TargetProperty targetProperty)

"const char*", no space between the * and the type. "get" is a bit of an odd prefix for this. Why not have a static lookup table keyed by enum? it could live in .rodata

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:462
> +    char nameBuffer[256];
> +    sprintf(nameBuffer, "%s-%d%s", getTargetPropertyName(animation->targetProperty()), animation->group(), animation->isControllingInstance() ? "(impl)" : "");

use snprintf() if you have a fixed-sized buffer

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:473
> +    const char *oldRunStateName = getRunStateName(animation->runState());

The * goes with the type (const char), not the variable name.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:481
> +    char stateBuffer[256];
> +    sprintf(stateBuffer, "%s->%s", oldRunStateName, newRunStateName);

ditto

[James Robinson]

Comment on attachment 158628 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158628&action=review

I think with those changes you're good to go.  Feel free to put another patch up for review if you're unsure about any of it or just want a second look.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:475
> +    const char *newRunStateName = getRunStateName(runState);

and again

Created attachment 158835 [details]
Patch

Addressed reviewer comments, but adding r? because I reworked the patch a fair
bit. Now that CCActiveAnimations own all the information to do the tracing, it
makes no sense for that code to live in CCLayerAnimationController. This made
the patch much cleaner, but there was one catch -- in the animation controller,
when we cloned animations for the impl thread, this was often followed by an
initialization step to set up the initial run state and start time. These state
changes caused logging, but they should not have. To get around this, I made a
new clone function called cloneAndInitialize for cloning and initializing.

[James Robinson]

Comment on attachment 158835 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158835&action=review

R=me again - this looks real good. Left a few comments for you to ponder, but I leave it up to you to decide what to do about them.

> Source/WebCore/ChangeLog:8
> +        This patch issues the trace events from the controller. It will report

looks a bit out of date, since the traces are now issued by the animation itself

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:36
> +static const char* const s_runStateNames[] = {

could you comment the enums these statics are supposed to match up with?

can we compile assert that these stay in sync with the enums - or at least have the same length? perhaps we could add a sentinel enum value and COMPILE_ASSERT() on the size of these arrays?

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.h:117
> +        ForImplThread = 0,
> +        ForMainThread

could we express this enum in terms of controlling / non controlling rather than main thread / impl thread? other than this, the CCActiveAnimation logic doesn't know anything about threads specifically - does it?

Created attachment 158978 [details]
Patch

(In reply to comment #18)
> (From update of attachment 158835 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158835&action=review
>
> R=me again - this looks real good. Left a few comments for you to ponder, but I leave it up to you to decide what to do about them.
>
> > Source/WebCore/ChangeLog:8
> > +        This patch issues the trace events from the controller. It will report
>
> looks a bit out of date, since the traces are now issued by the animation itself
Fixed.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:36
> > +static const char* const s_runStateNames[] = {
>
> could you comment the enums these statics are supposed to match up with?
Done.
>
> can we compile assert that these stay in sync with the enums - or at least have the same length? perhaps we could add a sentinel enum value and COMPILE_ASSERT() on the size of these arrays?
Done.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.h:117
> > +        ForImplThread = 0,
> > +        ForMainThread
>
> could we express this enum in terms of controlling / non controlling rather than main thread / impl thread? other than this, the CCActiveAnimation logic doesn't know anything about threads specifically - does it?
Done.

[WebKit Review Bot]

Comment on attachment 158978 [details]
Patch

Clearing flags on attachment: 158978

Committed r125892: <http://trac.webkit.org/changeset/125892>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 94350

Created attachment 159458 [details]
Patch

Fix the windows build.

[WebKit Review Bot]

Comment on attachment 159458 [details]
Patch

Clearing flags on attachment: 159458

Committed r126045: <http://trac.webkit.org/changeset/126045>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.