Closed Bug 1354913 Opened 8 years ago Closed 6 years ago

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

Categories

(Core :: Graphics, defect, P3)

Product:
Core
Component:
Graphics
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla68
Project Flags:
Performance Impact low
Tracking Flags:
Tracking Status
firefox-esr60 --- wontfix
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- fixed

People

(Reporter: jfkthame, Assigned: whawkins)

References

Details

(Keywords: perf, perf:resource-use, Whiteboard: [gfx-noted])

Attachments

(1 file)

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
(deleted), text/x-phabricator-request
Details
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.
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.
Component: General → Layout
Product: Firefox → Core
[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.
Whiteboard: [qf:investigate:p1]
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.
See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=1356533
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.
(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
Component: Layout → Graphics
Summary: Excessive CPU usage whenever the About window is open → Animated GIFs and animated PNGs tick the refresh driver even when they are hidden
Priority: -- → P3
Whiteboard: [qf:investigate:p1] → [qf:investigate:p1][gfx-noted]
Assignee: nobody → aosmond
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.
Whiteboard: [qf:investigate:p1][gfx-noted] → [qf:p3:resource][gfx-noted]

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

Summary: Animated GIFs and animated PNGs tick the refresh driver even when they are hidden → Animated GIFs and animated PNGs tick the refresh driver even when they are hidden in a XUL deck
Attachment #9053035 - Flags: feedback?(jfkthame)
Attachment #9053035 - Flags: feedback?(dholbert)
Attachment #9053035 - Flags: feedback?(aosmond)

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.

Flags: needinfo?(whawkins)
Attachment #9053035 - Flags: feedback?(jfkthame)
Attachment #9053035 - Flags: feedback?(dholbert)
Attachment #9053035 - Flags: feedback?(aosmond)
Assignee: aosmond → whawkins

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.

(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

Flags: needinfo?(whawkins)
Flags: needinfo?(dholbert)
Flags: needinfo?(aosmond)

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.

Flags: needinfo?(dholbert)
Flags: needinfo?(aosmond)

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

Flags: needinfo?(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.)

Flags: needinfo?(dholbert) → needinfo?(whawkins)
Attachment #9053035 - Attachment description: Bug 1354913: Animated GIFs and animated PNGs tick the refresh driver even when they are hidden in a XUL deck r=dholbert,aosmond,jfkthame → 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

(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

Flags: needinfo?(whawkins) → needinfo?(dholbert)

Looks great. Landing queued; should hit autoland shortly.

Flags: needinfo?(dholbert)
Pushed by dholbert@mozilla.com: 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

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

Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox68: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

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

status-firefox66: --- → wontfix
status-firefox67: --- → wontfix
status-firefox-esr60: --- → wontfix
Regressions: 1550437
Performance Impact: --- → P3
Keywords: perf:resource-use
Whiteboard: [qf:p3:resource][gfx-noted] → [gfx-noted]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: