Katelyn Gadd (:kael) Reporter
	Description • 10 years ago

CSS animations that force an element to be a composited layer during the animation cause it to remain one forever, even after they are permanently inactive.

The attached test case retains steady, relatively good performance in Chrome. Examination in their dev tools indicates that the elements become composited layers during the animation, but once animation ceases they return to normal non-composited elements.

In Firefox, the elements appear to become composited layers at the beginning of the animation and stay one forever. (If you update the script to use an animationend event handler to clear the animation attribute, the performance becomes good like Chrome's... but Chrome becomes slower because removing the attribute forces a full document layout. :| )

If you monitor CPU & GPU usage in Firefox it climbs steadily until it's maxing out a core and heavily loading your GPU. In Chrome the two remain relatively steady.

See bug URL for a more complex example of this bug (w/a workaround, because I can't stand the bad perf :-) )

	Katelyn Gadd (:kael) Reporter
	Comment 1 • 10 years ago

Attaching a second test case that applies a Firefox-specific fix (removing the animation name) and demonstrates partially improved performance.

This fix isn't as good as the one applied to my larger demo, for some reason. I realized that my larger demo also clears the display: inline-block css directive, but that isn't wholly sufficient here.

With the modified test case, performance starts out bad but improves as the test reaches the end of the scheduled animations.

(My best guess for why this version is suboptimal is that the attribute change or style change is triggering layout.)

Related chrome bug report: https://code.google.com/p/chromium/issues/detail?id=398810

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 2 • 10 years ago

Timothy, should this stay in layers or go into layout?

	Timothy Nikkel (:tnikkel)
	Comment 3 • 10 years ago

My initial  guess is layout.

	David Baron :dbaron:
	Comment 4 • 10 years ago

Brian, do you know what's going on here?

	Brian Birtles (:birtles) Assignee
	Comment 5 • 10 years ago

I don't have time to look into this properly before I go away this afternoon but from a quick look, I can see:

* After all animations have finished we're *not* calling NotifyAnimated so that part of things should be ok

* In ActiveLayerTracker::IsStyleAnimated we call nsLayoutUtils::HasAnimations which will return true even if all the animations have finished and are just doing a forwards fill (since it just calls GetAnimationsOrTransitions which calls HasAnimationOfProperty on the collection of animations). That seems odd to me since most of the call sites of IsStyleAnimated seem to be mostly interested in an actual changing animation. In fact, some of those call sites are marking the layer as active based on the return value of IsStyleAnimated.

With regards to that second point, if that is in fact the problem, then I think we want to do something more like GetAnimationsForCompositor (http://hg.mozilla.org/mozilla-central/file/08c23f12a43e/layout/style/AnimationCommon.cpp#l93) which, after checking HasAnimationsOfProperty, calls CanPerformOnCompositorThread which returns false if the animation is not running.

In that case, a "HasActiveAnimationOfProperty" that also checks anim->IsRunning might work (although perhaps it should return true if the animation is due to run in the next few ms so the layerization can happen ahead of time?)

Also, I don't *think* this is a recent regression since I've observed this behavior before with a similar test but never reduced it enough to file a bug.

	Brian Birtles (:birtles) Assignee
	Comment 6 • 10 years ago

If no-one gets to this before me, I'll make up a patch for this in a couple of weeks' time.

	Brian Birtles (:birtles) Assignee
	Comment 7 • 10 years ago

I'm not sure what approach we want to take to layerizing inside ActiveLayerTracker::IsStyleAnimated. Some possible approaches:

a) Only return true when we have animations that are running (i.e. not paused, not in delay phase) -- least memory usage, but more lag when we resume an animation or start an animation after a delay.

b) Return true whenever we have an animation that has yet to finish running (we already have HasCurrentAnimations for this actually) -- more memory used, less code (since we can reuse HasCurrentAnimations), less lag when starting animations.

c) Some middle-road from the above. e.g. return true if the animation is about to start in the next 200ms. Not sure about pausing.

I'm somewhat inclined to go with (b). Animations with a delay are (currently) somewhat uncommon so the memory we save but not layerizing them is probably not significant.

Pausing, however, is less clear. If an app sets up a bunch of paused animations and simply unpauses them when they are ready to run, we'll consume more memory. But perhaps the performance trade-off the app developer is aiming for? Setting the animations up in advance and pausing them so that there is less delay when they need to play them?

	Katelyn Gadd (:kael) Reporter
	Comment 8 • 10 years ago

One of the reasons to use delays is that it gives you precise control over animation scheduling. If you trigger animations manually using JS the timing is imprecise so you get jittery animations. In that scenario, (b) is ideal.

	Brian Birtles (:birtles) Assignee
	Comment 10 • 10 years ago

It's not the player that's "current" (a Web Animations term for an animation
that hasn't yet finished), but it's source content, if any. This patch renames
the method on AnimationPlayer accordingly.

At the same time this patch moves the method to the header file since it's
quite simple and could possibly benefit from inlining.

	Brian Birtles (:birtles) Assignee
	Comment 11 • 10 years ago

This patch adds another method to AnimationPlayerCollection alongside
HasCurrentAnimations that returns true if there is a player in the collection
with current source content that targets the specified property.

At the same time it also makes the original HasCurrentAnimations a const method.

	Brian Birtles (:birtles) Assignee
	Comment 12 • 10 years ago

This patch adds a method to nsLayoutUtils, alongside the existing
HasCurrentAnimations, that returns true if there exists an unfinished animation
on the element for the specified property.

	Brian Birtles (:birtles) Assignee
	Comment 13 • 10 years ago

Within the context of determining of a layer is active, we should only consider
an element animated if it has an animation that is yet to finish, i.e. a current
animation. Animations that have finished should not cause a layer to be active
(even if they are applying a forwards fill).

This patch makes that change by calling
nsLayoutUtils::HasCurrentAnimationsForProperty instead of
nsLayoutUtils::HasAnimations.

	Brian Birtles (:birtles) Assignee
	Comment 14 • 10 years ago

https://treeherder.mozilla.org/ui/#/jobs?repo=try

	David Baron :dbaron:
	Comment 15 • 10 years ago

Comment on attachment 8497276 [details] [diff] [review]
part 3 - Add AnimationPlayerCollection::HasCurrentAnimationsForProperty

>+  // Returns true if there is an animation that has yet to finish.
>+  bool HasCurrentAnimations() const;
>+  // REVIEW: Just call this |HasCurrentAnimations| ?
>+  bool HasCurrentAnimationsForProperty(nsCSSProperty aProperty) const;

Maybe HasCurrentAnimationsFor?

r=dbaron

	David Baron :dbaron:
	Comment 16 • 10 years ago

Comment on attachment 8497277 [details] [diff] [review]
part 4 - Add HasCurrentAnimationsForProperty to nsLayoutUtils

Though this makes me think maybe the current names are best.

	Brian Birtles (:birtles) Assignee
	Comment 17 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/723e749323c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/37090bda6564
https://hg.mozilla.org/integration/mozilla-inbound/rev/860a94d6002a
https://hg.mozilla.org/integration/mozilla-inbound/rev/4945682e29a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/dedaffb8297a

	Ryan VanderMeulen [:RyanVM]
	Comment 18 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/723e749323c6
https://hg.mozilla.org/mozilla-central/rev/37090bda6564
https://hg.mozilla.org/mozilla-central/rev/860a94d6002a
https://hg.mozilla.org/mozilla-central/rev/4945682e29a6
https://hg.mozilla.org/mozilla-central/rev/dedaffb8297a