Closed Bug 1301305 Opened 8 years ago Closed 7 years ago

graphical gap between menu close and viewport expand animations on trello.com because of async transform animation

Categories

(Core :: DOM: Animation, defect, P3)

49 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- wontfix
firefox53 --- fixed

People

(Reporter: karlt, Assigned: birtles)

References

Details

(Keywords: regression)

Attachments

(13 files, 4 obsolete files)

58 bytes, text/x-review-board-request
hiro
: review+
Details
58 bytes, text/x-review-board-request
hiro
: review+
Details
58 bytes, text/x-review-board-request
hiro
: review+
Details
58 bytes, text/x-review-board-request
hiro
: review+
Details
58 bytes, text/x-review-board-request
pbro
: review+
Details
58 bytes, text/x-review-board-request
flod
: review+
hiro
: review+
Details
58 bytes, text/x-review-board-request
hiro
: review+
Details
58 bytes, text/x-review-board-request
hiro
: review+
Details
58 bytes, text/x-review-board-request
hiro
: review+
Details
58 bytes, text/x-review-board-request
hiro
: review+
Details
58 bytes, text/x-review-board-request
hiro
: review+
Details
58 bytes, text/x-review-board-request
hiro
: review+
Details
4.24 KB, patch
Details | Diff | Splinter Review
The page needs an account to view.

1. Uncheck "Enable multi-process Nightly"
2. Uncheck "Use hardware acceleration [sic] when available"
3. Load a board with a number of lists so that they do not all fit in one
   viewport.  e.g. https://trello.com/b/qxgTGTHC/ if you have access
4. Click "Show menu", top-right.
5. After menu opens, close menu with click on X.

Expected results, as with build prior to changes for bug 1258904:
On a fast-enough system, the viewport for the lists expands so that lists are
seen where the menu was as it slides closed.

Actual results, as with build after changes for bug 1258904
https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-linux64-pgo/1462674743/:
Menu slides closed and viewport expansion follows with a delay, so that background is seen first and then lists appear where the menu was.

Behavior with e10s enabled prior to changes for bug 1258904 is
similar to behavior after changes for bug 1258904.

Bug 1301194 further regressed behavior on Linux, so use a build unaffected by that or a build prior to
https://hg.mozilla.org/integration/mozilla-inbound/rev/3de4044b57f4.
Keywords: perf, regression
Summary: lag between menu close and viewport expand animations on trello. → lag between menu close and viewport expand animations on trello.com
There is a transform animation when opening/closing the menu.  On firefox48 (which does not include the fix for bug 1258903) if devtools shows correctly 'running on the compositor' state, the transform is not running on the compositor.  Whereas on today's nightly it is running on the compositor.

So I think the problem is not a performance issue (actually the performance has been improved because the transform animation is running on the compositor), the problem is graphical gap caused by asynchronous transform animations and scrolling content.  We synchronize asynchronous transform animations on every 200ms [1] to the main thread, in other words, transform animations on the compositor and scrolling content causes by the transform don't synchronize during the period (200ms).

[1] http://hg.mozilla.org/mozilla-central/file/77940cbf0c2a/dom/animation/KeyframeEffectReadOnly.cpp#l851

I have no idea how to solve this gap.
Keywords: perf
Summary: lag between menu close and viewport expand animations on trello.com → graphical gap between menu close and viewport expand animations on trello.com because of async transform animation
(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> There is a transform animation when opening/closing the menu.  On firefox48
> (which does not include the fix for bug 1258903) if devtools shows correctly

Sorry for the wrong bug.  I meant bug1258904 of course.
Confirmed that on nightly the graphical gap is disappeared if layers.offmainthreadcomposition.async-animations is false.
See Also: → 1303649
Yes, this is definitely caused by trying to animate transform and margin-right at the same time. Unfortunately the target elements differ so we can't easily detect this case and, for example, disable using compositor animations for this.

Long-term, I think we may one or both of the following:

1. Introduce a CSS property to disable running animations on the compositor (or, more likely, something that says 'these animations must be synchronized' so either all run on the compositor or none do).

2. Introduce timing groups in which case animations like this could be put in the same timing group giving the browser some hint that it should synchronize the components.

I'm going to mark this as WONTFIX for now since we don't have any concrete intention of doing either of the above soon. That said, the bug report is very helpful since it's yet another data point in coming to recognize that we probably do need a way to synchronize animations.

If trello want to work around this in the mean time, they could try to force the transform animation to the main thread by, e.g. adding a small animation of a geometric property (although that probably only works for Firefox).
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
Is the work to deal with this case tracked somewhere other than in this bug? This is a real web page that regressed in Firefox and works well in other browsers (at least Chrome). Leaving this as wontfix isn't really an option for us, so thus reopening this bug until we can decide on a path forward here.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Though I don't know much about our APZ, can we manage to scroll content on the compositor if transform on descendant elements causes overflow? kats, any thoughts?
Flags: needinfo?(bugmail)
(In reply to Johnny Stenback  (:jst, jst@mozilla.com) from comment #5)
> Is the work to deal with this case tracked somewhere other than in this bug?
> This is a real web page that regressed in Firefox and works well in other
> browsers (at least Chrome). Leaving this as wontfix isn't really an option
> for us, so thus reopening this bug until we can decide on a path forward
> here.

Not yet. It's something I spoke to Google about last week but it's going to be a while before we get anything like this standardized and shipped.

We can try to extend our heuristic for turning off compositor animations to extend to simultaneously triggered animations where some affect transform and other affect geometric properties like margin (we already do this within a single element) but it will add considerable complexity and cause other pages to degrade in performance. We can back out the offending bug but that will also cause other pages to degrade in performance.

So I think we really need to add platform features for this (or just improve layout performance such that it closes the gap). Other than that we can try to contact trello to help them tweak their content.

For what it's worth, Edge shows the same behavior as Firefox here.
Priority: -- → P5
Priority: P5 → P3
I've been thinking about this a bit more, and maybe doing a document wide check for animations starting at the same time wouldn't be so expensive and might not regress existing content in a noticeable manner.

I'm thinking we could have a simple nsCSSPropertyIDSet on the EffectCompositor that tracks properties affected by animations on which we've called Play() within the current frame. If we detect a situation where we have both geometry and transform properties being animated, we could annotate those animations with a flag that says "don't run on the compositor" (which, perhaps we'd clear on a call to something like a call to Pause()).

Perhaps during display list building at the point where we go to prepare the animations for the compositor (i.e. GetAnimationForCompositor) we could check the EffectCompositor and set the flag then? We'd still need the flag to prevent sending the animation to the compositor on the next frame.

Anyway, I'll have a bit more of a think about it. If that works then it just remains to be seen how many sites this produces a noticeable performance drop on. Codrops has a lot of example app-style animations[1] that mix transform animations with others so that might be a good starting point for weighing it up.

[1] http://tympanus.net/codrops/
(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> Though I don't know much about our APZ, can we manage to scroll content on
> the compositor if transform on descendant elements causes overflow? kats,
> any thoughts?

Sorry, I'm not really clear on what you are asking. From reading through the bug, my understanding is that there are two animations on the page that are intended to run in sync. One is for the transform property and one is for margin-right. When the transform animation runs on the compositor it gets out of sync with the margin-right which is running on the main thread and so a gap appears. Is that correct? If so I don't see how APZ would help here as that only deals with scrolling of content and not really animation of CSS properties. If I'm misunderstanding please correct me. A reduced example page may also help.
Flags: needinfo?(bugmail)
As a slight refinement on my proposal in comment 8, it might even be
enough just to store a single TimeStamp on the EffectCompositor (which
is per-prescontext) that represents "last frame where we started an
animation which affects a geometric property" (using the refresh driver
time).

We'd set that flag inside Animation::PlayNoUpdate when we have geometric
properties (we might need some special handling to cover the case where
we call Play() followed by SetKeyframes()).

Then when we get a call to FindAnimationsForCompositor we check:

1. Is |aProperty| == eCSSProperty_transform?
2. Is the effect newly-started?

   There are a couple of possibilities here:

   (a) We just triggered it and are waiting to fill in its start time,
       i.e. Animation::mPendingState == PendingState::PlayPending &&
            Animation::mPendingReadyTime.IsNull()
   (b) We started it with a fixed start time---we can't really test this
       without adding extra state I think (we could try to compare, for
       example, Animation::mStartTime == current refresh driver time but
       then we'll trigger this condition if we seek backwards past the
       start time).

   I suspect we don't need to worry about (b), however, at least
   not for content using CSS animations/transitions since that always
   follows the behavior in (a).

3. Does EffectCompositor::mLastFrameWithNewGeometricAnimations ==
   current refresh driver time?

And if all of those are true, we set the flag on the effect that says
"async transform animation blocked by simultaneous geometric
properties". (If that flag is true, we don't return the effect from
FindAnimationsForCompositor when aProperty == transform, and instead set
an appropriate AnimationPerformanceWarning.)
Incidentally, I notice Blink have an almost identical bug[1] (which was marked as WONTFIX) where they came to the same conclusion as I did in comment 7.

> "On the topic of synchronising animations we ensure all property
> animations in @keyframes rules and element.animate() run on the same
> thread for synchronisation purposes (if any of them are main thread,
> all of them become main thread).
> We could force the same behaviour for transitions that start at the
> same time however I don't think pulling things off the compositor
> would be desirable in most cases especially considering the default
> value of transition-property is "all"."

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=598396

The approach I'm suggesting here, however, limits this to pulling transform animations off the compositor when geometric properties are in use.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #9)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> > Though I don't know much about our APZ, can we manage to scroll content on
> > the compositor if transform on descendant elements causes overflow? kats,
> > any thoughts?
> 
> Sorry, I'm not really clear on what you are asking. From reading through the
> bug, my understanding is that there are two animations on the page that are
> intended to run in sync. One is for the transform property and one is for
> margin-right. When the transform animation runs on the compositor it gets
> out of sync with the margin-right which is running on the main thread and so
> a gap appears. Is that correct? If so I don't see how APZ would help here as
> that only deals with scrolling of content and not really animation of CSS
> properties. If I'm misunderstanding please correct me. A reduced example
> page may also help.

Thank you kats.  I was confused.  Last time I saw the trello board, I did not notice the margin-right animations.  So my question does not make sense at all.
Assignee: nobody → bbirtles
Status: REOPENED → ASSIGNED
Attached patch WIP patch (obsolete) — Splinter Review
Attaching a WIP patch since I don't have time to work on this right now.

With this patch the animations when the panel comes in are successfully synchronized, but not when the panel goes out. It seems like the CurrentFrameHasNewGeometricAnimations() check might return false in that case? Perhaps we generate the transitions but don't call FindAnimationsForCompositor until the next refresh driver tick in that case?
Attached patch WIP patch (obsolete) — Splinter Review
Drop some unrelated changes from previous patch
Attachment #8803210 - Attachment is obsolete: true
(In reply to Brian Birtles (:birtles, high review load) from comment #13)
> Created attachment 8803210 [details] [diff] [review]
> WIP patch
> 
> Attaching a WIP patch since I don't have time to work on this right now.
> 
> With this patch the animations when the panel comes in are successfully
> synchronized, but not when the panel goes out. It seems like the
> CurrentFrameHasNewGeometricAnimations() check might return false in that
> case? Perhaps we generate the transitions but don't call
> FindAnimationsForCompositor until the next refresh driver tick in that case?

Yeah, very likely.  We will need to call MaybeNotifyNewlyStartedGeometricAnimation() in Tick(). But if so, I don't understand why coming-in panel works fine.

As far as I can tell, if setting transition property (i.e. style.transform = 'xxx') in rAF callback, attachment 8803211 [details] [diff] [review] will work fine, but if setting the property in other callbacks (e.g. mouse event or something), attachment 8803211 [details] [diff] [review] won't work.
I am sorry, comment 15 is somewhat misleading.  Animations on trello.com are transition.  Basically transition should be initiated in Tick(), in this case the time stamp is the same when we call FindAnimationsForCompositor().  But if there is a JS code that causes style flush (e.g. getComputedStyle(element).transform or getAnimations()), the transitions is initiated in a prior frame.  In such cases attachment 8803211 [details] [diff] [review] won't work.
Attached patch WIP attempt 2 (obsolete) — Splinter Review
This should fix the problem when the transition is created before we tick. I've checked that for Trello both the in and out animations are synchronized with this patch.

I still need to:

* Tidy up and split patches (the warning message text probably needs work too)
* Add tests and adjust existing tests
* Add some logging and browse through animation-heavy content (e.g. codrops) and see how often we hit this and what impact it has (e.g. are there any sites with notable performance regressions)
Attachment #8803211 - Attachment is obsolete: true
Attached patch WIP attempt 3 (obsolete) — Splinter Review
Just updating this patch so I can refer to it in bug 1305325.
Attachment #8809223 - Attachment is obsolete: true
Please do a try run for Android too. test_animation_performance_warning.html might be timed out.
Attachment #8814307 - Attachment is obsolete: true
Comment on attachment 8816035 [details]
Bug 1301305 - Add a performance warning type for transform animations that should be synchronized with geometric animations;

https://reviewboard.mozilla.org/r/96814/#review97334

::: dom/locales/en-US/chrome/layout/layout_errors.properties:37
(Diff revision 2)
>  ##                   CompositorAnimationWarningTransformFrameInactive,
>  ##                   CompositorAnimationWarningOpacityFrameInactive):
>  ## 'transform' and 'opacity' mean CSS property names, don't translate it.
>  CompositorAnimationWarningTransformSVG=Animations of ‘transform’ on elements with SVG transforms cannot be run on the compositor
>  CompositorAnimationWarningTransformWithGeometricProperties=Animations of ‘transform’ cannot be run on the compositor when geometric properties are animated on the same element at the same time
> +CompositorAnimationWarningTransformWithSyncGeometricAnimations=Animation of ‘transform’ cannot be run on the compositor because it should be synchronized with animations of geometric properties that started at the same time

I'm not really sure about this key name 'WithSyncGeometricAnimations' is supposed to mean "with synchronized geometric animations". I wonder if that makes sense to others?
Comment on attachment 8816039 [details]
Bug 1301305 - Add a member to track if an animation needs to be synchronized with geometric animations or not;

https://reviewboard.mozilla.org/r/96822/#review97336

::: dom/animation/Animation.cpp:993
(Diff revision 2)
> +  KeyframeEffectReadOnly* keyframeEffect = mEffect->AsKeyframeEffect();
> +  if (!keyframeEffect ||
> +      !keyframeEffect->HasAnimationOfProperty(eCSSProperty_transform)) {
> +    return;
> +  }
> +

I think we could actually drop this code and just set mSyncWithGeometricAnimations unconditionally on all animations. We check for a transform property before using it anyway.
Comment on attachment 8816030 [details]
Bug 1301305 - Move test data alongside the test function;

https://reviewboard.mozilla.org/r/96804/#review97352

::: dom/animation/test/chrome/test_animation_performance_warning.html:836
(Diff revision 2)
> +    },
> +  ].forEach(function(subtest) {

Nit: Let's use arrow function here as well.  We might want to use arrow function all over the place at some point.
Attachment #8816030 - Flags: review?(hiikezoe) → review+
Comment on attachment 8816031 [details]
Bug 1301305 - Rename assert_property_state_on_compositor to assert_all_properties_running_on_compositor;

https://reviewboard.mozilla.org/r/96806/#review97354
Attachment #8816031 - Flags: review?(hiikezoe) → review+
Comment on attachment 8816032 [details]
Bug 1301305 - Move propertyToIDL to testcommon.js;

https://reviewboard.mozilla.org/r/96808/#review97356

::: dom/animation/test/testcommon.js:121
(Diff revision 2)
> +  if (prefixMatch) {
> +    var prefix = prefixMatch[1] === "moz" ? "Moz" : prefixMatch[1];

Nit: Please use single quote instead of double quote.

::: dom/animation/test/testcommon.js:207
(Diff revision 2)
>      opener.done();
>    }
>  }
>  
>  /**
> - * Return a new MutaionObserver which started observing |target| element
> + * Return a new MutationObserver which started observing |target| element

Thanks for the correction!
Attachment #8816032 - Flags: review?(hiikezoe) → review+
Comment on attachment 8816033 [details]
Bug 1301305 - Expand the set of geometric properties to include margin and padding properties;

https://reviewboard.mozilla.org/r/96810/#review97360

::: dom/animation/test/chrome/test_animation_performance_warning.html:306
(Diff revision 2)
> +      const keyframes = {};
> +      keyframes[propertyToIDL(property)] = [ '100px', '200px' ];
> +      keyframes.transform = [ 'translate(0px)', 'translate(100px)' ];
> +

Can't we initialize this |keyframes| with computed property names?
Attachment #8816033 - Flags: review?(hiikezoe) → review+
Comment on attachment 8816035 [details]
Bug 1301305 - Add a performance warning type for transform animations that should be synchronized with geometric animations;

https://reviewboard.mozilla.org/r/96814/#review97364
Attachment #8816035 - Flags: review?(hiikezoe) → review+
Comment on attachment 8816041 [details]
Bug 1301305 - Add tests for transform animations synchronized with geometric animations:

https://reviewboard.mozilla.org/r/96826/#review97366

I wonder there is no simple test case such that two animations starting on different frames are NOT synchronized at all.

::: dom/animation/test/testcommon.js:143
(Diff revision 2)
> +function waitForIdleCallback() {
> +  return new Promise(function(resolve, reject) {
> +    window.requestIdleCallback(resolve);
> +  });
> +}

Nice!
Attachment #8816041 - Flags: review?(hiikezoe) → review+
Comment on attachment 8816036 [details]
Bug 1301305 - Add AnimationEffectReadOnly::AffectsGeometry() helper to identify effects that animate geometric properties;

https://reviewboard.mozilla.org/r/96816/#review97370

Actually I prefer this kind of wrapper function, but I've heard that Animation class takes on the role of timing specific process (I like this rule too).  I think AnimatesGeometry() breaks this rule, it that OK?

::: dom/animation/KeyframeEffectReadOnly.cpp:1258
(Diff revision 2)
> +KeyframeEffectReadOnly::HasGeometricProperties() const
> +{
> +  for (const AnimationProperty& property : mProperties) {
> +    if (IsGeometricProperty(property.mProperty)) {
> +      return true;
> +    }
> +  }
> +
> +  return false;
> +}

Don't we need to check mTarget here?  I guess no target geometric animation blocks transform.
Attachment #8816036 - Flags: review?(hiikezoe) → review+
Comment on attachment 8816036 [details]
Bug 1301305 - Add AnimationEffectReadOnly::AffectsGeometry() helper to identify effects that animate geometric properties;

https://reviewboard.mozilla.org/r/96816/#review97370

> Don't we need to check mTarget here?  I guess no target geometric animation blocks transform.

Ah, no target animation is not added in play pending queue, right?
Comment on attachment 8816036 [details]
Bug 1301305 - Add AnimationEffectReadOnly::AffectsGeometry() helper to identify effects that animate geometric properties;

https://reviewboard.mozilla.org/r/96816/#review97370

> Ah, no target animation is not added in play pending queue, right?

But we can nullify the target soon after play().  Hmm, I would like to see the test result.
Comment on attachment 8816037 [details]
Bug 1301305 - Extend PendingAnimationTracker to mark play-pending animations if there are geometric animations starting at the same time;

https://reviewboard.mozilla.org/r/96818/#review97388

::: dom/animation/PendingAnimationTracker.h:89
(Diff revision 2)
> +  mutable CheckState mHasPlayPendingGeometricAnimations =
> +    CheckState::Indeterminate;
>  };

I actually don't think using mutable is reasonable here.  I didn't know that we have already such code though.

::: dom/animation/PendingAnimationTracker.cpp:121
(Diff revision 2)
> +
> +  mHasPlayPendingGeometricAnimations = CheckState::Absent;
> +  for (auto iter = mPlayPendingSet.ConstIter(); !iter.Done(); iter.Next()) {
> +    if (iter.Get()->GetKey()->AnimatesGeometry()) {
> +      mHasPlayPendingGeometricAnimations = CheckState::Present;
> +      break;

Can't we return immediately here?
Attachment #8816037 - Flags: review?(hiikezoe) → review+
Comment on attachment 8816038 [details]
Bug 1301305 - Factor out check for main-thread synchronization to a method on Animation;

https://reviewboard.mozilla.org/r/96820/#review97394

::: dom/animation/Animation.cpp:807
(Diff revision 2)
> +  KeyframeEffectReadOnly* keyframeEffect = mEffect
> +                                           ? mEffect->AsKeyframeEffect()
> +                                           : nullptr;

Now it's time to have a wrapper function of this?
Attachment #8816038 - Flags: review?(hiikezoe) → review+
Comment on attachment 8816037 [details]
Bug 1301305 - Extend PendingAnimationTracker to mark play-pending animations if there are geometric animations starting at the same time;

https://reviewboard.mozilla.org/r/96818/#review97388

> I actually don't think using mutable is reasonable here.  I didn't know that we have already such code though.

It's quite common in our code base[1], and it's particularly well suited to this sort of situation where you have a const method that is caching a return value.

[1] http://searchfox.org/mozilla-central/search?q=%5CWmutable%5CW&case=true&regexp=true&path=.h

> Can't we return immediately here?

Well, we jump straight to the return statement which seems as good as returning. I think if we return true immediately we'd make it easier to introduce a bug by returning true and forgetting to update mHasPlayPendingGeometricAnimations.
(In reply to Brian Birtles (:birtles) from comment #58)
> Comment on attachment 8816037 [details]
> Bug 1301305 - Extend PendingAnimationTracker to report if it contains
> play-pending geometric animations;
> 
> https://reviewboard.mozilla.org/r/96818/#review97388
> 
> > I actually don't think using mutable is reasonable here.  I didn't know that we have already such code though.
> 
> It's quite common in our code base[1], and it's particularly well suited to
> this sort of situation where you have a const method that is caching a
> return value.
> 
> [1]
> http://searchfox.org/mozilla-central/
> search?q=%5CWmutable%5CW&case=true&regexp=true&path=.h
> 
> > Can't we return immediately here?
> 
> Well, we jump straight to the return statement which seems as good as
> returning. I think if we return true immediately we'd make it easier to
> introduce a bug by returning true and forgetting to update
> mHasPlayPendingGeometricAnimations.

I think it's an adverse effect of mutable variable in const function.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #59)
> I think it's an adverse effect of mutable variable in const function.

I don't understand, the mutable variable is for caching the result. What is the adverse effect?
(In reply to Brian Birtles (:birtles) from comment #60)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #59)
> > I think it's an adverse effect of mutable variable in const function.
> 
> I don't understand, the mutable variable is for caching the result. What is
> the adverse effect?

Because the mutable label sits only header file, when reading the const function, people hard to notice that there is mutable variables without mutex, at least for me.
I don't think we make any assumptions about const methods being thread-safe in our code base.

(Whereas using mutable for memoization is common.)
Comment on attachment 8816039 [details]
Bug 1301305 - Add a member to track if an animation needs to be synchronized with geometric animations or not;

https://reviewboard.mozilla.org/r/96822/#review97400

::: dom/animation/Animation.cpp:1068
(Diff revision 2)
>    }
>  
>    mPendingState = PendingState::PlayPending;
>  
> +  // Clear flag that causes us to sync transform animations with the main
> +  // thread for now. We'll set this when the go to set up compositor

when we go to set?
Attachment #8816039 - Flags: review?(hiikezoe) → review+
Comment on attachment 8816039 [details]
Bug 1301305 - Add a member to track if an animation needs to be synchronized with geometric animations or not;

https://reviewboard.mozilla.org/r/96822/#review97336

> I think we could actually drop this code and just set mSyncWithGeometricAnimations unconditionally on all animations. We check for a transform property before using it anyway.

Then, let's add an assertion to avoid calling this accidentaly in future.
Comment on attachment 8816035 [details]
Bug 1301305 - Add a performance warning type for transform animations that should be synchronized with geometric animations;

https://reviewboard.mozilla.org/r/96814/#review97334

> I'm not really sure about this key name 'WithSyncGeometricAnimations' is supposed to mean "with synchronized geometric animations". I wonder if that makes sense to others?

I am also not sure about the key name, but the message is really easy to understand what's going on in that cases.
Comment on attachment 8816039 [details]
Bug 1301305 - Add a member to track if an animation needs to be synchronized with geometric animations or not;

https://reviewboard.mozilla.org/r/96822/#review97336

> Then, let's add an assertion to avoid calling this accidentaly in future.

I mean we should just drop this check. We will still call this for animations that don't animate transform so I don't think we can add an assertion.
Comment on attachment 8816040 [details]
Bug 1301305 - Notify animations when they should synchronize with geometric animations;

https://reviewboard.mozilla.org/r/96824/#review97406

::: dom/animation/EffectCompositor.cpp:82
(Diff revision 2)
> +    PendingAnimationTracker* tracker =
> +      aFrame->PresContext()->Document()->GetPendingAnimationTracker();
> +    if (tracker && tracker->HasPlayPendingGeometricAnimations()) {
> +      for (KeyframeEffectReadOnly* effect : *effects) {
> +        MOZ_ASSERT(effect && effect->GetAnimation());
> +        effect->GetAnimation()->NotifyGeometricAnimationsStartingThisFrame();
> +      }

This could be a member function (static function) of PendingAnimationTracker.
Attachment #8816040 - Flags: review?(hiikezoe) → review+
Comment on attachment 8816039 [details]
Bug 1301305 - Add a member to track if an animation needs to be synchronized with geometric animations or not;

https://reviewboard.mozilla.org/r/96822/#review97336

> I mean we should just drop this check. We will still call this for animations that don't animate transform so I don't think we can add an assertion.

Ah, I see.  Now your comment noticed me that we should clear mSyncWithGeometricAnimations flag when we change playback rate.  I think we should clear it at least for zero playback rate, no?
Comment on attachment 8816036 [details]
Bug 1301305 - Add AnimationEffectReadOnly::AffectsGeometry() helper to identify effects that animate geometric properties;

https://reviewboard.mozilla.org/r/96816/#review97414

::: dom/animation/KeyframeEffectReadOnly.cpp:1258
(Diff revision 2)
> +KeyframeEffectReadOnly::HasGeometricProperties() const
> +{
> +  for (const AnimationProperty& property : mProperties) {
> +    if (IsGeometricProperty(property.mProperty)) {
> +      return true;
> +    }
> +  }
> +
> +  return false;
> +}

I think if we want to check that, we should add it to Animation::AnimatesGeometry or a different method on this class. This method is called HasGeometricProperties() so that's all it should report.
(In reply to Brian Birtles (:birtles) from comment #69)
> I think if we want to check that, we should add it to
> Animation::AnimatesGeometry or a different method on this class. This method
> is called HasGeometricProperties() so that's all it should report.

Oops, that was supposed to be a response to comment 53.
Comment on attachment 8816039 [details]
Bug 1301305 - Add a member to track if an animation needs to be synchronized with geometric animations or not;

https://reviewboard.mozilla.org/r/96822/#review97336

> Ah, I see.  Now your comment noticed me that we should clear mSyncWithGeometricAnimations flag when we change playback rate.  I think we should clear it at least for zero playback rate, no?

It turns out we do need this. Otherwise, for example, while looking for async opacity animations, we'll get here and if there's a transform animation with the flag set, we'll stop the opacity animation from running on the compositor.
Comment on attachment 8816039 [details]
Bug 1301305 - Add a member to track if an animation needs to be synchronized with geometric animations or not;

https://reviewboard.mozilla.org/r/96822/#review97336

> It turns out we do need this. Otherwise, for example, while looking for async opacity animations, we'll get here and if there's a transform animation with the flag set, we'll stop the opacity animation from running on the compositor.

Also, on further thought, I think it's better if we check this in Animation::ShouldBeSynchronizedWithMainThread since that means at least we can respond to changes in the properties on the animation itself (we can't easily respond to changes in the geometric animations that caused this to be synchronized, however).
Attached patch Logging patchSplinter Review
I did a bit of logging to see how often this crops up in practice but only hit the following URLs:

(Trello)
http://tympanus.net/Tutorials/PolaroidStackGrid/
https://twitter.com/i/notifications (this turned out to be a false positive due to some changes I made but then reverted -- see comment 71)

For my own reference, after building with this patch I ran:

MOZCONFIG=.mozconfig-opt MOZ_LOG="Trello:5,timestamp,sync" MOZ_LOG_FILE="log.txt" mach run --disable-e10s
Comment on attachment 8816035 [details]
Bug 1301305 - Add a performance warning type for transform animations that should be synchronized with geometric animations;

https://reviewboard.mozilla.org/r/96814/#review97434

Nothing wrong with the l10n part.
Attachment #8816035 - Flags: review?(francesco.lodolo) → review+
Regarding the PolaroidStackGrid example we have:

* A spinner on a pseudo element that animates the height property. I think this is where the box fills up. (This keeps running forever despite being covered up).
* At the same time there is a transform animation on the down button that also runs forever.

The two animations have the same start time but the second one has a delay. I'd suggest that for this particular content synchronization is not important and is actually a performance regression. However a few notes:

* Even if we synchronize on start time we wouldn't be able to avoid this case because it is using delay.
* Trying to detect animations that should be synchronized based on delay seems problematic.
* It seems like this content could be rewritten to use scale instead of height to workaround this (i.e. the DevTools warning would be quite helpful to the author here).

So I'm inclined to think this optimization is worth trying. We can land it on nightly and see what sort of bug reports come in.
Thanks for all the great review feedback. I haven't worked it all in yet (still thinking about how to approach some of it) but I'm going to update the patches here anyway just so I can pick them up on another machine.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #53)
> Comment on attachment 8816036 [details]
> Bug 1301305 - Add Animation::AnimatesGeometry() helper to identify
> animations that animate geometric properties;
> 
> https://reviewboard.mozilla.org/r/96816/#review97370
> 
> Actually I prefer this kind of wrapper function, but I've heard that
> Animation class takes on the role of timing specific process (I like this
> rule too).  I think AnimatesGeometry() breaks this rule, it that OK?

I think that's what the classes do, but it's ok to add wrapper functions when appropriate (we already have HasCurrentEffect/IsInEffect etc.). For now I've moved this helper to AnimationEffectReadOnly (which probably makes more sense since we'll need to overload it for group effects anyway).

> ::: dom/animation/KeyframeEffectReadOnly.cpp:1258
> (Diff revision 2)
> > +KeyframeEffectReadOnly::HasGeometricProperties() const
> > +{
> > +  for (const AnimationProperty& property : mProperties) {
> > +    if (IsGeometricProperty(property.mProperty)) {
> > +      return true;
> > +    }
> > +  }
> > +
> > +  return false;
> > +}
> 
> Don't we need to check mTarget here?  I guess no target geometric animation
> blocks transform.

I'm not sure this is particularly important (since playing animations with geometric properties set and no target element seems unlikely) but I've added it for now along with a test in the final patch.

(In reply to Brian Birtles (:birtles) from comment #62)
> I don't think we make any assumptions about const methods being thread-safe
> in our code base.

While I don't think we need this to be non-const in this case (we're not using STL containers or accessing from multiple threads), I've marked it as non-const now as part of the refactoring mentioned below.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #57)
> ::: dom/animation/Animation.cpp:807
> (Diff revision 2)
> > +  KeyframeEffectReadOnly* keyframeEffect = mEffect
> > +                                           ? mEffect->AsKeyframeEffect()
> > +                                           : nullptr;
> 
> Now it's time to have a wrapper function of this?

I think that if we find we're doing this a lot, we should consider moving methods to AnimationEffectReadOnly. In this particular case, since we reference |keyframeEffect| twice, however, putting the methods on AnimationEffectReadOnly would mean we'd have two virtual method calls instead of once so I've left this instance as it is for now.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #67)
> ::: dom/animation/EffectCompositor.cpp:82
> (Diff revision 2)
> > +    PendingAnimationTracker* tracker =
> > +      aFrame->PresContext()->Document()->GetPendingAnimationTracker();
> > +    if (tracker && tracker->HasPlayPendingGeometricAnimations()) {
> > +      for (KeyframeEffectReadOnly* effect : *effects) {
> > +        MOZ_ASSERT(effect && effect->GetAnimation());
> > +        effect->GetAnimation()->NotifyGeometricAnimationsStartingThisFrame();
> > +      }
> 
> This could be a member function (static function) of PendingAnimationTracker.

I looked into this and decided that, although I was trying to keep this logic out of PendingAnimationTracker, doing it there would mean we could actually just iterate over the play-pending animations (rather than *all* the animation in each EffectSet). I've added in some further optimizations to avoid worst-case O(n^2) behavior and re-ordered the patches to accommodate the change in logic so I'll need to request re-review for this part.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #52)
> I wonder there is no simple test case such that two animations starting on
> different frames are NOT synchronized at all.

I've added tests for this now.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #68)
> > I mean we should just drop this check. We will still call this for animations that don't animate transform so I don't think we can add an assertion.
> 
> Ah, I see.  Now your comment noticed me that we should clear
> mSyncWithGeometricAnimations flag when we change playback rate.  I think we
> should clear it at least for zero playback rate, no?

I guess so. It doesn't seem like a common situation, and generally we don't clear that flag unless we re-start the animation. I'll add a test for this and see how much of a code change it is but I'm a little bit wary of adding too many special cases.
Comment on attachment 8816037 [details]
Bug 1301305 - Extend PendingAnimationTracker to mark play-pending animations if there are geometric animations starting at the same time;

Re-requesting review since quite a bit has changed when incorporated review feedback.
Attachment #8816037 - Flags: review+ → review?(hiikezoe)
Comment on attachment 8816034 [details]
Bug 1301305 - Make DevTools tests that expect all animation properties to run on the compositor use 'opacity';

https://reviewboard.mozilla.org/r/96812/#review97796
Attachment #8816034 - Flags: review?(pbrosset) → review+
Comment on attachment 8816037 [details]
Bug 1301305 - Extend PendingAnimationTracker to mark play-pending animations if there are geometric animations starting at the same time;

https://reviewboard.mozilla.org/r/96818/#review97802

The comment in MarkAnimationsThatMightNeedSynchronization() is great!, it really helps to understand what's going on there!
Attachment #8816037 - Flags: review?(hiikezoe) → review+
(In reply to Brian Birtles (:birtles) from comment #101)
> 
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #52)
> > I wonder there is no simple test case such that two animations starting on
> > different frames are NOT synchronized at all.
> 
> I've added tests for this now.

Thanks for this test. I would like to mark the patch as review++!
Thank you for all the reviews!

(In reply to Brian Birtles (:birtles) from comment #101)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #53)
> > Ah, I see.  Now your comment noticed me that we should clear
> > mSyncWithGeometricAnimations flag when we change playback rate.  I think we
> > should clear it at least for zero playback rate, no?
> 
> I guess so. It doesn't seem like a common situation, and generally we don't
> clear that flag unless we re-start the animation. I'll add a test for this
> and see how much of a code change it is but I'm a little bit wary of adding
> too many special cases.

I looked into doing this and I think we don't need to do anything.

Firstly, changing the playback rate to something non-zero doesn't necessarily mean we shouldn't synchronize. For example, if the playback rate is 0.5 it might still be desirable to synchronize.

If the playback rate of the *transform* animation is 0, then Animation::ShouldBeSynchronizedWithMainThread will already return zero because the call to IsPlaying() in that method returns false.

If the *geometric* animation changes playback rate after playing starts then we don't do anything just like if the geometric animation no longer animates any geometric properties we don't update anything. In order to keep things simple, we just make this a one-time check at play time (doing otherwise might be useful in the future, but for now it seems ok).

So, I think the only case where we could change the behavior is for a geometric animation that is set to zero playback rate before it begins playback. That seems pretty unlikely and so I'm not sure we need to do anything here.
Failed to land this with autoland due to bug 1307664.
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca7fd1a24055
Move test data alongside the test function; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa5c5ad83b28
Rename assert_property_state_on_compositor to assert_all_properties_running_on_compositor; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/aba14a8202e0
Move propertyToIDL to testcommon.js; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a3bdee53cec
Expand the set of geometric properties to include margin and padding properties; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/ece7cb85c381
Make DevTools tests that expect all animation properties to run on the compositor use 'opacity'; r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/d14b2103446d
Add a performance warning type for transform animations that should be synchronized with geometric animations; r=hiro, r=flod
https://hg.mozilla.org/integration/mozilla-inbound/rev/49a60bdadd4e
Add AnimationEffectReadOnly::AffectsGeometry() helper to identify effects that animate geometric properties; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a8a06811d79
Factor out check for main-thread synchronization to a method on Animation; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc0424e6e32d
Add a member to track if an animation needs to be synchronized with geometric animations or not; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f7a10d823e2
Extend PendingAnimationTracker to mark play-pending animations if there are geometric animations starting at the same time; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/aea2d20ddd34
Notify animations when they should synchronize with geometric animations; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0c1e6b61ded
Add tests for transform animations synchronized with geometric animations: r=hiro
Depends on: 1320236
Thanks Brian and everyone who helped out with this, makes a huge difference in performance when using trello.com!
Do we want to try backporting this to Aurora or let it ride the 53 train?
Flags: needinfo?(bbirtles)
Flags: in-testsuite+
This is a pretty big patch queue that probably needs a fair bit of rebasing (although I haven't tried, I just know we've touched that part of the code quite a bit recently). It fixes a regression from Firefox 49 so we're already shipping it and I think this is the only major property where it's been reported, so I think it's not worth backporting.
Flags: needinfo?(bbirtles)
Depends on: 1495653
No longer depends on: 1495653
Blocks: 1747345
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: