Closed Bug 1194037 Opened 4 years ago Closed 4 years ago

Batch animation mutation observer records even when animations are not ticked by element

Categories

(Core :: DOM: Animation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(6 files, 6 obsolete files)

2.02 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
6.71 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
3.86 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
13.52 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.61 KB, patch
heycam
: review+
Details | Diff | Splinter Review
1.86 KB, patch
heycam
: review+
Details | Diff | Splinter Review
This corresponds roughly to bug 1151731 comment 4 part c:
"c) Move mutation observer batching to happen when we compose style, *not* when we tick"

I'm not sure if that description is quite correct, however.

The end game is to have CommonAnimationManager::WillRefresh simply call AnimationCollection::Tick on each collection (and make FlushAnimations a completely independent method). Doing that means moving everything that needs to happen on a regular refresh driver tick into Animation::Tick. The reason is that ultimately we want to calling Animation::Tick from the *timeline* that the animation is observing.

The last piece remaining is the animation mutation observer work. I thought we could just take care of this when we compose style but I think there's one case where that won't work: when we have an animation with a KeyframeEffect that has a duration but no properties. In this case we'll never compose style but we should still dispatch a removed record on the tick when it completes.

So, I think we need to queue mutation observer records from within Animation::Tick. I'm unclear, however, what the granularity of batching should be in this case. e.g. if we have:

#a {
  animation: anim1 3s, anim2 3s
}

Then, since both animations finish at the same time, I suppose we should just dispatch one record.

Once we make *timelines* observe the refresh driver and then call Animation::Tick on all the animations observing the timeline (which are stored in a hashmap so will be called randomly) I don't know how we're going to collate all those records since nsAutoAnimationMutationBatch is per-element.

e.g. if we have

#a {
  animation: anim1 3s, anim2 3s
}
#b {
  animation: anim3 3s
}

We might call Animation::Tick in the following order:

 anim1
 anim3
 anim2

And we need to make sure the records produced by anim1 and anim2 get combined. I'm going to look into the mutation observer code but Cameron, if you have any suggestions please let me know!
So far some options I can see are:

a) Forget batching for this case. This is a chrome-only API and DevTools etc. can cope with multiple records for the same element. We'll probably need to update tests, however.

b) Rewrite / extend nsAutoAnimationMutationBatch to support multiple batch targets.

c) Rework the storage / sample order in AnimationTimeline to somehow group animations by target element.

(c)'s not going to work once we have GroupEffects so I'd rather avoid that. (b) seems preferable to (a) but might be a lot of work. I'll have a look at it.
Per IRC discussion, it looks like (b) would indeed be the best option.
(b) might not be so hard after all. Here is a WIP patch for it.
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Attachment #8647340 - Attachment is obsolete: true
Part 3 here is causing intermittent timeouts in test_animation-currenttime.html and test_animation-starttime.html
(In reply to Brian Birtles (:birtles) from comment #7)
> Part 3 here is causing intermittent timeouts in
> test_animation-currenttime.html and test_animation-starttime.html

I think what's happening here is that:

* in these tests we're waiting on an animationend event
* we fast-forward to the end of the animation which triggers us to post a restyle
* we do FlushAnimations but as of part 3 we no longer call Animation::Tick hence we don't queue events
* we do EnsureStyleRuleFor and call Animation::ComposeStyle which will leave aNeedsRefreshes to false since the animation has now finished
* we end up calling MaybeStartOrStopObservingRefreshDriver somewhere (a subsequent call to FlushAnimations presumably) and stop observing the refresh driver
* hence Animation::Tick never gets called again and the animationend event never gets dispatched

The underlying problem is the place where we decide if we need to observe the refresh driver is completely separate from the place that gets called by the refresh driver. That is, getting ticks and dispatching events are related to animation timing, but we're registering and unregistering from the refresh driver when we compose style.

Pretty soon we should be able to make timelines tick their animations directly and fix the whole refresh driver observing behavior completely (it's currently very error prone and we often encounter these kind of bugs). In the meantime, as a temporary measure, I think we should add a virtual Animation::HasEndEventToDispatch() method that CSSAnimation and CSSTransition override to indicate if they still need another tick in order to fire their corresponding end event. Then in Animation::ComposeStyle call this as part of updating aNeedsRefreshes.
Actually, it's not quite right. I *think* what happens is that we do a compose, notice the animation has finished, so we update AnimationCollection::mNeedsRefreshes to false. Then, in the mean time, another animation collection is destroyed, and that triggers a call to CommonAnimationManager::ElementCollectionRemoved at which point we decide to stop watching the refresh driver--i.e. between finishing the animation and doing the tick it needs to queue up its last event.

If that is, in fact, what is going wrong, then it might be the cause of bug 1162208 and bug 1169112 as well.
If comment 9 is correct then ultimately the problem is indeed that we determine if we should watch the refresh driver or not when we *compose* not when we tick and the workaround in comment 8 should probably work.
Blocks: 1195180
In bug 1195180 we plan to tick animations from their timeline where they
are stored in a hashtable. As a result, we will not visit them in order of
their associated target element (indeed, part of the reason we are doing
this is to support animations that do not have, or even have multiple target
elements).

The current animation mutation observer batching mechanism, however, assumes
that we visit each target element in turn and make all the necessary work at
once.  In order to support visiting animations in a potentially random order
this patch reworks the animation mutation observer batching mechanism so that
it can support batching multiple elements at once.
Attachment #8648583 - Flags: review?(bugs)
Attachment #8647353 - Attachment is obsolete: true
In order to support ticking animations from their timeline we want to separate
the following two methods:

CommonAnimationManager::WillRefresh - responsible for responding to refresh
  driver ticks by updating timing information and posting the necessary
  pending restyles. This is the functionality we will eventually move to
  Animation.

CommonAnimationManager::FlushAnimations - responsible simply for posting
  pending restyles.

Currently, WillRefresh calls FlushAnimations. This patch separates the two by
copying the necessary functionality into WillRefresh. Later in this patch series
we will further separate the two by removing duplicate functionality from
WillRefresh.
Attachment #8647355 - Attachment is obsolete: true
We currently determine if we need refresh driver ticks when composing style
but sometimes we might not need ticks for composing style but we might need
one more tick in order to queue a final end event. Currently, this doesn't
seem to be a problem because FlushAnimations calls Animation::Tick where we
queue up events. When we remove the call to Animation::Tick from
FlushAnimations in order to make FlushAnimations purely responsible for
posting restyles, however, we will create a situation where we might mark an
animation collection as no longer needing refreshes and not simultaneously
queueing the corresponding event. If another animation collection is deleted in
the meantime we may trigger the code that causes us to disassociate from the
refresh driver and the corresponding event will never be dispatched.

Long-term (bug 1195180) we will check if it we can stop observing the refresh
driver and queue events in the same step. Until then, this patch adds a method
to detect this particular situation and uses it to avoid unregistering from
the refresh driver while we still have end events to queue.
This patch makes FlushAnimations purely responsible for posting restyles. All
ticking behavior is performed in response to an actual refresh driver tick
(currently CommonAnimationManager::WillRefresh).
Attachment #8647356 - Attachment is obsolete: true
Comment on attachment 8648585 [details] [diff] [review]
part 2 - Make WillRefresh no longer call FlushAnimations

Daniel, sorry to load you up with more reviews. I was planning on giving these ones to Cameron but they didn't turn out to be really related to the animation observer functionality after all and I figure you probably have the necessary context in your head now having looked at bug 1188251 for me.
Attachment #8648585 - Flags: review?(dholbert)
Attachment #8648590 - Flags: review?(dholbert)
Attachment #8648591 - Flags: review?(dholbert)
Comment on attachment 8648585 [details] [diff] [review]
part 2 - Make WillRefresh no longer call FlushAnimations

># Parent  6b0f469d37d28e2d7c2f55dcd8b6a82259faed4b
>Bug 1194037 part 2 - Make WillRefresh no longer call FlushAnimations
>
[...]
>Currently, WillRefresh calls FlushAnimations. This patch separates the two by
>copying the necessary functionality into WillRefresh. Later in this patch series
>we will further separate the two by removing duplicate functionality from
>WillRefresh.

I think you mean "...from FlushAnimations" at the end here? (not "from WillRefresh")

(I assume this is hinting at part 4, which is labeled "Remove ticking from FlushAnimations".)
Comment on attachment 8648585 [details] [diff] [review]
part 2 - Make WillRefresh no longer call FlushAnimations

r=me on part 2, modulo the extended-commit-message fix noted in comment 16.

(side note: the boilerplate for iterating over mElementCollections here seemed a bit painful, so I filed bug 1195523 to modernize that. If you'd like, I'm happy to hold off on pushing that bug forward until this bug has landed, to avoid bitrotting you here.)
Attachment #8648585 - Flags: review?(dholbert) → review+
Comment on attachment 8648590 [details] [diff] [review]
part 3 - Add Animation::HasEndEventToQueue()

Review of attachment 8648590 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on part 3, just one nit:

::: dom/animation/Animation.h
@@ +299,5 @@
>                      bool& aNeedsRefreshes);
> +
> +  // FIXME: Because we currently determine if we need refresh driver ticks
> +  // during restyling (specifically ComposeStyle above) and not necessarily
> +  // part during a refresh driver tick, we can arrive at a situation where we

The phrase "not necessarily part during a refresh driver tick" doesn't make sense to me - I think you might've meant to remove the word "part"?  Or something like that.
Attachment #8648590 - Flags: review?(dholbert) → review+
Comment on attachment 8648591 [details] [diff] [review]
part 4 - Remove ticking from FlushAnimations

Review of attachment 8648591 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following:

::: layout/style/AnimationCommon.h
@@ +99,5 @@
>  
>      return false;
>    }
>  
> +  void FlushAnimations();

While you're adjusting what FlushAnimations() is repsonsible for, could you add a brief comment here to document its current (newly narrowed-in-scope) purpose?

Something like:
 // Calls RequestRestyle() on each managed AnimationCollection that has an
 // out-of-date mStyleRuleRefreshTime.

...or some version of your prose from comment 12.
Attachment #8648591 - Flags: review?(dholbert) → review+
Comment on attachment 8648583 [details] [diff] [review]
part 1 - Make nsAutoAnimationMutationBatch batch multiple elements at once

I don't see how we can use hashtable, or only hashtable here.
We want some particular order, doesn't perhaps matter what, but not random order.
Without that testing this all properly becomes rather hard
(and we do need tests for this)
Could you perhaps add a separate array which holds {target, EntryArray} and then the hashtable could just hold a pointer to the array or something.
Then just iterate the array in nsAutoAnimationMutationBatch::Done() and not the hashtable.


(I'm not sure why we want to break the ordering of things here anyway. Why couldn't we just stop batching if target changes, or effective stop the old batch and start a new one.)
Attachment #8648583 - Flags: review?(bugs) → review-
Yeah, I was thinking the same thing just recently. This patch is preparing us for the situation where we sample *animations* in random order. Hence we end up doing this:

* sample animX -> add record for elementA
* sample animJ -> add record for elementB
* sample animT -> add record for elementA

Hence we can't just stop batching in the target changes. We want those two changes for elementA above to be batched.

Currently we sample all the animations on an element at once so we only have one target at a time. However, the order in which we visit those elements is pretty obscure. The order is based on when an element last transitioned from having 0 animations to having 1 or more animations. So it's certainly not intuitive. But it's not random.

Probably ideally we should do something like comment 20 but also sort the array by document position of the target element.

I'm fixing fixing almost exactly the same problem for CSS animation events and transition events (bug 1183461).
Attached patch Fixes WIP (obsolete) — Splinter Review
I've worked on making the record dispatch stable but I'm currently stuck
because the test case fails due to an unexpected "change" record that comes
about due to the way we update the mWinsInCascade member on one of the
animations. I need to look into why this is changing.
The problem with mWinsInCascade appears to be that when we have multiple animations targetting the same property, such as the following:

  animation: anim 100s, anim 100s

when we run UpdateCascadeResults we'll update the mWinsInCascade member for the appropriate property in the *first* animation in the last since it is overridden by the second.

However, when we go to run CheckAnimationRule, we'll regenerate the animations using BuildAnimations and then compare them. In BuildAnimations, however, we unconditionally set mWinsInCascade to true and we don't run UpdateCascadeResults on the new animations before comparing them with the old. As a result, we detect a change in the properties between new and old and end up generating a change mutation record even if nothing has changed.

I'm reluctant to update AnimationProperty::operator== to ignore mWinsInCascade since it seems like in other circumstances that's the correct thing to do (and was specifically added in https://hg.mozilla.org/mozilla-central/diff/8d01ce976a2b/dom/animation/Animation.h). Instead, it's probably better to define a separate comparison method that specifically ignores the mWinsInCascade method and use that in this case.
I've updated this patch based on feedback from comment 20.

Specifically I've made the following changes:

* Added the mBatchTargets array and used that (after sorting by tree order) to
  determine the order in which we create mutation observer records.
* Adjusted the behavior when we have nested nsAutoAnimationMutationBatch objects
  on the stack so that ::Done() does nothing until we unwind to the bottom-most
  object that was initialized with a document that can have dom mutation
  observers.

I have a test case for this behavior forthcoming.
Attachment #8653332 - Flags: review?(bugs)
Attachment #8648583 - Attachment is obsolete: true
This test will currently fail due an extra change record that gets erroneously
added as described in comment 22 and comment 23. I'll add a part 5 that fixes
this behavior after I've determined how best to fix this.
Attachment #8653339 - Flags: review?(cam)
Attachment #8652908 - Attachment is obsolete: true
David, as discussed in comment 23, I'm seeing a bug in our animation mutation observer implementation where we are detecting changes to the mWinsInCascade member on animation properties and generating change records where we shouldn't.

The reason for this is:

1. In nsAnimationManager::CheckAnimationRule when we recreate animations and compare the old and new animations we end up using nsTArray<AnimationProperty>::operator== as part of the comparion. That, in turn, invokes AnimationProperty::operator==.
2. As of [1], AnimationProperty::operator== compares the mWinsInCascade member.
3. In nsAnimationManager::BuildAnimations (as called in step 1 above) we unconditionally set mWinsInCascade to true on the new animations. However, the old animations might have mWinsInCascade == false. (If we do end up using the new animations we'll update mWinsInCascade later in CheckAnimationRule when we run UpdateCascadeResults).
4. As a result, nsTArray<AnimationPropery>::operator== may return false even though nothing has really changed.
5. As a result of that we'll end up dispatching a spurious change record.

We can solve this by either:

A. Updating operator== to always ignore mWinsInCascade.
B. Adding a separate suitably-named comparison method to AnimationProperty (and a method on KeyframeEffectReadOnly to call it) that ignores mWinsInCascade.

It appears we never use AnimationProperty::operator== anywhere else in the code base so I'm inclined to just do A and consider mWinsInCascade a piece of dynamic state that isn't part of an AnimationProperty's identity when comparing objects. Does that seem ok to you or was there a specific reason you added mWinsInCascade to operator== in [1]?

[1] https://hg.mozilla.org/mozilla-central/rev/8d01ce976a2b
Flags: needinfo?(dbaron)
Summary: Generate animation mutation observer records from Animation::Tick → Batch animation mutation observer records even when animations are not ticked by element
Comment on attachment 8653332 [details] [diff] [review]
part 1 - Make nsAutoAnimationMutationBatch batch multiple elements at once

>+  mBatchTargets.Sort(TreeOrderComparator());
I'm tiny bit worried about the performance of this, but since the API is for chrome/devtools only for now, this shouldn't slow down pages usually.
Did you try this with some testcase which adds lots of batch targets?
Attachment #8653332 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #27)
> Comment on attachment 8653332 [details] [diff] [review]
> part 1 - Make nsAutoAnimationMutationBatch batch multiple elements at once
> 
> >+  mBatchTargets.Sort(TreeOrderComparator());
> I'm tiny bit worried about the performance of this, but since the API is for
> chrome/devtools only for now, this shouldn't slow down pages usually.
> Did you try this with some testcase which adds lots of batch targets?

Good point. Currently this will only happen when you have a bunch of animations that finish within the same tick. I've created a test to produce this situation and I don't see any obvious jank. This is also only going to happen when the user has the animations panel active.

I think longer term we'll probably want to look at sampling animations in tree order so we can skip doing this kind of sorting both here and for animation events (for events, if we assume the events are stored in tree order, we could simplify the sorting to a stable sort based on animation time).
Keywords: leave-open
(In reply to Brian Birtles (:birtles) from comment #28)
> Good point. Currently this will only happen when you have a bunch of
> animations that finish within the same tick. I've created a test to produce
> this situation and I don't see any obvious jank.> 
FWIW, in this kind of cases I'd use a profiler to see how badly the possibly slow code shows up in the performance profile.
Attachment #8653339 - Flags: review?(cam) → review+
Comment on attachment 8653339 [details] [diff] [review]
part 6 - Add test for order of mutation observer records

>Bug 1194037 part 6 - Add test for order of mutation observer records

Should be part 5, I guess.
Thanks! I left room for part 5 to fill in whatever I decide to do with comment 26 (based on dbaron's feedback).
(In reply to Brian Birtles (:birtles) from comment #26)
> It appears we never use AnimationProperty::operator== anywhere else in the
> code base so I'm inclined to just do A and consider mWinsInCascade a piece
> of dynamic state that isn't part of an AnimationProperty's identity when
> comparing objects. Does that seem ok to you or was there a specific reason
> you added mWinsInCascade to operator== in [1]?

I don't think there was a specific reason, but it seems a bit dangerous to have an operator== that lies like that.  I guess I'm ok with it if you have clear comments on both the operator and the mWinsInCascade member variable.
Flags: needinfo?(dbaron)
Attachment #8657696 - Flags: review?(cam) → review+
Keywords: leave-open
This has caused bustage on OS X and Windows debug Mochitest Other that looks like this https://treeherder.mozilla.org/logviewer.html#?job_id=13764375&repo=mozilla-inbound.
(In reply to Nigel Babu [:nigelb] from comment #37)
> This has caused bustage on OS X and Windows debug Mochitest Other that looks
> like this
> https://treeherder.mozilla.org/logviewer.html#?job_id=13764375&repo=mozilla-
> inbound.

It seems like this leak existed even before this bug. The test added in part 6 simply exposes it.
Keywords: leave-open
Depends on: 1203423
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/3aa15d0662f9
https://hg.mozilla.org/mozilla-central/rev/991e80fcdd03
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.