1354913 - Animated GIFs and animated PNGs tick the refresh driver even when they are hidden in a XUL deck

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[Jonathan Kew [:jfkthame]]

In current Nightly (55.0a1) on macOS, I'm seeing high CPU usage for the chrome process whenever the About Nightly window is open. Simply opening "About Nightly" causes CPU usage as shown in Activity Monitor to jump from an idle value of around 10% to about 135-140%.

Profile: https://perfht.ml/2nR2aSz

From this profile, it looks to me like we're firing the refresh driver far too often whenever the About window is open, even though there's nothing to do.

This applies even though the About window is showing entirely static content -- not even doing a check-for-updates -- and remains true even if the window is entirely hidden. It's all too easy to accidentally leave the About window open but hidden, by simply clicking another browser window that comes in front of it, and it will then merrily chew through CPU cycles (and battery life) for no useful purpose.

Comment 1

[Jonathan Kew [:jfkthame]]

Moving this to Core:Layout to try and get some more knowledgeable eyes on it, as I believe that's where the refresh driver lives, though presumably the root cause of us ticking it too frequently might lie elsewhere.

Comment 2

[(no longer active)]

[qf:investigate:p1] since triggering the refresh driver needlessly might be caused by a bug that shows up in more than just the About window.

Comment 3

[Jonathan Kew [:jfkthame]]

See also bug 1356533, which shows a similar example of firing the refresh driver excessively (on an eBay page, in that case), though currently it's not clear to me whether the underlying cause is related to this one or something entirely independent.

Comment 4

[Mantaroh Yoshinaga[:mantaroh]]

This is related with bug1356533, If we set ASAP mode(layout.frame_rate = 0), the tick of refresh driver is frequently happen.

However I think that about dialog doesn't have the animation now, however refresh driver is active.

I Investigated aboutDialog.xul. If we remove or hidden the <image> element from about dialog, the tick will be quiet.
This image is APNG, and parent element is deck.[1]

[1] https://dxr.mozilla.org/mozilla-central/rev/f7adbf457ee20eeffde72694e0d17d73616e3cfd/browser/base/content/aboutDialog.xul#58-109

Perhaps, I think that this APNG is cause refresh driver tick and deck has these frame always.

Comment 5

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #4)
> This is related with bug1356533, If we set ASAP mode(layout.frame_rate = 0),
> the tick of refresh driver is frequently happen.
> 
> However I think that about dialog doesn't have the animation now, however
> refresh driver is active.
> 
> I Investigated aboutDialog.xul. If we remove or hidden the <image> element
> from about dialog, the tick will be quiet.
> This image is APNG, and parent element is deck.[1]
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> f7adbf457ee20eeffde72694e0d17d73616e3cfd/browser/base/content/aboutDialog.
> xul#58-109
> 
> Perhaps, I think that this APNG is cause refresh driver tick and deck has
> these frame always.

I reproduced this phenomenon without about window. If we load the content which has many APNG resource, CPU usage is increase.
Profiler : https://perfht.ml/2sg4yFZ

This phenomenon will occur on other animation image(e.g. animation gif).
Profiler : https://perfht.ml/2sguEIW

If we have animation image, refresh driver will have image requester. I think that we will animate and paint every tick when refresh driver has this requester. [1]

[1] http://searchfox.org/mozilla-central/rev/1a0d9545b9805f50a70de703a3c04fc0d22e3839/layout/base/nsRefreshDriver.cpp#1940-1960

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

In the about dialog, there is a XUL deck element called "updateDeck." Several of its children use the update throbber image, which is the animated image in question. While only one of the children is painted at any given time, the actual nsImageBoxFrames continue to exist for each of these children using the throbber despite not being visible. If the frames exist, they will register the animated image with the refresh driver to advance the animation on each tick.

This is consistent with how animated images work with nsImageFrame, nsBulletFrame, etc. If the frame exists and the image is animated, it will be registered with the refresh driver, regardless of the visibility of the image itself.

What confuses me are collected profiles. They don't match with what I have collected on Linux and on Mac using nightly. The original profiles suggest a consistent sampling error where the impact of the image lookup machinery and related mutexes are overstated; they do show up in my own profiles on Mac but not to the same degree. Most of the work is done in advancing the animation (it needs to blend the next frame with the current) and repainting (unnecessarily in this case), which is what I would expect.

Bug 1337111 will allow us to blend the frames on the image decoder threads *once* instead of on every frame advance on the main thread. This will eliminate most of the cost on the imgIContainer::RequestRefresh path.

I have some simple patches to avoid the mutex lock and a malloc which appeared in the attached profiles. The changes are simple enough, even though I'm not convinced it will have a significant impact (unless there was a lot more contention on that mutex I unaware of).

This just leaves the visibility problem. Theoretically that is similar to bug 1429728 where an animated image we don't even see causing the refresh tick to advance the animation and repaint.

Comment 7

[Daniel Holbert [:dholbert]]

This is still an issue, BTW.

Here are two profiles showing the "Marker Chart" view, filtered for the RefreshDriverTick row...

1. Without about dialog open: http://bit.ly/2XbqLV7
2. With about dialog open: http://bit.ly/2Xeej6U

Profile #2 (with the about dialog open) shows blobs for continuous RefreshDriverTick events (with each one being extremely short).

Comment 8

[Will Hawkins]

Attachment: Bug 1354913: Fix nsDeckFrame, nsImageBoxFrame and nsImageFrame so that nsDeckFrame does not tick the refresh driver when its child images are animated and hidden. r=dholbert,aosmond,jfkthame

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 9053035 [details]
Bug 1354913: Fix nsDeckFrame, nsImageBoxFrame and nsImageFrame so that nsDeckFrame does not tick the refresh driver when its child images are animated and hidden. r=dholbert,aosmond,jfkthame

Thanks for fixing this! Looks like aosmond r+'d this, so I haven't bothered to take a closer look.

--> Canceling feedback requests, but let us know if there's anything in particular that needs further feedback here.

Comment 10

[Daniel Holbert [:dholbert]]

Also: this probably wants a Try run, and then (if that goes well) is probably ready to be autolanded via Lando!

Let me know if you need help with either of those.

Comment 11

[Will Hawkins]

(In reply to Daniel Holbert [:dholbert] from comment #10)

Also: this probably wants a Try run, and then (if that goes well) is probably ready to be autolanded via Lando!

Let me know if you need help with either of those.

Thanks for the offer to help, dholbert! I do need your assistance, actually. I have the results of a try run, but I am still not sure about how to interpret them and whether they are "successful". Here are the results

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f2f93a5cc5b12453ca4e254b13337d20587153f&selectedJob=235674768

Things look "good", but I would really appreciate your help in deciding if that's true!

Thanks again, everyone!

Will

Comment 12

[Daniel Holbert [:dholbert]]

Yeah, I think that Try run looks "good". All the failures seem to be known-intermittents or unrelated and intermittent-looking.

(If there were any that look suspicious/unsure, you could "retrigger" the particular test-run (with the little circular arrow near bottom-right when you've clicked a testrun), to run it again & hopefully out if the failure was a sporadic one-off issue or not.)

For now I think you're OK to land on Lando. Let me know if you'd like me to do that, and I'll be happy to.

Comment 13

[Will Hawkins]

Thanks, dholbert! I'll let you do the Lando honors :-)

Comment 14

[Daniel Holbert [:dholbert]]

Aha, sorry -- one nit that I noticed when about to trigger lando: the commit message should describe the change, but right now it instead describes the bug. I think it's just this bug title, which typically does not make a good commit message.

See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Checkin_comment including "Good" / "Bad" examples.

Would you mind updating the commit message to briefly describe what you're actually changing? You can do that directly in phabricator, via "Edit Revision" at top-right.

(If you want to see recent sample commit messages, you can look at treeherder; not everyone follows this best-practice guideline, but they should.)

Comment 15

[Will Hawkins]

(In reply to Daniel Holbert [:dholbert] from comment #14)

Aha, sorry -- one nit that I noticed when about to trigger lando: the commit message should describe the change, but right now it instead describes the bug. I think it's just this bug title, which typically does not make a good commit message.

See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Checkin_comment including "Good" / "Bad" examples.

Would you mind updating the commit message to briefly describe what you're actually changing? You can do that directly in phabricator, via "Edit Revision" at top-right.

(If you want to see recent sample commit messages, you can look at treeherder; not everyone follows this best-practice guideline, but they should.)

Thank you for pointing this out! I thought that I was doing the right thing by quoting the bug title in the revision, but now I see the the difference!

I modified it and I think that I provided a solid message. Let me know what you think.

Thanks again for the mentorship on this.

Will

Comment 16

[Daniel Holbert [:dholbert]]

Looks great. Landing queued; should hit autoland shortly.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f5dd833c3b60
Fix nsDeckFrame, nsImageBoxFrame and nsImageFrame so that nsDeckFrame does not tick the refresh driver when its child images are animated and hidden. r=aosmond

Comment 18

[Raul Gurzau (:RaulG)]

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

Comment 19

[Chris Peterson [:cpeterson]]

67=wontfix because I assume we don't need to uplift this fix to 67 Beta.