Closed Bug 1454149 Opened 3 years ago Closed 3 years ago

Categories

(Core :: ImageLib, defect, P3)

defect

Tracking

()

VERIFIED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox59 --- unaffected
firefox60 --- unaffected
firefox61 + fixed
firefox62 + fixed
firefox65 --- verified
firefox66 --- verified
firefox67 --- verified

People

(Reporter: mayankleoboy1, Assigned: aosmond)

References

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(1 file, 2 obsolete files)

go to  https://blog.github.com/2016-08-19-building-your-first-atom-plugin/
Let the GIF load
Watch the CPU usage

uses 75-100% of my CPU
https://perfht.ml/2ELbbFd
Flags: needinfo?(aosmond)
Regression from bug 523950. Unfortunately unrelated to bug 1444537 from what I can tell (it would be nice to have an STR).

CPU usage is high even in release, but at least drops off as the decoding completes, at the cost of using a few GB of RAM. However comparing to Chrome, I see that it doesn't even advance animated images which are not in view. I think that should be the solution.
Depends on: 523950
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(aosmond)
Priority: -- → P3
Whiteboard: [gfx-noted]
I think this is the simplest change I can make which should fix the problem, in addition to making this more efficient in general for animated images outside the current view.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1d87311384c4c433aac6f8427fdc9437cd045bf

This patch will make us ignore refresh ticks for an animated image if nothing is accessing its composited frame. This should work for all frame accessing paths for a RasterImage -- Draw, GetFrame, GetFrameAtSize, GetImageContainer, UpdateImageContainer. The image container case will still have a high CPU usage if the entity which requested the image container keeps it around, but doesn't otherwise use it (bad entity!); since animated images shouldn't be layerized, I don't think this is a big concern (correct me if I'm wrong -- we still need to solve this for WebRender as well!).

I think it should be upliftable to beta, assuming the try is green, but I'll let my reviewer make that call. If it is too risky, we need to increase image.animated.decode-on-demand.threshold-kb to the highest value for beta to delay the feature to the next release.
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Attachment #8968293 - Flags: review?(tnikkel)
See Also: → 1454824
Comment on attachment 8968293 [details] [diff] [review]
0001-Bug-1454149-Do-not-advance-animated-images-which-are.patch, v1

Disable review for now. We discussed this on IRC, and it definitely needs more work.
Attachment #8968293 - Flags: review?(tnikkel)
This should take care of discarded images, as well as adding a pref to disable this feature if it causes problems after making its way into a build.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6703abeb8756257005433e07b3d421a0e2ed2e7

I still need to dig into the bug history to find the reasons why we hesitated to change this behaviour in the past.
Attachment #8968293 - Attachment is obsolete: true
I can't find any bugs discussing the "we should advance animated images when out of view" issue. Maybe you can dig it up :).
Attachment #8970294 - Attachment is obsolete: true
Attachment #8970660 - Flags: review?(tnikkel)
The landing of bug 1454824 should have mitigated the problem on 60, so I'm marking it as unaffected. We'll still want a patch for 61 though.
Where are we on review?
Flags: needinfo?(aosmond)
(In reply to Marion Daly [:mdaly] from comment #7)
> Where are we on review?

I decided to disable the regressing feature, just landed the pref change so it should make it into 61 beta as normal. Once it is merged I'll mark 61 as unaffected here.
Flags: needinfo?(aosmond)
Attachment #8970660 - Flags: review?(tnikkel) → review+
(In reply to Andrew Osmond [:aosmond] from comment #5)
> I can't find any bugs discussing the "we should advance animated images when
> out of view" issue. Maybe you can dig it up :).

I did a quick look, didn't find anything.
This is technically "fixed" for all affected versions now since the disabling landed on m-c directly, but I'm assuming we want to leave this bug open for fixing the underlying problem (and re-enabling it for 62?).

In the mean time, requesting QA to at least give this a spot check on 61 betas to ensure that things are looking good there again.
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba377bd503e1
Do not advance animated images which are not displayed. r=tnikkel
https://hg.mozilla.org/mozilla-central/rev/ba377bd503e1
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
The issue is still reproducible for me on 61.0b3 and 62.0a1 (2018-05-09). From what I observed, the cpu usage is around 60% utilization when scrolling the page, and firefox's ram usage is jumping all around (as example one second is at 1.5gb usage, then jumps to 700mb, then back up to 2.0gb). If I stop scrolling, the usage stays still at around 700mb, but as soon as the scrolling starts, the memory jumps back to a higher value. (Windows 10 system - Amd FX8320, 16gb DDR3, Radeon Integrated HD3000). 
On macOS 10.12 and 10.13, the cpu usage stays at 130-140%, but the memory usage is not touched.
(In reply to Sasca Catalin, QA [:csasca] from comment #14)
> The issue is still reproducible for me on 61.0b3 and 62.0a1 (2018-05-09).
> From what I observed, the cpu usage is around 60% utilization when scrolling
> the page, and firefox's ram usage is jumping all around (as example one
> second is at 1.5gb usage, then jumps to 700mb, then back up to 2.0gb). If I
> stop scrolling, the usage stays still at around 700mb, but as soon as the
> scrolling starts, the memory jumps back to a higher value. (Windows 10
> system - Amd FX8320, 16gb DDR3, Radeon Integrated HD3000). 
> On macOS 10.12 and 10.13, the cpu usage stays at 130-140%, but the memory
> usage is not touched.

Is the beta CPU usage high compared to 59? This was never a great page for us. Chrome should be similar as long as an animation is not in view. I expect memory usage to be very high on release/beta.

Today's nightly should similarly to Chrome. The memory footprint should be lower than beta, and the CPU footprint somewhat higher (there is a tradeoff for the lower memory usage). Unfortunately we aren't quite as good as Chrome with respect to out of view animated images, but if you stop scrolling around "Activation commands", the CPU should also hit zero. This is because we appear to be prerendering more parts above and below the current view, and it captures the animated images more frequently.

To confirm, the expected preferences on nightly are:

image.animated.decode-on-demand.threshold-kb == 20480
image.animated.resume-from-last-displayed == true
Hi Andrew. On 59.0.3 and 61.0b3, the cpu gets to 50%, then it slowly stabilizes at around 20%. On 62.0a1 (2018-05-09) it stays at around 35-40% cpu usage. The ram is jumping all around on every firefox build and if the page is scrolled all the way down, firefox has a ram usage of around 4gb and is jumping to 2gb then to 3gb, again to 4gb and so on. Chrome tops it's cpu usage to 25-30% and the memory usage is nearly untouched (200-400mb).
(In reply to Sasca Catalin, QA [:csasca] from comment #16)
> Hi Andrew. On 59.0.3 and 61.0b3, the cpu gets to 50%, then it slowly
> stabilizes at around 20%. On 62.0a1 (2018-05-09) it stays at around 35-40%
> cpu usage. The ram is jumping all around on every firefox build and if the
> page is scrolled all the way down, firefox has a ram usage of around 4gb and
> is jumping to 2gb then to 3gb, again to 4gb and so on. Chrome tops it's cpu
> usage to 25-30% and the memory usage is nearly untouched (200-400mb).

I think bug 1382683 is going to help with the CPU usage. The memory is surprisingly, I'll need to think further on that.
See Also: → 1382683
Hi Andrew. Have you managed to find something about memory usage?
Flags: needinfo?(aosmond)
I think that recycling might help with the memory usage. For non-WebRender users, everything you need is already in tree (nightly only). All you need to do is set image.animated.generate-full-frames to true, restart the browser, and check the memory usage. Since we are reusing the buffers now, the footprint shouldn't be constantly jumping around and thus might look better in the snapshots.
Flags: needinfo?(aosmond)

Verified as fixed on Firefox Nightly 67.0a1 (2019-02-07) on Windows 10(64-bit)

**On a side note the memory usage still fluctuates anywhere from 1 GB to 3 GB

Status: RESOLVED → VERIFIED

I've also verified it on Firefox 65.0(2019-01-24) and Firefox 66.0b5(2019-02-04)

Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.