The default bug view has changed. See this FAQ.

DecodeABit can spend more time in Timestamp than image decoding on Win7

RESOLVED FIXED in Firefox 17

Status

()

Core
ImageLib
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: BenWa, Assigned: rclick)

Tracking

({perf})

unspecified
mozilla17
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox17+ fixed)

Details

(Whiteboard: [sps][qa-])

Attachments

(1 attachment, 1 obsolete attachment)

1.59 KB, patch
rclick
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
Looking at the profile here reported by a user:
http://people.mozilla.com/~bgirard/cleopatra/?report=542efa04d60977a067a3623a00765d837cb952ba

This will be made better once we add a cap in the time we can spend decoding image. However even with that fix DecodeABit shouldn't spend most of it time in Timestamp but rather doing useful decode.

We can:
1) Bump up the decode size.
2) Make timestamp faster on windows
3) Using something like timestamp that has weaker properties and is faster.
IIRC Jeff was working on some other implementation of Timestamp?
Jeff suggests we use PR_Now() instead of Timestamp, which is fast but unfortunately only has 15ms resolution on Windows.
How about QueryPerformanceCounter?
Here's a sample usage:
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=710935&attachment=601970
I can do that and time the difference if you want, but it'd be a windows only fix.
(Assignee)

Comment 5

5 years ago
I looks like the loop logic here is wrong: https://hg.mozilla.org/mozilla-central/file/ee1fb253dfce/image/src/RasterImage.cpp#l2902

We spin a loop for image.mem.max_ms_before_yield milliseconds, but we won't bail out of the loop if decoding finishes. That means that if decoding finishes we'll spend the rest of the timeslice hammering DecodeSomeOfImage (which returns early as there's no decoding to do) and the timestamp goop.

I have a patch for this locally that I'll post after it finishes building.
(Assignee)

Updated

5 years ago
Assignee: nobody → rclickenbrock
Status: NEW → ASSIGNED
(In reply to Brian R. Bondy [:bbondy] from comment #3)
> How about QueryPerformanceCounter?
> Here's a sample usage:
> https://bugzilla.mozilla.org/page.cgi?id=splinter.
> html&bug=710935&attachment=601970

QueryPerformanceCounter is broken on Windows. See the implementation of TimeStamp_windows.cpp and bug 676349 for all of the sadness.
(In reply to Robert Lickenbrock [:rclick] from comment #5)
> I looks like the loop logic here is wrong:
> https://hg.mozilla.org/mozilla-central/file/ee1fb253dfce/image/src/
> RasterImage.cpp#l2902
> 
> We spin a loop for image.mem.max_ms_before_yield milliseconds, but we won't
> bail out of the loop if decoding finishes. That means that if decoding
> finishes we'll spend the rest of the timeslice hammering DecodeSomeOfImage
> (which returns early as there's no decoding to do) and the timestamp goop.
> 
> I have a patch for this locally that I'll post after it finishes building.

Aha, excellent catch.
(Reporter)

Updated

5 years ago
Depends on: 685516
Yikes. I bet that crept in with my endless requests to simplify that loop.

Updated

5 years ago
Summary: DecodeABit can spend more time in Timestamp then image decoding on Win7 → DecodeABit can spend more time in Timestamp than image decoding on Win7
(Reporter)

Comment 9

5 years ago
We're merging next week I believe. Ideally we should push for this to get fix before aurora but this is a hard blocker to ship.
tracking-firefox17: --- → ?
(Assignee)

Comment 10

5 years ago
Created attachment 654767 [details] [diff] [review]
patch

It turns out that calling DecodeSomeOfImage in a loop isn't necessary; it already checks if decoding is taking too long and bails. Thus, this patch removes the loop.

This appears to fix the slow tab switching regression on pages that only have small images, although there's still a regression on pages that have a lot of large images.
Attachment #654767 - Flags: review?(joe)
Comment on attachment 654767 [details] [diff] [review]
patch

It feels like we could use some more precise names than DecodeSomeOf and DecodeABitOf
Comment on attachment 654767 [details] [diff] [review]
patch

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

Hoorah! Great.
Attachment #654767 - Flags: review?(joe) → review+
(Assignee)

Comment 13

5 years ago
Created attachment 654864 [details] [diff] [review]
patch
Attachment #654767 - Attachment is obsolete: true
Attachment #654864 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Couldn't push it to mozilla-inbound because it was closed, so I pushed it to try just for giggles:  https://tbpl.mozilla.org/?tree=Try&rev=d1059e1e0848
This is already running on Try.
https://tbpl.mozilla.org/?tree=Try&rev=d0696c39e893
Windows and Linux-32 mochitests were still pending, but the rest looked good.

 https://hg.mozilla.org/integration/mozilla-inbound/rev/3331ec03e60f
Pro-tip: Remove checkin-needed when landing ;)
Flags: in-testsuite-
Keywords: checkin-needed
(In reply to Benoit Girard (:BenWa) from comment #9)
> We're merging next week I believe. Ideally we should push for this to get
> fix before aurora but this is a hard blocker to ship.

Merge is on Monday - if low risk, please be ready to land Monday either on Aurora 16 or Beta 16 depending on what side of the merge we're on.
tracking-firefox17: ? → +
https://hg.mozilla.org/mozilla-central/rev/3331ec03e60f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Updated

5 years ago
status-firefox17: --- → fixed
Whiteboard: [sps] → [sps][qa-]
You need to log in before you can comment on or make changes to this bug.