High CPU usage and jerky animation on Firefox 53

RESOLVED FIXED in Firefox 54

Status

()

Core
Graphics
P3
normal
RESOLVED FIXED
6 months ago
3 months ago

People

(Reporter: CoolCmd, Assigned: lsalzman)

Tracking

({regression})

53 Branch
mozilla55
All
Windows
regression
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox-esr52 unaffected, firefox53 wontfix, firefox54 fixed, firefox55 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 months ago
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170323105023

Steps to reproduce:

1. install extension: https://addons.mozilla.org/en-US/firefox/addon/twitch_5/
2. navigate to: twitch5:player?channel=coolcmd
3. CPU usage should be around 1%
4. press the EQUAL key on the keyboard. settings will be shown at the bottom right corner. click the 'colors', 'buffering' and 'general'. you will see the smooth animation.
5. press the MINUS key on the keyboard (to the left of EQUAL key). statistics will be shown at the top left corner.



Actual results:

6. CPU usage is around 12% on my machine
7. click the 'colors', 'buffering' and 'general'. you will see the JERKY animation.



Expected results:

6. CPU usage should be around 5% on my machine as in Firefox 52-
7. animation should be SMOOTH as in Firefox 52-
(Reporter)

Updated

6 months ago
Summary: High CPU usage on Firefox 53 → High CPU usage and jerky animation on Firefox 53

Updated

6 months ago
Component: Untriaged → DOM: Animation
Product: Firefox → Core
Mantaroh, do you mind triaging this to see if it is the right component or not?
Flags: needinfo?(mantaroh)
Sorry for late reply.

(In reply to Brian Birtles (:birtles) from comment #1)
> Mantaroh, do you mind triaging this to see if it is the right component or
> not?

The animation of settings panel is CSS-Transition which animating height and visibility properties. And statistics panel is changing DOM text content triggered by setInterval.

In the profile result:
https://perfht.ml/2oWKuIX

I think that this cause is gfxAlphaBoxBlur::DoBlur caused by replace content text. Then this animation was jumpy, I think.

lsalzman,
Could you please take a look this profile result? Perhaps, Is this related with bug 1250037?
Component: DOM: Animation → Graphics
Flags: needinfo?(mantaroh) → needinfo?(lsalzman)
(Reporter)

Comment 3

6 months ago
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #2)
> Perhaps, Is this related with bug 1250037?
text-shadow: 0 0 10px #000 - 20% CPU usage
text-shadow: 0 1px 1px #333 - 15% CPU usage
text-shadow: none - 5% CPU usage
(Assignee)

Comment 4

6 months ago
I tried typing in the twitch5 link in the URL bar. The add-on did not interpret at all. It just treated it as a search term and took me to yahoo like any other non-url text. ???
Flags: needinfo?(lsalzman) → needinfo?(CoolCmd)
(Reporter)

Comment 5

6 months ago
(In reply to Lee Salzman [:lsalzman] from comment #4)
> I tried typing in the twitch5 link in the URL bar. The add-on did not
> interpret at all. It just treated it as a search term and took me to yahoo
> like any other non-url text. ???

1. please install this version: https://addons.mozilla.org/en-US/firefox/addon/twitch_5/versions/?page=1#version-2017.4.18
2. do NOT use autocomplete in the URL bar! (this is another Firefox bug)
Flags: needinfo?(CoolCmd)
(Reporter)

Comment 6

6 months ago
open URL bar's context menu and choose 'paste & go'
(Assignee)

Comment 7

6 months ago
Created attachment 8865745 [details] [diff] [review]
only accelerate gfxAlphaBoxBlur when blur area is large enough to balance resource creation costs

When we're using HW acceleration for making text/box shadows, when the area of the shadow is small, the cost of creating the D2D draw target and flushing it can be very expensive relative to the actual speedup we're getting from doing the accelerated blur on that small number of pixels. That is the case here.

So, this makes sure we only use the HW accel if the area is sufficiently large.
Assignee: nobody → lsalzman
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8865745 - Flags: review?(mchang)
Comment on attachment 8865745 [details] [diff] [review]
only accelerate gfxAlphaBoxBlur when blur area is large enough to balance resource creation costs

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

::: gfx/thebes/gfxBlur.cpp
@@ +75,5 @@
>    // Check if the backend has an accelerated DrawSurfaceWithShadow.
>    // Currently, only D2D1.1 supports this.
>    // Otherwise, DrawSurfaceWithShadow only supports square blurs without spread.
>    if (aBlurRadius.IsSquare() && aSpreadRadius.IsEmpty() &&
> +      blurDataSize >= 8192 &&

Please comment why this is the magic number.
Attachment #8865745 - Flags: review?(mchang) → review+

Comment 9

6 months ago
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5aa7f8838b2b
only accelerate gfxAlphaBoxBlur when blur area is large enough to balance resource creation costs. r=mchang

Comment 10

6 months ago
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4c0c46ce1e9
followup - extra fuzz for D2D box-shadow reftest. r=me

Comment 11

6 months ago
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/120d8562d4a5
followup - extra fuzz for D2D box-shadow reftest. r=me a=merge

Comment 12

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5aa7f8838b2b
https://hg.mozilla.org/mozilla-central/rev/120d8562d4a5
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Comment 13

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e4c0c46ce1e9
(Assignee)

Comment 14

6 months ago
Can you check if this is fixed for you in new nightly builds?
Flags: needinfo?(CoolCmd)
(Reporter)

Comment 15

6 months ago
(In reply to Lee Salzman [:lsalzman] from comment #14)
> Can you check if this is fixed for you in new nightly builds?
fixed, thank you.

CPU LOAD
Firefox 53: 18-32%
Firefox 55 (nightly): 2-4%
Chrome 58: 0-1% (wow!)

i see the 'verify' button at the bottom of this page. should i click it?

and... it's a pity that users will have to wait 3 months before this fix reaches them. :(
Flags: needinfo?(CoolCmd)
(Assignee)

Comment 16

6 months ago
Comment on attachment 8865745 [details] [diff] [review]
only accelerate gfxAlphaBoxBlur when blur area is large enough to balance resource creation costs

Approval Request Comment
[Feature/Bug causing the regression]: bug 1250037
[User impact if declined]: Performance regression on some small box shadows on Windows/D2D.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: no
[Why is the change risky/not risky?]: Just falls back to the old software blur implementation we are using if the size of the blur is too small to benefit from hardware acceleration.
[String changes made/needed]: none
Attachment #8865745 - Flags: approval-mozilla-beta?
(Assignee)

Updated

6 months ago
Blocks: 1250037
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox53: --- → affected
status-firefox54: --- → affected
status-thunderbird_esr45: --- → unaffected
status-thunderbird_esr52: --- → unaffected
Keywords: regression
OS: Unspecified → Windows
Priority: -- → P3
Hardware: Unspecified → All
Depends on: 1364176
status-firefox53: affected → wontfix
status-firefox-esr45: --- → unaffected
status-firefox-esr52: --- → unaffected
status-thunderbird_esr45: unaffected → ---
status-thunderbird_esr52: unaffected → ---
Comment on attachment 8865745 [details] [diff] [review]
only accelerate gfxAlphaBoxBlur when blur area is large enough to balance resource creation costs

Fix a perf regression. Beta54+. Should be in 54 beta 8.
Attachment #8865745 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 18

6 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/de9ea8491a29
status-firefox54: affected → fixed
(In reply to Lee Salzman [:lsalzman] from comment #16)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]: no 

Setting qe-verify- based on Lee's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
(Assignee)

Updated

4 months ago
Depends on: 1365794
Blocks: 1365876
You need to log in before you can comment on or make changes to this bug.