623739 - CPU usage spikes because of animated hidden images

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

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

When I open about:addons (and am currently viewing it in the selected tab), I see in task manager that firefox.exe is fairly constantly using ~20% CPU (it drops down to 0% every once in a while, but it holds almost constantly above 15%) for me, using a Core2Duo processor.

Disabling hardware acceleration didn't seem to make a difference, nor did safe mode.

I found that when I deleted the following element via Firebug, CPU usage dropped down to close to 0%:
/page/hbox[2]/box/deck/deck/vbox/hbox/image
(This is the empty <image> tag inside the "alert loading" <hbox>, which is in the "discover-loading" <vbox>, which is in the "discover-view" <deck>, in the "view-port" <deck>, in the "view-port-container" <box>.)

Mossop mentioned needing a profile to help identify what's going on here. I'm not quite sure what I need to do to create that profile, but I'm willing to help out with guidance.

Comment 1

[Dave Townsend [:mossop]]

Oh, don't tell me we're still animating hidden images?...

Comment 2

[Dave Townsend [:mossop]]

<Mossop> joe: Do we still animate images that aren't visible?
<joe> Mossop: yeah
* Mossop cries
<joe> if we don't we have to keep track of how far along they are in their animation

Probably going to want to block on this. We need to make images that are animated (the loading throbbers) switch to a blank image whenever they are hidden.

Comment 3

[Dave Townsend [:mossop]]

Or apparently display: none on an image will stop it animating (where visibility: hidden) will not)

Comment 4

[Ken]

Attachment: Example of: Add-on manager high CPU usage problem

High CPU usage (recently, at least on one cpu (I have only 1)), when displaying about:addons (any page, ex: Get add-ons; Extensions...). Using AMD XP 2600.

If I remember correctly problem seemed to have started when 'Bug 611241 - xul:image with a SVG filter won't display on "about:" pages' landed.

This bug landing seemed to have caused other bugs such as:
    Bug 621253 - "Extension"/"Plugins" pane of "Add-ons Manager" is drawn wrongly if zoom level not equal 100% in site specific zoom and disabled extension/plugin existed.

Comment 5

[Csaba Kozák [:WonderCsabo]]

I'm experiencing this, too. This also makes addons list scrolling extremely slow and laggy.

Comment 6

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

Attachment: would this work?

I have no idea if this would even work.

Something along these lines?

Comment 7

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

Comment on attachment 501932 [details] [diff] [review]
would this work?

Could've sworn I hit the "patch" checkbox the first time around...

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #5)
> I'm experiencing this, too. This also makes addons list scrolling extremely
> slow and laggy.

That's bug 623615 and as it looks like not caused by the animated images.

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 501932 [details] [diff] [review]
would this work?

It's along the right lines but I imagine you'll then still see the CPU use if the discovery pane is visible but the loading message hidden.

You can I think simplify it by doing something like

#discover-view:not(selectedIndex="<whatever this is>") .loading {
  display: none
}

There is also two other loading throbbers in the UI, do they need the same treatment?

Comment 11

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

(In reply to comment #10)
> 
> There is also two other loading throbbers in the UI, do they need the same
> treatment?

Only deleting the discovery view throbber via Firebug helped with CPU usage for me, for whatever reason.

> Henrik Skupin [:whimboo]: Keywords: perfHardware: x86 ⇒ All OS: Windows 7 ⇒ All
Is this affecting non-Windows builds too? If it does, the patch will have to be elsewhere or in all the themes.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

No, sorry about that. I have changed it because of the wrongly duped bug.

Comment 13

[Dave Townsend [:mossop]]

Yeah the patch should go into the extensions.css in content rather than the theme code.

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

I could only see a 3% to 4% CPU hit from this on my machine (and even then, only once every few seconds). But I believe this fixes it. In addition to hiding the discovery pane throbber, I've moved the code that hides the details pane loading message out of themes and into content, and made it hidden rather than just collapsed. AFAIK, every other throbber is already hidden when its not visible.

Since the performance hit on my machine (3-4%) was so little compared to the reporter's (20%), I want to double check this improves things. I'll have Try builds up tomorrow for people to test.

Comment 15

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

The w32 tryserver build has no CPU spikes that I can see from a quick testing on my machine.
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-4554a2185bf9/

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #15)
> The w32 tryserver build has no CPU spikes that I can see from a quick testing
> on my machine.

Great - thanks for checking :)

Comment 17

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Landed: https://hg.mozilla.org/mozilla-central/rev/2570a56d5786

Would there normally be a litmus test for this sort of thing? Might be useful to have something generic, checking CPU usage doesn't go up unexpectedly.

Comment 18

[Alfred Kayser]

It should have been in content in the first place, and secondly using display:none is always better than visibility:hidden IMHE.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I can't see the spikes anymore. Verfied fixed with Mozilla/5.0 (Windows NT 6.1; rv:2.0b10pre) Gecko/20110112 Firefox/4.0b10pre

I don't think it is worth a Litmus test. We know why it happens, we have solved the problem, and it's noted in the code. Also wouldn't it be safer when even "!important" is specified for the rule?

Comment 20

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Maybe, but those rules are pretty specific - much more specific than anything you'd use to style what they hide (more specific rules win, in CSS). I always try to avoid using !important whenever possible - it seems to do more harm than good, in my experience.