Closed Bug 1339578 Opened 3 years ago Closed 3 years ago

OMTA doesn't trigger for 4x4 layers leading to CPU burn on Facebook

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: BenWa, Assigned: jnicol)

References

Details

(Whiteboard: platform-rel-Facebook)

Attachments

(2 files)

Attached file reduce.html
Here's a simple testcase of Facebook's someone is typing animation 'mercuryTypingAnimation'. Since Firefox doesn't trigger OMTA it causes a 60Hz displaylist construction burning a lot of CPU for the animation. Chrome OMTAs the animation just fine and doesn't consume as much CPU.
We don't send animations on small frames, currently its size limit is 16 pixel[1].  I don't know how much memory consumes per layer, is it worthwhile sending small animations even if its size is 1? On Quantum Render we don't need to worrying about it?

https://hg.mozilla.org/mozilla-central/file/1060668405a9/layout/painting/nsDisplayList.cpp#l5046
I agree it's really small. But what's worthwhile is not recomputing the display list on a real website at 60 Hz just to drive a simple animation. Firefox is throwing away a ton of CPU cycles because of it compared to Chrome that allows it to run off the main thread. I'd rather not have to inflate the layer to 16x16 with transparent space just to get this fast path to trigger.
Jamie should probably take a look at this, since it actively conflicts with bug 1338238.
Flags: needinfo?(jnicol)
Recomputing the display list at 60Hz because of this is bad, so this would be a good improvement.

My motivation for bug 1338238 is that when an item's layererization frequently changes between active and inactive it can cause lots of other content to be relayerized and repainted at the same frequency.

By removing the minimum size for compositor animations, but increasing the minimum size for manual restyles, that might satisfy both use cases? (My terminology might be wrong - I mean EffectCompositor::HasAnimationsForCompositor() vs ActiveLayerTracker::IsStyleAnimated())
Flags: needinfo?(jnicol)
That's sound like a great idea to me. In the case for ActiveLayerTracker::IsStyleAnimated you're going to be running through the main thread anyways and pay the cost of a displaylist so you don't win much with the layer. And this would give us the layer when we need it, the OMTA case.
Whiteboard: platform-rel-Facebook
Yeah agreed, I think that's a good solution.
I'll fix this and bug 1338238 together next week when I'm back from Toronto.
Assignee: nobody → jnicol
The above patch does what I said in comment 4. For transforms and opacities.

The test for min size now expects the animation to be done on the compositor instead of a warning to be emitted. Maybe instead we want the test to trigger the ActiveLayerTracker::IsStyleAnimated() code path rather than EffectCompositor::HasAnimationsForCompositor(), but I wasn't entirely sure how best to do that in the test.
Comment on attachment 8839228 [details]
Bug 1339578 - Remove min active layer size for animations;

https://reviewboard.mozilla.org/r/113926/#review115468

Awesome! I think this puts us in a better place for both use cases.
Attachment #8839228 - Flags: review?(matt.woodrow) → review+
(In reply to Jamie Nicol [:jnicol] from comment #9)
> The above patch does what I said in comment 4. For transforms and opacities.
> 
> The test for min size now expects the animation to be done on the compositor
> instead of a warning to be emitted. Maybe instead we want the test to
> trigger the ActiveLayerTracker::IsStyleAnimated() code path rather than
> EffectCompositor::HasAnimationsForCompositor(), but I wasn't entirely sure
> how best to do that in the test.

After this change, it seems to me that the warning message (i.e CompositorAnimationWarningContentTooSmall) is not useful.  I guess the case is something like this:

@keyframes anim {
  from { transform: none; }
  to { transform: translateX(100px); }
}

target.style.willChange = 'transform';
requestAnimationFrame(() => {
  // I am guessing we get the warning at this moment even though the target element does not have animation at all.
  target.style.animation = 'anim 10s';
  requestAnimationFrame(() => {
    // at this moment, we don't get any warnings.
  });
});

Is there any test cases that causes the warning?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> After this change, it seems to me that the warning message (i.e
> CompositorAnimationWarningContentTooSmall) is not useful.

Using will change like you said should trigger the warning, good idea. But are you suggesting instead that we remove the warning altogether?

> Is there any test cases that causes the warning?

Not deliberately - devtools/server/tests/browser/animation.html tests were so I had to increase the div size.
Flags: needinfo?(hikezoe)
Duplicate of this bug: 1338238
(In reply to Jamie Nicol [:jnicol] from comment #13)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> > After this change, it seems to me that the warning message (i.e
> > CompositorAnimationWarningContentTooSmall) is not useful.
> 
> Using will change like you said should trigger the warning, good idea. But
> are you suggesting instead that we remove the warning altogether?

Yes.  If we no longer output this warning while animation is running, let's drop the warning!
Flags: needinfo?(hikezoe)
Thanks for doing this!  We still need those two test cases. :-)

Thanks!
(In reply to Jamie Nicol [:jnicol] from comment #16)
> Comment on attachment 8839228 [details]
> Bug 1339578 - Remove min active layer size for css animations;

Oops!  I did forget to mention about the commit message.  'css animations' is slightly misleading.  We have script animation, css animation and css transition.  All of them will be affected by this change. So, we can use just 'animations' there. Thanks!
Thanks a lot!
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0b55ea90ed2a
Remove min active layer size for animations; r=mattwoodrow
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0b55ea90ed2a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Duplicate of this bug: 1330834
Thank you for shipping this fix. Given that 54+ is now widely deployed I'll restore the animation on Facebook. The CPU usage is now ~20-30% so it's still high but it's more reasonable.
You need to log in before you can comment on or make changes to this bug.