Bug 84210 - [chromium] Add tracing for active composited animations
Summary: [chromium] Add tracing for active composited animations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: vollick
URL:
Keywords:
Depends on: 94350
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-17 16:34 PDT by Nat Duca
Modified: 2012-08-20 11:22 PDT (History)
4 users (show)

See Also:


Attachments
Patch (5.87 KB, patch)
2012-04-17 16:35 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Fixes (7.03 KB, patch)
2012-04-18 13:40 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Fixes (7.03 KB, patch)
2012-04-18 13:45 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Patch for landing (7.23 KB, patch)
2012-04-25 00:40 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
One that builds, hopefully (26.88 KB, text/plain)
2012-04-25 00:49 PDT, Nat Duca
no flags Details
Patch (19.28 KB, patch)
2012-08-15 11:26 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (20.77 KB, patch)
2012-08-15 13:38 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (12.06 KB, patch)
2012-08-16 09:03 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (13.13 KB, patch)
2012-08-16 19:17 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (13.65 KB, patch)
2012-08-20 09:54 PDT, vollick
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nat Duca 2012-04-17 16:34:48 PDT
[chromium] Add tracing for active composited animations
Comment 1 Nat Duca 2012-04-17 16:35:05 PDT
Created attachment 137632 [details]
Patch
Comment 2 vollick 2012-04-17 16:59:29 PDT
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?
Comment 3 Nat Duca 2012-04-18 13:08:31 PDT
(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. :'(
Comment 4 vollick 2012-04-18 13:18:34 PDT
(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.
Comment 5 Nat Duca 2012-04-18 13:33:13 PDT
Whoops! You're right.
Comment 6 Nat Duca 2012-04-18 13:40:39 PDT
Created attachment 137756 [details]
Fixes
Comment 7 Nat Duca 2012-04-18 13:45:10 PDT
Created attachment 137759 [details]
Fixes
Comment 8 James Robinson 2012-04-18 20:12:31 PDT
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?
Comment 9 Nat Duca 2012-04-25 00:40:31 PDT
Created attachment 138750 [details]
Patch for landing
Comment 10 Nat Duca 2012-04-25 00:49:15 PDT
Created attachment 138751 [details]
One that builds, hopefully
Comment 11 vollick 2012-08-15 11:26:25 PDT
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.
Comment 12 James Robinson 2012-08-15 11:35:19 PDT
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?
Comment 13 Nat Duca 2012-08-15 11:44:32 PDT
Comment on attachment 158603 [details]
Patch

Wow, thats nice. Maybe just fix the impl thing with a construction-time member var?
Comment 14 vollick 2012-08-15 13:38:45 PDT
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?
Comment 15 James Robinson 2012-08-15 16:04:00 PDT
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
Comment 16 James Robinson 2012-08-15 19:32:45 PDT
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
Comment 17 vollick 2012-08-16 09:03:54 PDT
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.
Comment 18 James Robinson 2012-08-16 10:06:18 PDT
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?
Comment 19 vollick 2012-08-16 19:17:58 PDT
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.
Comment 20 WebKit Review Bot 2012-08-17 06:48:34 PDT
Comment on attachment 158978 [details]
Patch

Clearing flags on attachment: 158978

Committed r125892: <http://trac.webkit.org/changeset/125892>
Comment 21 WebKit Review Bot 2012-08-17 06:48:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 WebKit Review Bot 2012-08-17 08:29:49 PDT
Re-opened since this is blocked by 94350
Comment 23 vollick 2012-08-20 09:54:52 PDT
Created attachment 159458 [details]
Patch

Fix the windows build.
Comment 24 WebKit Review Bot 2012-08-20 11:22:01 PDT
Comment on attachment 159458 [details]
Patch

Clearing flags on attachment: 159458

Committed r126045: <http://trac.webkit.org/changeset/126045>
Comment 25 WebKit Review Bot 2012-08-20 11:22:06 PDT
All reviewed patches have been landed.  Closing bug.