1357692 - High CPU usage and jerky animation on Firefox 53

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[CoolCmd]

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-

Comment 1

[Brian Birtles (:birtles)]

Mantaroh, do you mind triaging this to see if it is the right component or not?

Comment 2

[Mantaroh Yoshinaga[: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?

Comment 3

[CoolCmd]

(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

Comment 4

[Lee Salzman [:lsalzman]]

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. ???

Comment 5

[CoolCmd]

(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)

Comment 6

[CoolCmd]

open URL bar's context menu and choose 'paste & go'

Comment 7

[Lee Salzman [:lsalzman]]

Attachment: 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.

Comment 8

[Mason Chang [Inactive] [: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.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4c0c46ce1e9
followup - extra fuzz for D2D box-shadow reftest. r=me

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/120d8562d4a5
followup - extra fuzz for D2D box-shadow reftest. r=me a=merge

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5aa7f8838b2b
https://hg.mozilla.org/mozilla-central/rev/120d8562d4a5

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e4c0c46ce1e9

Comment 14

[Lee Salzman [:lsalzman]]

Can you check if this is fixed for you in new nightly builds?

Comment 15

[CoolCmd]

(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. :(

Comment 16

[Lee Salzman [:lsalzman]]

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

Comment 17

[Gerry Chang [:gchang]]

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.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/de9ea8491a29

Comment 19

[Andrei Vaida [:avaida]]

(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.