Closed Bug 1339220 Opened 5 years ago Closed 5 years ago

Add a telemetry probe to measure compositor animation duration

Categories

(Core :: Graphics: Layers, defect)

54 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: kats, Assigned: kats)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

We talked about the telemetry probe desired for bug 1338347 today. One of the things that came up was that the proposed metric in bug 1338347 produces a single number (number of frames composited / expected number of frames) regardless of the length of the animation. This can skew the numbers because short animations (just a few frames) can disproportionately pull down the metric compared to longer animations.

Something we decided would be useful to do in general, but also to better bucketize the above metric, is to land a probe to record the duration of compositor animations. Being able to flag the start/end of compositor animations is also useful for other things like the gecko profiler, and putting this machinery in place now will make it simpler to write the patch for bug 1338347. I'll write a patch for this sometime this week.
Attached patch WIP (obsolete) — Splinter Review
Here's a first cut at doing this. It seems to detect compositor animations but I haven't tested it thoroughly yet. It seems to fire a lot - for example any time you mouse over a link and the little URL tooltip thingy appears in the bottom corner of the browser UI, that triggers an animation. I don't know if we care about those or want to filter them out?
> I don't know if we care about those or want to filter them out?

As they fade in, we should care. This leads to another factor we should track, can we tell how much of the viewport animates (tooltip/spinner vs large panels moving)?

> It seems to detect compositor animations but I haven't tested it thoroughly yet.

We talked about tell apart animations from content vs main; is that feasible or should we go with the aggregate first? I guess we can tell for the main thread animation metric but not for compositor.
(In reply to :Harald Kirschner :digitarald from comment #2)
> As they fade in, we should care. This leads to another factor we should
> track, can we tell how much of the viewport animates (tooltip/spinner vs
> large panels moving)?

Ok, so I won't do any filtering. I can record a ballpark "area" of animations but I'm not sure how useful it will be. As layers animate they can change in size, so the size isn't constant over time. Also they might be partly offscreen or clipped by other things on top of them, and all that won't be captured at the level that I'm instrumenting. Still I suppose it might be useful to have an idea of how much screen area is being animated so I'll record a high-watermark value per animation.

> We talked about tell apart animations from content vs main; is that feasible
> or should we go with the aggregate first? I guess we can tell for the main
> thread animation metric but not for compositor.

We can distinguish compositor animations that are coming from the content process vs compositor animations triggered by the UI process. However I'd rather defer that to later since it's not clear that it's going to provide any actionable information at this point. Unless you're interested in splitting the buckets by process? (e.g. <500ms animations in content process would go into a separate histogram from <500ms animations in UI process).
Attachment #8837170 - Attachment is obsolete: true
Also note that I intentionally kept the AnimationTracker per-compositor, because different compositors might be running on different displays with different refresh rates/vsyncs, and going forward we'll want to track those independently.
Comment on attachment 8837742 [details]
Bug 1339220 - Add telemetry probes to measure duration and max-area of animations in the compositor.

https://reviewboard.mozilla.org/r/112774/#review114278

::: gfx/layers/composite/AnimationTracker.h:17
(Diff revision 1)
> +namespace layers {
> +
> +/**
> + * Tracks the start and end of compositor animations.
> + */
> +class AnimationTracker {

The main-thread side of the Web Animations implementation has a class named PendingAnimationTracker (which is a "tracker" in a different sense, it tracks when animation need to start / be scheduled).

To avoid confusion, can we add a word to this class name, like CompositorAnimationTracker, or AnimationMetricsTracker?

::: gfx/layers/composite/AnimationTracker.h:22
(Diff revision 1)
> +class AnimationTracker {
> +public:
> +  AnimationTracker();
> +  ~AnimationTracker();
> +
> +  void UpdateAnimationInProgress(bool aInProgress, uint64_t aLayerArea);

Please document the units for aLayerArea. Based on the call site it looks like Layer pixels squared.

::: gfx/layers/composite/AnimationTracker.cpp:55
(Diff revision 1)
> +
> +void
> +AnimationTracker::AnimationEnded()
> +{
> +  MOZ_ASSERT(mCurrentAnimationStart);
> +  Telemetry::AccumulateTimeDelta(mozilla::Telemetry::COMPOSITOR_ANIMATION_DURATION, mCurrentAnimationStart);

The mozilla:: prefixes on names in this function are redundant, since we're inside namespace mozilla.

(Unless you're being super defensive in case someone introduces mozilla::layers::Telemetry with the same API but different behaviour, in which case, best write ::mozilla::Telemetry in case someone introduces mozilla::layers::mozilla::Telemetry :p).

::: gfx/layers/composite/AsyncCompositionManager.cpp:1322
(Diff revision 1)
> +  uint64_t layerAreaAnimated = 0;
>    bool wantNextFrame = SampleAnimations(root,
>      !mPreviousFrameTimeStamp.IsNull() ?
> -      mPreviousFrameTimeStamp : aCurrentFrame);
> +      mPreviousFrameTimeStamp : aCurrentFrame,
> +    &layerAreaAnimated);
> +  mAnimationTracker.UpdateAnimationInProgress(wantNextFrame, layerAreaAnimated);

You're computing |wantNextFrame| based on the return value of SampleAnimations(), which is computed based on the return value of SampleAnimationForEachNode().

However, |layerAreaAnimated| is set if SampleAnimationForEachNode() put 'true' into |hasInEffectAnimations|.

Looking at SampleAnimationForEachNode(), the return value can be true without setting |hasInEffectAnimations| to true, if there are animations but they all take this early-exit path [1].

As a result, you may get some |UpdateAnimationInProgress(true, 0)| calls. That may be OK, just wanted to make sure you're aware of it.

[1] http://searchfox.org/mozilla-central/rev/cac6cb6a10afb8ebb2ecfbeeedaff7c66f57dd75/gfx/layers/AnimationHelper.cpp#129

::: toolkit/components/telemetry/Histograms.json:200
(Diff revision 1)
> +    "alert_emails": ["kgupta@mozilla.com"],
> +    "bug_numbers": [1339220]
> +  },
> +  "COMPOSITOR_ANIMATION_MAX_LAYER_AREA" : {
> +    "expires_in_version": "58",
> +    "description": "High-watermark of layer area affected during a compositor animation",

Probably worth mentioning the units (Layer pixels squared) here as well.
Attachment #8837742 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #6)
> Looking at SampleAnimationForEachNode(), the return value can be true
> without setting |hasInEffectAnimations| to true, if there are animations but
> they all take this early-exit path [1].

(In case you're wondering what the semantic difference is, it's roughly "at layer-building time, the main thread code saw fit to forward this animation to the compositor in case it'll be in effect at composition time" vs. "the animation is actually in effect at composition time".)
Comment on attachment 8837742 [details]
Bug 1339220 - Add telemetry probes to measure duration and max-area of animations in the compositor.

https://reviewboard.mozilla.org/r/112774/#review114278

> The main-thread side of the Web Animations implementation has a class named PendingAnimationTracker (which is a "tracker" in a different sense, it tracks when animation need to start / be scheduled).
> 
> To avoid confusion, can we add a word to this class name, like CompositorAnimationTracker, or AnimationMetricsTracker?

I renamed it to AnimationMetricsTracker. I figured that was more useful than CompositorAnimationTracker, particularly given it's sitting in the composite/ folder.

> Please document the units for aLayerArea. Based on the call site it looks like Layer pixels squared.

Done

> The mozilla:: prefixes on names in this function are redundant, since we're inside namespace mozilla.
> 
> (Unless you're being super defensive in case someone introduces mozilla::layers::Telemetry with the same API but different behaviour, in which case, best write ::mozilla::Telemetry in case someone introduces mozilla::layers::mozilla::Telemetry :p).

Removed the prefixes (it was leftover copypasta :))

> You're computing |wantNextFrame| based on the return value of SampleAnimations(), which is computed based on the return value of SampleAnimationForEachNode().
> 
> However, |layerAreaAnimated| is set if SampleAnimationForEachNode() put 'true' into |hasInEffectAnimations|.
> 
> Looking at SampleAnimationForEachNode(), the return value can be true without setting |hasInEffectAnimations| to true, if there are animations but they all take this early-exit path [1].
> 
> As a result, you may get some |UpdateAnimationInProgress(true, 0)| calls. That may be OK, just wanted to make sure you're aware of it.
> 
> [1] http://searchfox.org/mozilla-central/rev/cac6cb6a10afb8ebb2ecfbeeedaff7c66f57dd75/gfx/layers/AnimationHelper.cpp#129

That's a good point. I think in this case it's ok to get calls of (true, 0). If we "Finish an animation" (i.e. go back to calls of (false, 0) without the layer area ever increasing, it will show up as a zero-area histogram in the probe, which is easy enough to filter out at that point if we need to. And it might be useful to know how many times that's happening anyway. The only concern I have is that we might be artificially inflating the length of actual non-zero-area animations. e.g. if we get calls like this:

(true, 0)
(true, 0)
...
(true, 0)
(true, 1000)
(true, 1000)
(false, 0)

Then an animation that was in actual fact only two frames would get inflated to be much longer. But I assume this case is kind of rare, and maybe the inflation is more representative of real-world behaviour because we are actually still compositing those frames.

> Probably worth mentioning the units (Layer pixels squared) here as well.

Done.
Comment on attachment 8837742 [details]
Bug 1339220 - Add telemetry probes to measure duration and max-area of animations in the compositor.

https://reviewboard.mozilla.org/r/112774/#review117408

data-r=me
Attachment #8837742 - Flags: review?(benjamin) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8ddcd324a0f1
Add telemetry probes to measure duration and max-area of animations in the compositor. r=botond,bsmedberg
https://hg.mozilla.org/mozilla-central/rev/8ddcd324a0f1
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Data is starting to show up on telemetry.mozilla.org. For three days worth (March 1 - March 4) there around 43 million compositor animations, and the histogram distributions looks reasonable.
Interesting spikes that are probably our own animations (url overlay fade, tab closing/opening?).

Can we tell what animations are triggered by content vs main?
(In reply to :Harald Kirschner :digitarald from comment #14)
> Interesting spikes that are probably our own animations (url overlay fade,
> tab closing/opening?).

I presume so, yes

> Can we tell what animations are triggered by content vs main?

Not from this data, no.
You need to log in before you can comment on or make changes to this bug.