Closed Bug 1046055 Opened 5 years ago Closed 5 years ago

Inactive animations force elements to remain composited layers forever

Categories

(Core :: Layout, defect, major)

x86_64
Windows 7
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: kael, Assigned: birtles)

References

()

Details

(Keywords: perf)

Attachments

(7 files)

Attached file Test case
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 :-) )
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
Timothy, should this stay in layers or go into layout?
Flags: needinfo?(tnikkel)
My initial  guess is layout.
Component: Graphics: Layers → Layout
Flags: needinfo?(tnikkel)
Brian, do you know what's going on here?
Flags: needinfo?(birtles)
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.
Flags: needinfo?(birtles)
If no-one gets to this before me, I'll make up a patch for this in a couple of weeks' time.
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?
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.
Assignee: nobody → birtles
Status: NEW → ASSIGNED
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.
Attachment #8497275 - Flags: review?(dbaron)
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.
Attachment #8497276 - Flags: review?(dbaron)
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.
Attachment #8497277 - Flags: review?(dbaron)
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.
Attachment #8497278 - Flags: review?(dbaron)
Attachment #8497274 - Flags: review?(dbaron) → review+
Attachment #8497275 - Flags: review?(dbaron) → review+
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
Attachment #8497276 - Flags: review?(dbaron) → review+
Comment on attachment 8497277 [details] [diff] [review]
part 4 - Add HasCurrentAnimationsForProperty to nsLayoutUtils

Though this makes me think maybe the current names are best.
Attachment #8497277 - Flags: review?(dbaron) → review+
Attachment #8497278 - Flags: review?(dbaron) → review+
Depends on: 1083079
See Also: → 1155998
You need to log in before you can comment on or make changes to this bug.