Images on slow network continously use 1 full core.

VERIFIED FIXED in Firefox 22

Status

()

Core
ImageLib
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: Mayank Bansal, Assigned: rclick)

Tracking

(Blocks: 1 bug, {regression})

22 Branch
mozilla23
x86_64
Windows 7
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox21 unaffected, firefox22+ verified, firefox23 verified)

Details

(Whiteboard: [uplift with 859718])

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

4 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130325 Firefox/22.0
Build ID: 20130325105600

Steps to reproduce:

1. Create fresh profile.
2. Go to http://images.anandtech.com/doci/6857/7970_BLACK_PCB_678x452.png
3. Monitor task manager.


Actual results:

FF continously uses 1 complete core. The image is small in size, ~200KB.


Expected results:

Not so.
(Reporter)

Comment 1

4 years ago
Something like if an image is on a very slow network, it is being continously processed or something.
This ~200KB took 22 seconds to load, using 1 full core the whole time.

Profile : http://people.mozilla.com/~bgirard/cleopatra/#report=e30419955389892340f66c0f5ba8c4441769d221



This also occured to me on 9gag, when i was using a 8KB/s internet connection.
Blocks: 716140

Comment 2

4 years ago
Me too facing the same problem on medium connection
or pages with loads of thumbnails

Comment 3

4 years ago
add this as a blocker to 854394

Comment 4

4 years ago
Visit Fb.com
see cpu usage Goto 75%-100%
no matter how much decoding/rendering is done
Cpu/Gpu usage should be minimal(plus save heating & battery)
(Reporter)

Comment 5

4 years ago
(In reply to Sillius Soddus from comment #4)
> Visit Fb.com
> see cpu usage Goto 75%-100%
> no matter how much decoding/rendering is done
> Cpu/Gpu usage should be minimal(plus save heating & battery)


Please stop spamming this bug.
If you believe there is a problem in Firefox, file a *new* bug.
Does playing with the image.multithreaded_decoding.limit Pref (e.g. set to 1) make any Difference?
(Reporter)

Comment 7

4 years ago
cant check now :(
The load on the network has reduced, so the image streams in a second.

Can anyone host this image on a speed-limited server, and try to repro ?
(Reporter)

Comment 8

4 years ago
OK, i reproduced this by installing netbalancer from here : http://seriousbit.com/netbalancer/ 

I limit the upload and download limit of Nightly to 6KB/s.

Then i go to the  URL ( you may have to ctrl+F5 to bypass the disk cache)

Result : 

Nightly  -- >  100% CPU usage while the image loads (1 core)
Release 19  -->  2-3% load.

100% load reproducible with  image.multithreaded_decoding.limit set to both the default value (-1) and (1) as suggested in comment 6.
(Assignee)

Comment 9

4 years ago
(In reply to Sillius Soddus from comment #3)
> add this as a blocker to 854394

Bug 854394 is about tuning when we do synchronous decoding on the main thread, which is unrelated to this bug.
(Reporter)

Comment 10

4 years ago
This is happening to me a lot, because i have a poor ISP who reduce speed or make some sites slow to load.
Issue manifests as 100% load on one core, resulting in laptop heating up and fan whirring.

(This does not affect the snappy-ness of the browser in any way though.)

Comment 11

4 years ago
(In reply to mayankleoboy1 from comment #10)
> This is happening to me a lot, because i have a poor ISP who reduce speed or
> make some sites slow to load.
> Issue manifests as 100% load on one core, resulting in laptop heating up and
> fan whirring.
> 
> (This does not affect the snappy-ness of the browser in any way though.)

You may temporarily disable image.multithreaded_decoding.enabled

Comment 12

4 years ago
Confirmed on restricted network 9kbps VMware Player.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 13

4 years ago
Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/bef19bca23f9
Mozilla/5.0 (Windows NT 5.1; rv:22.0) Gecko/20130325 Firefox/22.0 ID:20130325015122
Bad:
http://hg.mozilla.org/mozilla-central/rev/4d3250f3afea
Mozilla/5.0 (Windows NT 5.1; rv:22.0) Gecko/20130325 Firefox/22.0 ID:20130325093524
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bef19bca23f9&tochange=4d3250f3afea


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/71fe5b69c834
Mozilla/5.0 (Windows NT 5.1; rv:22.0) Gecko/20130324 Firefox/22.0 ID:20130324205822
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/1541b7a03e17
Mozilla/5.0 (Windows NT 5.1; rv:22.0) Gecko/20130324 Firefox/22.0 ID:20130324220925
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=71fe5b69c834&tochange=1541b7a03e17

In local build:
Last Good: 71fe5b69c834
First Bad: 9e2bdda8c3ca
Triggered by:
  9e2bdda8c3ca Joe Drew — Bug 716140 - Implement multithreaded decoding using a thread pool. r=seth


Strangely, image.multithreaded_decoding.enabled=false does not help.
(Assignee)

Comment 14

4 years ago
I can reproduce using netbalancer to simulate a slow network. I'm looking into it.
Assignee: nobody → rclickenbrock
Status: NEW → ASSIGNED
(Assignee)

Comment 15

4 years ago
Created attachment 731774 [details] [diff] [review]
Part 1: Account for more decoder state in RasterImage::IsDecodeFinished()

IsDecodeFinished can sometimes be wrong because it checks the RasterImage's size and decode done state, which might not have been synchronized yet. The solution is to check the decoder's equivalent state.

Additionally, I added a check for the case where we've allocated a new frame but haven't yet flushed the data the decoder set aside. We do check for this elsewhere, but not consistently. Checking for it in IsDecodeComplete makes it so.
Attachment #731774 - Flags: review?(seth)
(Assignee)

Comment 16

4 years ago
Created attachment 731783 [details] [diff] [review]
Part 2: Don't try to enqueue more decoding from DecodeDoneWorker

The problem here is that DecodeDoneWorker always requests more decoding, even if we've decoded all the data we have and we're waiting on the network to send more. What ends up happening is DecodeDoneWorker enqueues a DecodeJob, and that dispatches another DecodeDoneWorker... We constantly do this in a loop until we receive enough data to finish the decode.

The solution I've opted for is to simply not trigger more decoding. DecodeDoneWorker only gets dispatched after we've done all the decoding we can, so it shouldn't be necessary.
Attachment #731783 - Flags: review?(seth)
Comment on attachment 731774 [details] [diff] [review]
Part 1: Account for more decoder state in RasterImage::IsDecodeFinished()

Review of attachment 731774 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

::: image/src/RasterImage.cpp
@@ +3343,5 @@
>  RasterImage::IsDecodeFinished()
>  {
>    // Precondition
>    NS_ABORT_IF_FALSE(mDecoder, "Can't call IsDecodeFinished() without decoder!");
> +  MOZ_ASSERT(mDecodeRequest);

I think we should add mDecodingMutex.AssertCurrentThreadOwns() here. If we don't hold the lock then a lot of this stuff is unsafe. (We always do hold the lock right in IsDecodedFinished right now, but it'd be good to catch this if something changes in the code later.)
Attachment #731774 - Flags: review?(seth) → review+
Comment on attachment 731783 [details] [diff] [review]
Part 2: Don't try to enqueue more decoding from DecodeDoneWorker

Review of attachment 731783 [details] [diff] [review]:
-----------------------------------------------------------------

I concur, this should be fine. AddSourceData should take care of calling RequestDecode in the situations where it's necessary as soon as new data arrives.
Attachment #731783 - Flags: review?(seth) → review+
(Assignee)

Comment 19

4 years ago
Thanks for the review!

(In reply to Seth Fowler [:seth] from comment #17)
> I think we should add mDecodingMutex.AssertCurrentThreadOwns() here. If we
> don't hold the lock then a lot of this stuff is unsafe. (We always do hold
> the lock right in IsDecodedFinished right now, but it'd be good to catch
> this if something changes in the code later.)

I completely agree. Will do.
(Assignee)

Comment 20

4 years ago
This had a mostly green try run (https://tbpl.mozilla.org/?tree=Try&rev=8360f6cd74a9). I suspect the failures aren't caused by my patches, though I will look closer to make sure.

Builds are available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/rclickenbrock@gmail.com-8360f6cd74a9 if anyone wants to test them.
(Assignee)

Updated

4 years ago
Duplicate of this bug: 856602
The try run looks good to me. I'd love to get this landed ASAP.
(Assignee)

Comment 23

4 years ago
Created attachment 733093 [details] [diff] [review]
Part 1: Account for more decoder state in RasterImage::IsDecodeFinished()

Addressed review comments and added r=seth to commit message.
Attachment #731774 - Attachment is obsolete: true
Attachment #733093 - Flags: review+
(Assignee)

Comment 24

4 years ago
Created attachment 733095 [details] [diff] [review]
Part 2: Don't try to enqueue more decoding from DecodeDoneWorker

Added r=seth to commit message.
Attachment #731783 - Attachment is obsolete: true
Attachment #733095 - Flags: review+
(Assignee)

Comment 25

4 years ago
Requesting check-in.

The assertion added in part 1 shouldn't need another try run because there's no way it could possibly fail without other already existing assertions also failing.
Keywords: checkin-needed
(Assignee)

Comment 26

4 years ago
It would help if I remembered to actually qref the patch...
Keywords: checkin-needed
(Assignee)

Comment 27

4 years ago
Created attachment 733098 [details] [diff] [review]
Part 1: Account for more decoder state in RasterImage::IsDecodeFinished()

The right patch this time. Sorry for the bugspam.
Attachment #733093 - Attachment is obsolete: true
Attachment #733098 - Flags: review+
(Assignee)

Comment 28

4 years ago
Created attachment 733099 [details] [diff] [review]
Part 2: Don't try to enqueue more decoding from DecodeDoneWorker
Attachment #733095 - Attachment is obsolete: true
Attachment #733099 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #733099 - Attachment is patch: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Great! Pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2dc2f5db57ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/29f935fba166
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2dc2f5db57ea
https://hg.mozilla.org/mozilla-central/rev/29f935fba166
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23

Comment 31

4 years ago
(In reply to Robert Lickenbrock [:rclick] from comment #28)
> Created attachment 733099 [details] [diff] [review]
> Part 2: Don't try to enqueue more decoding from DecodeDoneWorker

(In reply to Seth Fowler [:seth] from comment #29)
> Great! Pushed to inbound:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/2dc2f5db57ea
> https://hg.mozilla.org/integration/mozilla-inbound/rev/29f935fba166

(In reply to Ryan VanderMeulen [:RyanVM] from comment #30)
> https://hg.mozilla.org/mozilla-central/rev/2dc2f5db57ea
> https://hg.mozilla.org/mozilla-central/rev/29f935fba166

*new profile*
Still seeing very high Cpu usage on Site with Images
Continuous 40% to 60% CPU usage
also on a side not
It feels as if the images reload from the server every time FF23 is started after browsing some sites closing FF,again visiting same sites?
(bypassing cache thought about:cache shows caches being saved)
beta version seems Fine

Comment 32

4 years ago
Created attachment 734147 [details]
Still High CPU usage/two screenshots/on new profile

Still High CPU usage with sites with images//two screenshots//New Profile

Comment 33

4 years ago
Created attachment 734149 [details]
On windows/Linux with a fresh profile HWA disabled
(In reply to ElevenReds from comment #32)
> Created attachment 734147 [details]
> Still High CPU usage/two screenshots/on new profile
> 
> Still High CPU usage with sites with images//two screenshots//New Profile

Please file a new bug with specific steps to reproduce the issue. I'd prefer if you would wait 24 hours, though, and then ensure that you can still reproduce the issue with the next Nightly release. A lot of bugs related to image decoding have been fixed within the past day or so.

FWIW, I cannot reproduce any of the problems listed on this bug anymore.

Comment 35

4 years ago
(In reply to Seth Fowler [:seth] from comment #34)
> (In reply to ElevenReds from comment #32)
> > Created attachment 734147 [details]
> > Still High CPU usage/two screenshots/on new profile
> > 
> > Still High CPU usage with sites with images//two screenshots//New Profile
> 
> Please file a new bug with specific steps to reproduce the issue. I'd prefer
> if you would wait 24 hours, though, and then ensure that you can still
> reproduce the issue with the next Nightly release. A lot of bugs related to
> image decoding have been fixed within the past day or so.
> 
> FWIW, I cannot reproduce any of the problems listed on this bug anymore.


Compared to FF21beta isnt the CPU usage far more(attachment)

Sure will wait :)

Thanks

Comment 36

4 years ago
Can the fixes for this problem be uplifted to aurora, please?
Flags: needinfo?(rclickenbrock)

Updated

4 years ago
status-firefox21: --- → unaffected
status-firefox22: --- → affected
status-firefox23: --- → fixed
tracking-firefox22: --- → ?
Whoa there. Bug 859718 is a regression caused by this bug. (Which I'm about to upload a patch for.) This should _not_ be uplifted without that fix being included.
Depends on: 859718

Updated

4 years ago
tracking-firefox22: ? → +
Keywords: regression
Whiteboard: [uplift with 859718]
Bug 859718 has hit m-c as of yesterday. If I don't hear of any regressions by tonight I'm going to nominate both this bug and bug 859718 for uplift to Aurora.
(Assignee)

Comment 39

4 years ago
I don't have anything to add beyond what Seth has already said. Uplifting is blocked on Bug 859718.
Flags: needinfo?(rclickenbrock)
Comment on attachment 733098 [details] [diff] [review]
Part 1: Account for more decoder state in RasterImage::IsDecodeFinished()

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 716140
User impact if declined: Continuous high CPU usage in some situations.
Testing completed (on m-c, etc.): On m-c since 2013-04-04. One regression identified and fixed in bug 859718, which is also approval-mozilla-aurora? and should be uplifted with this patch.
Risk to taking this patch (and alternatives if risky): Low risk. This modifies how image decoders behave, but the changes are not complex and have been tested for two weeks now.
String or IDL/UUID changes made by this patch: None.
Attachment #733098 - Flags: approval-mozilla-aurora?
Comment on attachment 733099 [details] [diff] [review]
Part 2: Don't try to enqueue more decoding from DecodeDoneWorker

[Approval Request Comment]
(Same as above.)
Attachment #733099 - Flags: approval-mozilla-aurora?
Comment on attachment 733098 [details] [diff] [review]
Part 1: Account for more decoder state in RasterImage::IsDecodeFinished()

Please make sure to uplift patch in Bug 859718 when uplifting this.Thank you !
Attachment #733098 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 733099 [details] [diff] [review]
Part 2: Don't try to enqueue more decoding from DecodeDoneWorker

Well tested on nightly, regression from Bug 716140 - Multithreaded image decoding which is in Fx 22. Avoids use of high CPU utilization which was reported to happen in a few scenario's .
Attachment #733099 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/d8135447bfec
https://hg.mozilla.org/releases/mozilla-aurora/rev/50055800e40c
status-firefox22: affected → fixed

Updated

4 years ago
Blocks: 856670
I confirm the fix is verified on FF 22.b1 on windows 7 x64:

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0(20130514181517)
status-firefox22: fixed → verified
I confirm the fix is verified on FF 23b4 too on windows 7 x64:

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0(20130708202947)
Status: RESOLVED → VERIFIED
status-firefox23: fixed → verified
You need to log in before you can comment on or make changes to this bug.