1714846 - High CPU consumption when downloading multiple .mp4 files

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect)

Description

[u682798]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:89.0) Gecko/20100101 Firefox/89.0

Steps to reproduce:

1. Open Firefox
2. Download 5 or more .mp4 files at the same time

Actual results:

The CPU value goes up and the fans increase their RPM.

Expected results:

The CPU stays at normal values and the fans do not increase their RPM.

Comment 1

[Pablo]

Hi,

I checked some mp4 multiple downloads and i was able to replicate on MacOs 10.15 using :
Firefox 90.0b4 affected
Firefox Release 89 affected
Firefox Nightly 91.0a1 affected

Windows10 and Ubuntu are not affected.

Used this site to download multiple mp4s
filesamples.com/formats/mp4

cpu jumps to 100%

Comment 2

[Marco Bonardo [:mak]]

Could you please attach a log from the about:support page?

Comment 3

[Marco Bonardo [:mak]]

I'm sorry, before moving the bug, could you confirm you are downloading the mp4s and not previewing them in the browser?

Comment 4

[u682798]

(In reply to Marco Bonardo [:mak] from comment #3)

I'm sorry, before moving the bug, could you confirm you are downloading the mp4s and not previewing them in the browser?

Yes, I download the MP4 files but I do not see them in the browser preview.

Comment 5

[u682798]

Attachment: about:support

(In reply to Marco Bonardo [:mak] from comment #2)
> Could you please attach a log from the about:support page?

Comment 6

[u682798]

(In reply to Marco Bonardo [:mak] from comment #2)

Could you please attach a log from the about:support page?

I have attached the data present in the about:support page.

Comment 7

[Marco Bonardo [:mak]]

The log doesn't show anything particularly problematic, a see a few privacy options and clear on shutdown flipped on, but those are unlikely to cause problems with downloads.
There's also dns prefetch disabled, but again I'd expect that to just affect starting of the download. Just to be overzealous, I'd probably sugges to try a new profile and see if the problem happens there too. See https://support.mozilla.org/en-US/kb/profile-manager-create-remove-switch-firefox-profiles

What would be very useful here, is a performance profile when the bug happens. Could you please go to https://profiler.firefox.com/ and, following the instructions, start profiling before starting the downloads, and stop profiling after the problem happened. It should generate a profile link that you can add to the bug report and analyzing it we could find what is consuming cpu time.

Comment 8

[u682798]

(In reply to Marco Bonardo [:mak] from comment #7)

The log doesn't show anything particularly problematic, a see a few privacy options and clear on shutdown flipped on, but those are unlikely to cause problems with downloads.
There's also dns prefetch disabled, but again I'd expect that to just affect starting of the download. Just to be overzealous, I'd probably sugges to try a new profile and see if the problem happens there too. See https://support.mozilla.org/en-US/kb/profile-manager-create-remove-switch-firefox-profiles

What would be very useful here, is a performance profile when the bug happens. Could you please go to https://profiler.firefox.com/ and, following the instructions, start profiling before starting the downloads, and stop profiling after the problem happened. It should generate a profile link that you can add to the bug report and analyzing it we could find what is consuming cpu time.

Creating a new profile does not solve the issue.
Please Pablo, I do not have much time these days, since you were able to reproduce could you include some more details?

Comment 9

[Pablo]

Hello
this was captured with profiler

https://share.firefox.dev/3rj104s

altho on latest nightly i had to open more mp4 files since the cpu usage was not getting into 100% like some weeks ago when i tested it.
regards

Comment 10

[:Gijs (he/him)]

(In reply to Pablo from comment #9)

Hello
this was captured with profiler

https://share.firefox.dev/3rj104s

altho on latest nightly i had to open more mp4 files since the cpu usage was not getting into 100% like some weeks ago when i tested it.
regards

Hey Emilio, would you be able to have a look at this? It looks like we end up doing a style and paint pass every few ms, and I don't really understand why that is happening. There is code at https://searchfox.org/mozilla-central/rev/523ac2e17550f1f1da16aacdc6ac647af8b35508/browser/components/downloads/content/indicator.js#485-489 that would set a CSS variable repeatedly, but as far as I can tell the check at the start of that block (to see if aValue is different from the previous completion percentage) is working, so we shouldn't be changing the values that go into the conic gradient used for the progress display. What's more, the downloads from filesamples.com do not send a Content-Length so we can't even compute a completion percentage, so we never update the display anyway (bug 1717946). So I don't understand what is triggering the restyles/repaints that appear in the profile. Perhaps/hopefully it is more obvious to you?

Comment 11

[Florian Quèze [:florian]]

(In reply to :Gijs (he/him) from comment #10)

(In reply to Pablo from comment #9)

https://share.firefox.dev/3rj104s
It looks like we end up doing a style and paint pass every few ms, and I don't really understand why that is happening.

The "RefreshDriverTick" markers (happening at 60Hz) show "Tick reasons: HasObservers (DocumentTimeline animations [Style])", and we have "CSS animation - downloadProgressSlideX" markers. The "SetNeedStyleFlush" markers have "nsRefreshDriver::Tick" as the first frame of the stack, so I would say that there's nothing else that invalidates; it's just that we are playing the downloadProgressSlideX CSS animation.

(But I'm still interested in seeing if emilio can find other relevant information from the profile)

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Yep, that's it, it's a CSS animation. We don't run background-position animations on the compositor. Perhaps we can tweak that code to be a transform animation of sorts?

Comment 13

[:Gijs (he/him)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #12)

Yep, that's it, it's a CSS animation. We don't run background-position animations on the compositor. Perhaps we can tweak that code to be a transform animation of sorts?

Shouldn't the animation not be running while the panel is not visible?

Comment 14

[Florian Quèze [:florian]]

(In reply to :Gijs (he/him) from comment #13)

Shouldn't the animation not be running while the panel is not visible?

That reminds me bug 1693176.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Yep... That was a pain to get landed last time, but we should probably try again or something.

Comment 16

[Sam Foster [:sfoster] (he/him)]

I think we should have a bug on file to update the panel progress animations. These didn't make the MR1 scope, but we do have a new design for them, and can ensure that implementation animates on the compositor. I don't have the bug # handy...

Comment 17

[Emilio Cobos Álvarez (:emilio)]

I rebased bug 1693176 and try looked good so will try to reland it.

Comment 18

[:Gijs (he/him)]

(In reply to Sam Foster [:sfoster] (he/him) from comment #16)

I think we should have a bug on file to update the panel progress animations. These didn't make the MR1 scope, but we do have a new design for them, and can ensure that implementation animates on the compositor. I don't have the bug # handy...

bug 1699530 ? If not, we should ensure something is on file given this is duped. :-)

Comment 19

[Sam Foster [:sfoster] (he/him)]

(In reply to :Gijs (he/him) from comment #18)

bug 1699530 ? If not, we should ensure something is on file given this is duped. :-)

Yes that's the one. Its a bit thin on detail, but specs exist. I'm not sure if that's the MR1.1 team or the downloads panel folks though, which might be why its falling between the cracks right now.

Comment 20

[Florian Quèze [:florian]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #17)

I rebased bug 1693176 and try looked good so will try to reland it.

*** This bug has been marked as a duplicate of bug 1693176 ***

Emilio, bug 1693176 is fixed now, but I still see the downloadProgressSlideX CSS animation and the refresh driver ticks happening at 60Hz during the entire duration of a download, even though the animation is in an hidden panel.

Example profile on a recent Windows build (I also saw this on Mac) : https://share.firefox.dev/3mfUTvE
The steps to reproduce were just to start profiling, then starting the download of a massive file (so that it doesn't finish quickly), eg. http://test-debit.free.fr/10485760.rnd and closing any download panel.

In the profile, I notice that when the panel is open we paint at 60Hz (even though nothing changes on screen), and when the panel is closed we have the refresh driver ticks, but no restyle.

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Ouch, that's because of how shadow dom styling works, this declaration has more priority than the style attribute.

Let's see: https://treeherder.mozilla.org/#/jobs?repo=try&revision=84c0b349e6fdf32349d8050de8919a39442c3230

Comment 22

[:Gijs (he/him)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #21)

Ouch, that's because of how shadow dom styling works, this declaration has more priority than the style attribute.

Let's see: https://treeherder.mozilla.org/#/jobs?repo=try&revision=84c0b349e6fdf32349d8050de8919a39442c3230

Did this help / work? (I can't really tell from the trypush...) And should we move this bug to layout?

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Yeah, that helps but causes some test failures that I don't have time to investigate... It's definitely not a layout bug... I'd say it's either a Toolkit bug, or XUL (if it needs extra platform support to fix the tests).

Comment 25

[:Gijs (he/him)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #23)

Yeah, that helps but causes some test failures that I don't have time to investigate... It's definitely not a layout bug... I'd say it's either a Toolkit bug, or XUL (if it needs extra platform support to fix the tests).

It looks like the webextension panel tests are failing. If I had to guess, they probably fail because they rely on being able to load content in a <browser> inside a panel, and measure how big it is and have logic that resizes the browser and/or panel based on that. I expect that with the panel "really" display: none, the <browser> probably doesn't load any content? Rob, is there a way to exempt webextension panels from Emilio's patch, and/or do you know if there's some other way we could land this performance optimization without breaking webextensions?

Comment 26

[Markus Stange [:mstange]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #21)

Ouch, that's because of how shadow dom styling works, this declaration has more priority than the style attribute.

Does this mean that bug 1693176 didn't work? Or did it only work in some cases?

Comment 27

[Emilio Cobos Álvarez (:emilio)]

It didn't work, the patch in comment 21 makes it work and causes some failures.

Comment 28

[Rob Wu [:robwu]]

In response to comment 25: Looks like the <browser> elements inside the <panel> are not properly initialized when the <panel> is hidden with display:none. In the browser action case, there are even two nested <panel> elements (see tempPanel and panel (ViewPopup) mentioned below).

If I add logic to prevent these from being hidden, then the try push becomes somewhat green. There are some oranges, but when I ran them locally on macOS (artifact release build), the tests passed:
https://treeherder.mozilla.org/jobs?repo=try&revision=482f2d093f9b059fac08b975578b7977fabbe981

Patch: https://hg.mozilla.org/try/rev/edf23562741a7766e76b0c115b01351dfa99de3d

The main modifications that I did was making removing display:none for three specific elements:

• tempPanel at https://searchfox.org/mozilla-central/rev/ead7da2d9c5400bc7034ff3f06a030531bd7e5b9/browser/components/customizableui/content/panelUI.js#463
• panel (PanelPopup) at https://searchfox.org/mozilla-central/rev/ead7da2d9c5400bc7034ff3f06a030531bd7e5b9/browser/components/extensions/ExtensionPopups.jsm#490
• panel (ViewPopup) at https://searchfox.org/mozilla-central/rev/ead7da2d9c5400bc7034ff3f06a030531bd7e5b9/browser/components/extensions/ExtensionPopups.jsm#548

Side note: the logic at https://searchfox.org/mozilla-central/rev/ead7da2d9c5400bc7034ff3f06a030531bd7e5b9/toolkit/content/widgets/panel.js#62-69 can be simplified. The if (!this.arrowPanel)-branch can be removed, as the resulting shadow tree is identical these days (in the past it was different).

Comment 29

[:Gijs (he/him)]

Attachment: Bug 1714846 - Really hide panels on hide, r?robwu!,emilio!

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6e298bdd0dd8
Really hide panels on hide, r=robwu,emilio

Comment 31

[Atila Butkovits]

Backed out for causing assertion failures at CompositorBridgeParent.cpp.

Backout link: https://hg.mozilla.org/integration/autoland/rev/a780c70767faae5aef9505d49277b9a7a70766ea

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=RFftoIQBTvyd4l0x2y7qOw.0&revision=6e298bdd0dd8933ce165b620bac966b14e07a3e7

Failure log: https://treeherder.mozilla.org/logviewer?job_id=360120843&repo=autoland&lineNumber=6685

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8e5e86466731
Really hide panels on hide, r=robwu,emilio

Comment 33

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8e5e86466731

Comment 34

[Arthur K. (he/him)]

Will this get a beta uplift as well for 96?

Comment 35

[:Gijs (he/him)]

(In reply to Arthur K. [He/Him] from comment #34)

Will this get a beta uplift as well for 96?

Given 1 backout + relanding and now mysterious regressions, I don't think so.

Comment 36

[Arthur K. (he/him)]

I'm still seeing the high CPU usage when downloading, for example, an ISO from the VLSC. The OS >does< seem more responsive than before this fix but it seems, in my case, the fix hasn't quite crossed the finish line. Can anyone else confirm? I am testing with 97.0b8 x64.

Comment 37

[Arthur K. (he/him)]

If it's of any use, I uploaded a perf profile demonstrating the behavior here: https://share.firefox.dev/3IGlmw5

Comment 38

[Oana Ardelean]

Arthur K. , I encountered the same behavior as you when running on 97.0b8 x64. It seems that the CPU usage is lighter than in the affected builds but still significant for this fix.

Comment 39

[:Gijs (he/him)]

(In reply to Oana Ardelean from comment #38)

Arthur K. , I encountered the same behavior as you when running on 97.0b8 x64. It seems that the CPU usage is lighter than in the affected builds but still significant for this fix.

I think the remainder of what needs to happen here is tracked in bug 1742797.

Comment 40

[Arthur K. (he/him)]

(In reply to :Gijs (he/him) from comment #39)

(In reply to Oana Ardelean from comment #38)

Arthur K. , I encountered the same behavior as you when running on 97.0b8 x64. It seems that the CPU usage is lighter than in the affected builds but still significant for this fix.

I think the remainder of what needs to happen here is tracked in bug 1742797.

Thanks for the heads up Gijs.

Comment 41

[Arthur K. (he/him)]

(In reply to :Gijs (he/him) from comment #39)

(In reply to Oana Ardelean from comment #38)

Arthur K. , I encountered the same behavior as you when running on 97.0b8 x64. It seems that the CPU usage is lighter than in the affected builds but still significant for this fix.

I think the remainder of what needs to happen here is tracked in bug 1742797.

I just commented in bug 1742797 that it seems to have gotten worse in 100.0b. Just following up here as well.

Comment 42

[Arthur K. (he/him)]

This high CPU util is still present in 102 and 103b.