Closed
Bug 1339578
Opened 8 years ago
Closed 8 years ago
OMTA doesn't trigger for 4x4 layers leading to CPU burn on Facebook
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: BenWa, Assigned: jnicol)
References
Details
(Whiteboard: platform-rel-Facebook)
Attachments
(2 files)
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.
Comment 1•8 years ago
|
||
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
Reporter | ||
Comment 2•8 years ago
|
||
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.
Comment 3•8 years ago
|
||
Jamie should probably take a look at this, since it actively conflicts with bug 1338238.
Flags: needinfo?(jnicol)
Assignee | ||
Comment 4•8 years ago
|
||
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)
Reporter | ||
Comment 5•8 years ago
|
||
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
Comment 6•8 years ago
|
||
Yeah agreed, I think that's a good solution.
Assignee | ||
Comment 7•8 years ago
|
||
I'll fix this and bug 1338238 together next week when I'm back from Toronto.
Assignee: nobody → jnicol
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•8 years ago
|
||
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 10•8 years ago
|
||
mozreview-review |
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+
Comment 11•8 years ago
|
||
(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?
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•8 years ago
|
||
(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)
Comment 15•8 years ago
|
||
(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)
Comment hidden (mozreview-request) |
Comment 17•8 years ago
|
||
Thanks for doing this! We still need those two test cases. :-)
Thanks!
Comment 18•8 years ago
|
||
(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!
Comment hidden (mozreview-request) |
Comment 20•8 years ago
|
||
Thanks a lot!
Assignee | ||
Comment 21•8 years ago
|
||
try push with latest revision: https://treeherder.mozilla.org/#/jobs?repo=try&revision=45f0ebdc3a6fa3fb667c598caa4be6ab2d717caf
Keywords: checkin-needed
Comment 22•8 years ago
|
||
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
Comment 23•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Reporter | ||
Comment 25•7 years ago
|
||
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.
Description
•