Closed Bug 784756 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: ImageLib, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17
Tracking Status
firefox17 + fixed

People

(Reporter: BenWa, Assigned: rclick)

References

Details

(Keywords: perf, Whiteboard: [sps][qa-])

Attachments

(1 file, 1 obsolete file)

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.
I can do that and time the difference if you want, but it'd be a windows only fix.
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: 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.
Depends on: 685516
Yikes. I bet that crept in with my endless requests to simplify that loop.
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
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.
Attached patch patch (obsolete) — Splinter Review
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+
Attached patch patchSplinter Review
Attachment #654767 - Attachment is obsolete: true
Attachment #654864 - Flags: review+
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
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.
https://hg.mozilla.org/mozilla-central/rev/3331ec03e60f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Whiteboard: [sps] → [sps][qa-]
You need to log in before you can comment on or make changes to this bug.