Summary: | [chromium] Add tracing for active composited animations | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nat Duca <nduca> | ||||||||||||||||||||||
Component: | New Bugs | Assignee: | vollick | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | cc-bugs, jamesr, vollick, webkit.review.bot | ||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Bug Depends on: | 94350 | ||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||
Attachments: |
|
Description
Nat Duca
2012-04-17 16:34:48 PDT
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? (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. Whoops! You're right. Created attachment 137756 [details]
Fixes
Created attachment 137759 [details]
Fixes
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? Created attachment 138750 [details]
Patch for landing
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.
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 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? 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 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.
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. Comment on attachment 158978 [details] Patch Clearing flags on attachment: 158978 Committed r125892: <http://trac.webkit.org/changeset/125892> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by 94350 Created attachment 159458 [details]
Patch
Fix the windows build.
Comment on attachment 159458 [details] Patch Clearing flags on attachment: 159458 Committed r126045: <http://trac.webkit.org/changeset/126045> All reviewed patches have been landed. Closing bug. |