1454324 - Skip calculating animation value on the compositor if animation progress value hasn't been changed since the last composition

Status: RESOLVED FIXED

(Core :: DOM: Animation, enhancement)

Description

[Hiroyuki Ikezoe (:hiro)]

We've been doing such optimizations for animations on the main-thread.  There is no reason we don't do the optimizations on the compositor.

Actually front-end people expected it (bug 1448133).

Comment 1

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4e47d38ba7f2715d4876a96e80378550ebf7387

Comment 2

[Hiroyuki Ikezoe (:hiro)]

test_missing-keyframe-on-compositor.html failed in the try.  There are something I am missing.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

I was missing 'missing-keyframe' case.  I am going to optimize single animation case here.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d411b83ae19eba17a65dd309880fadd6ff338a6

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Also I am missing animation-timing-function and transition-timing-function are the default timing function for each keyframe.  So, the approach in the try is not sufficient CSS animations/transitions.  We have to check that the target segment is not changed and the position in the segment is not changed as well.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

There were more things that needed to be involved. When we skip composing new animation values we have to use the previous composed values in any way.  That means that we can skip animation calculation but can't skip composing.  But on WebRender, it seems we don't need to compose the previous values?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd1eaee9b0dc1650116d05b2f9d24ef52e212e25

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Thanks for looking at this, hiro. Based on what you've found here, what are the chances this could ride the trains on 61? I ask, because I'm hoping to drive a SHIELD study on reducing the tab throbber frame rate to see how it impacts page load time, and I think we'd need this.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Yep, I've been working on this to land this as soon as possible.  Unfortunately, the approach in comment 5 didn't work in some cases [1].  So I had to figure out another way.  The another way seems to work fine [2].  So it will be in time for 61. :)

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=b51fd88b667d35353d064cce47ef790dd16b961b&selectedJob=174266133

[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=52a0e986d9b7d758e384f1806dd8534f9ac8d244

Comment 8

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> [2]
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=52a0e986d9b7d758e384f1806dd8534f9ac8d244

Oops, forgot to mention about H3 failure in this try.  it's bug 1454839.  I have no idea why the bug number doesn't appear there.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Also note that the approach what I am doing is less effective than the equivalent optimization on the main-thread.  Since on the compositor, even if we skip calculating animation values, we've already started a new transaction at that moment.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1454324 - Cache animation TimingParams on the compositor.

We don't need to calculate TimingParams each time we compose an animation on
the compositor because TimingParams is immobile since the animation was sent to
the compositor.

Review commit: https://reviewboard.mozilla.org/r/237870/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/237870/

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1454324 - Set non-animated values to the layer only if there is no running animation.

In the next patch, we skip updating animation value for the layer if the
animation value isn't changed.  So without this patch, we will have to update
animation value even if the value isn't changed at all.

Review commit: https://reviewboard.mozilla.org/r/237872/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/237872/

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1454324 - Skip calculating animation value if animation's progress value hasn't been changed since the last composition and if there are no other animations on the same element.

Note that we have to calculate animation values if there is another animation
since the other animaiton might be 'accumulate' or 'add', or might have a
missing keyframe (i.e. the calculated animation values will be used in the
missing keyframe).

Review commit: https://reviewboard.mozilla.org/r/237874/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/237874/

Comment 13

[Brian Birtles (:birtles)]

Comment on attachment 8969152 [details]
Bug 1454324 - Cache animation TimingParams on the compositor.

https://reviewboard.mozilla.org/r/237870/#review243604

::: gfx/layers/AnimationHelper.cpp:462
(Diff revision 1)
> +      Move(AnimationUtils::TimingFunctionToComputedTimingFunction(
> +           animation.easingFunction()))

(I actually wonder if we need the Move here -- I would have thought we already have an rvalue here -- but in the interests of keeping this patch just to moving code it's probably fine to leave this.)

Comment 14

[Brian Birtles (:birtles)]

Comment on attachment 8969154 [details]
Bug 1454324 - Skip calculating animation value if animation's progress value hasn't been changed since the last composition and if there are no other animations on the same element.

https://reviewboard.mozilla.org/r/237874/#review243608

This looks good but I'm curious about the second optimization here.

::: commit-message-fec26:4
(Diff revision 1)
> +Bug 1454324 - Skip calculating animation value if animation's progress value hasn't been changed since the last composition and if there are no other animations on the same element. r?birtles,kats
> +
> +Note that we have to calculate animation values if there is another animation
> +since the other animaiton might be 'accumulate' or 'add', or might have a

s/animaiton/animation/

::: dom/animation/KeyframeEffectReadOnly.cpp:1591
(Diff revision 1)
> +  const ComputedTiming& aComputedTiming,
> +  const Nullable<double>& aProgressOnLastCompose,
> +  IterationCompositeOperation aIterationComposite,
> +  uint64_t aCurrentIterationOnLastCompose)

aProgressOnLastCompose and aCurrentIterationOnLastCompose should be together since, together, they represent the result of the timing model from the last time we composed.

Perhaps the order could be:

  aComputedTiming,
  aIterationComposite,
  aProgressOnLastCompose,
  aCurrentIterationOnLastCompose

::: gfx/layers/AnimationHelper.h:29
(Diff revision 1)
>  struct AnimData {
>    InfallibleTArray<mozilla::AnimationValue> mStartValues;
>    InfallibleTArray<mozilla::AnimationValue> mEndValues;
>    InfallibleTArray<Maybe<mozilla::ComputedTimingFunction>> mFunctions;
>    TimingParams mTiming;
> +  // These two variables are the counter part of same name variables in

s/counter part/counterpart/

or just:

"These two variables correspond to the variables of the same names in KeyframeEffectReadOnly and are used for the same purpose: to skip composing animations whose progress has not changed."

::: gfx/layers/AnimationHelper.h:34
(Diff revision 1)
> +  // These two variables are used for a similar optimization above but work for
> +  // timing function in each keyframe.

s/but work for/but are applied to the/

::: gfx/layers/AnimationHelper.h:218
(Diff revision 1)
> +   * Returns SampleResult::None if the all animations haven't yet started (i.e.
> +   * in delay state), SampleResult::Skipped if the animation values haven't

"Returns SampleResult::None if none of the animations are producing a result (e.g. they are in the delay phase with no backwards fill), ..."

::: gfx/layers/AnimationHelper.h:219
(Diff revision 1)
> +   * in delay state), SampleResult::Skipped if the animation values haven't
> +   * changed since the last call of this function, SmapleResult::Sampled if

s/animation values haven't changed/animation output did not change/ ?

::: gfx/layers/AnimationHelper.h:220
(Diff revision 1)
> +   * changed since the last call of this function, SmapleResult::Sampled if
> +   * new animation values were sampled.

s/SmapleResult/SampleResult/

s/if new animation values were sample/if the animation output was updated/ ?

::: gfx/layers/AnimationHelper.cpp:181
(Diff revision 1)
>  
> +    dom::IterationCompositeOperation iterCompositeOperation =
> +        static_cast<dom::IterationCompositeOperation>(
> +          animation.iterationComposite());
> +
> +    if (iEnd == 1 &&

This needs a comment about why we check for iEnd == 1 (basically what you have in the commit message).

(It seems a shame we can only do this optimization in a very limited set of circumstances because it produces a kind of performance cliff. The animation an be performing well and you make it slightly more complicated and suddenly there is a significant change in performance. How hard is it to expand this optimization?)

::: gfx/layers/AnimationHelper.cpp:207
(Diff revision 1)
>  
>      double portion =
>        ComputedTimingFunction::GetPortion(animData.mFunctions[segmentIndex],
>                                           positionInSegment,
>                                       computedTiming.mBeforeFlag);
> +    if (iEnd == 1 &&

Likewise this too needs a comment.

::: gfx/layers/AnimationHelper.cpp:207
(Diff revision 1)
> +    if (iEnd == 1 &&
> +        animData.mSegmentIndexOnLastCompose == segmentIndex &&
> +        !animData.mPortionInSegmentOnLastCompose.IsNull() &&
> +        animData.mPortionInSegmentOnLastCompose.Value() == portion) {
> +      return SampleResult::Skipped;
> +    }

Why do we need this optimization?

I don't think we do this on the main thread do we?

::: gfx/layers/composite/AsyncCompositionManager.cpp:675
(Diff revision 1)
> -                             animationValue);
> +                               animationValue);
> -        } else {
> -          // Set non-animation values in the case there are no in-effect
> -          // animations (i.e. all animations are in delay state or already
> -          // finished with non-forward fill modes).
> +            break;
> +          }
> +          case AnimationHelper::SampleResult::Skipped:
> +            // We don't need to update animation values for this layer since
> +            // the values haven't been changed.

nit: s/haven't been changed/haven't changed/

Comment 15

[Brian Birtles (:birtles)]

Comment on attachment 8969154 [details]
Bug 1454324 - Skip calculating animation value if animation's progress value hasn't been changed since the last composition and if there are no other animations on the same element.

https://reviewboard.mozilla.org/r/237874/#review243618

Discussed this second optimization with hiro. Apparently we don't need it on the main thread because we perform do a similar optimization as part of our regular style processing anyway.

We do need this on the compositor, however, since we don't have the same optimization there and we hit this particular case whenever we use step timing functions (e.g. the tab throbber and any other frame-based animations).

r=me with comments addressed (including, ideally, a comment explaining why we need this optimization).

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8969153 [details]
Bug 1454324 - Set non-animated values to the layer only if there is no running animation.

https://reviewboard.mozilla.org/r/237872/#review243758

::: gfx/layers/composite/AsyncCompositionManager.cpp:666
(Diff revision 1)
>                                                      layer->GetAnimationData(),
>                                                      animationValue,
>                                                      hasInEffectAnimations);
>          if (hasInEffectAnimations) {
>            Animation& animation = animations.LastElement();
>            ApplyAnimatedValue(layer,

ApplyAnimatedValue seems like it will either set the transform properties, or the opacity properties, depending on what kind of animation is running. However we want to make sure both sets of shadow properties are set on the shadow layer. It looks like this patch leaves a gap because in the case where there is an opacity in-effect animation, the shadow transform properties don't get updated and vice-versa. Or am I missing something?

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8969154 [details]
Bug 1454324 - Skip calculating animation value if animation's progress value hasn't been changed since the last composition and if there are no other animations on the same element.

https://reviewboard.mozilla.org/r/237874/#review243780

I don't really know too much about animation specifics to comment on most of this patch. The high-level changes look reasonable enough, although I think my comment on the previous patch still applies - there are cases where we were assigning the shadow properties before which this patch removes. Either that's a bug and need fixing, or it needs documentation as to why it's ok.

::: gfx/layers/composite/AsyncCompositionManager.cpp:674
(Diff revision 1)
> -          // finished with non-forward fill modes).
> +            // We don't need to update animation values for this layer since
> +            // the values haven't been changed.

It might be worth adding an #ifdef DEBUG block here that asserts the animation values haven't changed.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #16)
> Comment on attachment 8969153 [details]
> Bug 1454324 - Set non-animated values to the layer only if there is no
> running animation.
> 
> https://reviewboard.mozilla.org/r/237872/#review243758
> 
> ::: gfx/layers/composite/AsyncCompositionManager.cpp:666
> (Diff revision 1)
> >                                                      layer->GetAnimationData(),
> >                                                      animationValue,
> >                                                      hasInEffectAnimations);
> >          if (hasInEffectAnimations) {
> >            Animation& animation = animations.LastElement();
> >            ApplyAnimatedValue(layer,
> 
> ApplyAnimatedValue seems like it will either set the transform properties,
> or the opacity properties, depending on what kind of animation is running.
> However we want to make sure both sets of shadow properties are set on the
> shadow layer. It looks like this patch leaves a gap because in the case
> where there is an opacity in-effect animation, the shadow transform
> properties don't get updated and vice-versa. Or am I missing something?

Oh, I thought that the layer for opacity animation doesn't apply any transform and the layer for transform animation doesn't apply opacity either.  I will set the non-animating opacity or transform respectively in ApplyAnimatedValue().  Thanks!

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8969154 [details]
Bug 1454324 - Skip calculating animation value if animation's progress value hasn't been changed since the last composition and if there are no other animations on the same element.

https://reviewboard.mozilla.org/r/237874/#review243608

> This needs a comment about why we check for iEnd == 1 (basically what you have in the commit message).
> 
> (It seems a shame we can only do this optimization in a very limited set of circumstances because it produces a kind of performance cliff. The animation an be performing well and you make it slightly more complicated and suddenly there is a significant change in performance. How hard is it to expand this optimization?)

I think it shouldn't be so hard.  We can do the further optimization later (when we ship composite operation?).  I will file a bug for that.

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8969153 [details]
Bug 1454324 - Set non-animated values to the layer only if there is no running animation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237872/diff/1-2/

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8969154 [details]
Bug 1454324 - Skip calculating animation value if animation's progress value hasn't been changed since the last composition and if there are no other animations on the same element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237874/diff/1-2/

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8969154 [details]
Bug 1454324 - Skip calculating animation value if animation's progress value hasn't been changed since the last composition and if there are no other animations on the same element.

https://reviewboard.mozilla.org/r/237874/#review243780

Though I did post a new patch, I am still thinking how to do this assertion.

Comment 23

[Hiroyuki Ikezoe (:hiro)]

The assertion is hit in some cases.
It seems to me that there remains animations in layer even after corresponding CompositorAnimationStorage is cleared.
 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5616dbf3184330cba644c517a2ffda299e9a847&selectedJob=174705958

Comment 24

[Hiroyuki Ikezoe (:hiro)]

There were two bugs.

1) We didn't reset mCompositorAnimationsId when we clear all animation data in AnimationInfo.
2) We didn't clear all animation data in AnimationInfo when LayerTransactionParent receives 'ReleaseLayer'

Though actually those bugs are unrelated to this bug (just un-wallpaper them by the assertion), hope the assertion works as expected (i.e. not being hit) in this try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dfdd3c41ac13947197619dd9885b0c449ffd20f

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Gah, more oranges, but can't reproduce locally...

Comment 26

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8969153 [details]
Bug 1454324 - Set non-animated values to the layer only if there is no running animation.

https://reviewboard.mozilla.org/r/237872/#review244194

Comment 27

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/try/rev/05338b60ee11142a7025ab5034961a94fdd8446b#l1.12 doesn't seem right. The CompositorAnimationStorage is shared between the "root" LayerTransactionParent (for the UI process layer tree) and the cross-process LayerTransactionParents (for the attached content layer trees). So if you clear the whole thing inside a LayerTransactionParent you're going to be clearing it for the other process too.

Comment 28

[Hiroyuki Ikezoe (:hiro)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #27)
> https://hg.mozilla.org/try/rev/05338b60ee11142a7025ab5034961a94fdd8446b#l1.
> 12 doesn't seem right. The CompositorAnimationStorage is shared between the
> "root" LayerTransactionParent (for the UI process layer tree) and the
> cross-process LayerTransactionParents (for the attached content layer
> trees). So if you clear the whole thing inside a LayerTransactionParent
> you're going to be clearing it for the other process too.

Thanks!  I did read your comment about it somewhere else, but totally forgot it!
A new try without the commit looks fine!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8c0b6ecc2d408c35b1792d519c97599e259324f

Comment 29

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1454324 - Reset mCompositorAnimationsId when we remove animations.

We should re-generate a new id when we add new animations to this layer.

Review commit: https://reviewboard.mozilla.org/r/238698/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/238698/

Comment 30

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1454324 - Clear all animation data when we release layer.

Review commit: https://reviewboard.mozilla.org/r/238700/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/238700/

Comment 31

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8969152 [details]
Bug 1454324 - Cache animation TimingParams on the compositor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237870/diff/1-2/

Comment 32

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8969153 [details]
Bug 1454324 - Set non-animated values to the layer only if there is no running animation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237872/diff/2-3/

Comment 33

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8969154 [details]
Bug 1454324 - Skip calculating animation value if animation's progress value hasn't been changed since the last composition and if there are no other animations on the same element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237874/diff/2-3/

Comment 34

[Hiroyuki Ikezoe (:hiro)]

Note that attachment 8969852 [details] fixes below test failures on WebRender
 test_animations_effect_timing_enddelay.html
 test_animations_iterationstart.html
 test_animations_pausing.html
 test_animations_playbackrate.html

Also a bunch of test cases in test_animations_omta.html are fixed by the patch.  There are still 6 test failures in test_animations_omta.html though.  Anyway I am going to enable those test in bug 1455999.

Comment 35

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8969852 [details]
Bug 1454324 - Reset mCompositorAnimationsId when we remove animations.

Hmm, attachment 8969852 [details] seems to break the machinery in nsDisplayOpacity::CreateWebRenderCommands and nsDisplayTransform::CreateWebRenderCommands.  We should reset the id after calling  AddCompositorAnimationsIdForDiscard().

Comment 36

[Hiroyuki Ikezoe (:hiro)]

Another try without attachment 8969852 [details].  It turns out that the attachment didn't need here.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b9121536e7def8a8e5115f549d9f099e78bed26

Comment 37

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8969152 [details]
Bug 1454324 - Cache animation TimingParams on the compositor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237870/diff/2-3/

Comment 38

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8969853 [details]
Bug 1454324 - Clear all animation data when we release layer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238700/diff/1-2/

Comment 39

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8969153 [details]
Bug 1454324 - Set non-animated values to the layer only if there is no running animation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237872/diff/3-4/

Comment 40

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8969154 [details]
Bug 1454324 - Skip calculating animation value if animation's progress value hasn't been changed since the last composition and if there are no other animations on the same element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237874/diff/3-4/

Comment 41

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8969853 [details]
Bug 1454324 - Clear all animation data when we release layer.

https://reviewboard.mozilla.org/r/238700/#review244590

This seems straightforward enough.

Comment 42

[Kartikaya Gupta (email:[e-mail])]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #34)
> Note that attachment 8969852 [details] fixes below test failures on WebRender

FYI I'm less sure about this patch that resets the id. IIRC the id was reused intentionally to avoid churn of some sort, I'd have to dig through the code history to remember more.

Comment 43

[Hiroyuki Ikezoe (:hiro)]

Thanks for the review!

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #42)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #34)
> > Note that attachment 8969852 [details] fixes below test failures on WebRender
> 
> FYI I'm less sure about this patch that resets the id. IIRC the id was
> reused intentionally to avoid churn of some sort, I'd have to dig through
> the code history to remember more.

I noticed that the reason why the id remains on the compositor is that, on testing refresh mode, we proceeds test code before we receive DeleteCompositorAnimations on the compositor.  Anyway it should be fixed in bug 1455999, I don't yet have any good idea to solve it though.

Comment 44

[Hiroyuki Ikezoe (:hiro)]

A final try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7bff66ee3ce695702c5d5012a9819b19421e7c05

Comment 45

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8969152 [details]
Bug 1454324 - Cache animation TimingParams on the compositor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237870/diff/3-4/

Comment 46

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8969853 [details]
Bug 1454324 - Clear all animation data when we release layer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238700/diff/2-3/

Comment 47

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8969153 [details]
Bug 1454324 - Set non-animated values to the layer only if there is no running animation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237872/diff/4-5/

Comment 48

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8969154 [details]
Bug 1454324 - Skip calculating animation value if animation's progress value hasn't been changed since the last composition and if there are no other animations on the same element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237874/diff/4-5/

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a31d1d7403d7
Cache animation TimingParams on the compositor. r=birtles
https://hg.mozilla.org/integration/autoland/rev/ddd9936f539d
Clear all animation data when we release layer. r=kats
https://hg.mozilla.org/integration/autoland/rev/6ba049b00c55
Set non-animated values to the layer only if there is no running animation. r=kats
https://hg.mozilla.org/integration/autoland/rev/b63e4bff951e
Skip calculating animation value if animation's progress value hasn't been changed since the last composition and if there are no other animations on the same element. r=birtles,kats

Comment 50

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/a31d1d7403d7
https://hg.mozilla.org/mozilla-central/rev/ddd9936f539d
https://hg.mozilla.org/mozilla-central/rev/6ba049b00c55
https://hg.mozilla.org/mozilla-central/rev/b63e4bff951e

Comment 51

[Mike Conley (:mconley) (:⚙️)]

Hey hiro, thanks for landing this!

Here's a profile with a tab throbber throbbing at 20fps:

https://perfht.ml/2qUC5GI

I notice that there's still a "composite" label in the Compositor thread every 16ms, though the pattern appears to be: two short composites (~0.15ms), followed by a longer composite (~0.75ms). Is this expected?

Comment 52

[Kartikaya Gupta (email:[e-mail])]

After looking through the code a little more, I'm now worried that the change in the last patch might have introduced a WR regression, although I'm not totally sure how to go about testing it. Basically, the way we do animations in WR is that when we build the display list, we set a PropertyBindingKey [1] on the stacking context. This is like a placeholder that says "this stacking context has a transform, but we will tell you the value of the transform at render time". And then when we want do a render/composite, we provide the corresponding PropertyBindingValue via the UpdateDynamicProperties call at [2].

However, I think what your patch is going to do, is if the sampling step produces SampleResult::Skipped because the value hasn't changed, then it will omit setting the PropertyBindingValue entirely, because the sample animation step will skip setting the transform into `transformArray` at [3]. When this happens, if WR goes to render a frame, it won't find the PropertyBindingValue and it will fall back to some default thing, which for transforms is the identity transform. So the animation might glitch because for a frame in the middle the transform might reset to the identity transform.

I tried to make a test case with a really slow animation to see if it would trigger this but I was unable to. So maybe my test page isn't good enough, or maybe I'm not understanding something. Either way if you have any thoughts on this let me know.

[1] https://searchfox.org/mozilla-central/rev/36dec78aecc40539ecc8d78e91612e38810f963c/gfx/webrender_bindings/src/bindings.rs#1626
[2] https://searchfox.org/mozilla-central/rev/36dec78aecc40539ecc8d78e91612e38810f963c/gfx/layers/wr/WebRenderBridgeParent.cpp#1282
[3] https://searchfox.org/mozilla-central/rev/36dec78aecc40539ecc8d78e91612e38810f963c/gfx/layers/wr/WebRenderBridgeParent.cpp#1276

Comment 53

[Hiroyuki Ikezoe (:hiro)]

(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #51)
> Hey hiro, thanks for landing this!
> 
> Here's a profile with a tab throbber throbbing at 20fps:
> 
> https://perfht.ml/2qUC5GI
> 
> I notice that there's still a "composite" label in the Compositor thread
> every 16ms, though the pattern appears to be: two short composites
> (~0.15ms), followed by a longer composite (~0.75ms). Is this expected?

Yes, that's what I mentioned in comment 9.  This optimization can skip calculation for animations but not skip composition itself.  I have an idea to skip the composition in the case where there needs composition only for unchanged animations and if the composition isn't triggered by non-animation.  But it would get more stuff involved.

Comment 54

[Hiroyuki Ikezoe (:hiro)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #52)
> After looking through the code a little more, I'm now worried that the
> change in the last patch might have introduced a WR regression, although I'm
> not totally sure how to go about testing it. Basically, the way we do
> animations in WR is that when we build the display list, we set a
> PropertyBindingKey [1] on the stacking context. This is like a placeholder
> that says "this stacking context has a transform, but we will tell you the
> value of the transform at render time". And then when we want do a
> render/composite, we provide the corresponding PropertyBindingValue via the
> UpdateDynamicProperties call at [2].
> 
> However, I think what your patch is going to do, is if the sampling step
> produces SampleResult::Skipped because the value hasn't changed, then it
> will omit setting the PropertyBindingValue entirely, because the sample
> animation step will skip setting the transform into `transformArray` at [3].
> When this happens, if WR goes to render a frame, it won't find the
> PropertyBindingValue and it will fall back to some default thing, which for
> transforms is the identity transform. So the animation might glitch because
> for a frame in the middle the transform might reset to the identity
> transform.
> 
> I tried to make a test case with a really slow animation to see if it would
> trigger this but I was unable to. So maybe my test page isn't good enough,
> or maybe I'm not understanding something. Either way if you have any
> thoughts on this let me know.
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> 36dec78aecc40539ecc8d78e91612e38810f963c/gfx/webrender_bindings/src/bindings.
> rs#1626
> [2]
> https://searchfox.org/mozilla-central/rev/
> 36dec78aecc40539ecc8d78e91612e38810f963c/gfx/layers/wr/WebRenderBridgeParent.
> cpp#1282
> [3]
> https://searchfox.org/mozilla-central/rev/
> 36dec78aecc40539ecc8d78e91612e38810f963c/gfx/layers/wr/WebRenderBridgeParent.
> cpp#1276

Thank you, kats!  This explanation really helps to understand WebRender stuff for me.  It sounds like bug 1456679 might introduce the regression, but at first glance, it won't since we just skip *updating* the previous animation values and the previous values are preserved and used actually.  I am going to check the code from now on.

Comment 55

[Jason Mechelynck]

Sorry didn't want to create a new bug since not sure what's going on here

can you guys look into this profiles

20/30 FPS throbbers freaking out and slow performance of page loads but still better than the default ones
Even when loading 10-20 pages which is very common 

Bad
https://perfht.ml/2r2GE0L
Worse
https://perfht.ml/2r0qi8I
Worst with WR
https://perfht.ml/2r39qOI
Something is very wrong with the new svg throbbers
circling ones were better

Comment 56

[Jason Mechelynck]

https://perfht.ml/2r5T3kK

HW accleration off

https://perfht.ml/2r0u2ag

Comment 57

[Kartikaya Gupta (email:[e-mail])]

(In reply to Jason Mechelynck from comment #55)
> 20/30 FPS throbbers freaking out and slow performance of page loads but
> still better than the default ones

Can you explain what you mean a little more clearly? How are you measuring the FPS? Is the page load slower than it was before (i.e. a regression)? How did you decide this patch/bug was responsible?

Comment 58

[Hiroyuki Ikezoe (:hiro)]

I think the FPS means the preference names [1] for throbbers on tabs.  But I don't see any suspicious on the compositor side in the profiles.

[1] https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/browser/app/profile/firefox.js#457

Comment 59

[Jason Mechelynck]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #58)
> I think the FPS means the preference names [1] for throbbers on tabs.  But I
> don't see any suspicious on the compositor side in the profiles.
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> 78dbe34925f04975f16cb9a5d4938be714d41897/browser/app/profile/firefox.js#457

Yes this

Comment 60

[Thinker Li [:sinker]]

I got following error when visit any instagram page.

Assertion failure: transform->mTransform.mTransformInDevSpace.FuzzyEqualsMultiplicative( (layer->AsHostLayer()->GetShadowBaseTransform())), at /home/thinker/progm/mozilla-central/gfx/layers/composite/AsyncCompositionManager.cpp:696

For example, https://www.instagram.com/explore/locations/49695104/brooklyn-bridge/
The process crashed when I scrolled down the page.

Comment 61

[Hiroyuki Ikezoe (:hiro)]

Hi Thinker!

(In reply to Thinker Li [:sinker] from comment #60)
> I got following error when visit any instagram page.
> 
> Assertion failure:
> transform->mTransform.mTransformInDevSpace.FuzzyEqualsMultiplicative(
> (layer->AsHostLayer()->GetShadowBaseTransform())), at
> /home/thinker/progm/mozilla-central/gfx/layers/composite/
> AsyncCompositionManager.cpp:696

That's bug 1457249.  I will land patches for the bug soon.