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

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Benoit Girard (:BenWa)]

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.

Comment 1

[Joe Drew (not getting mail)]

IIRC Jeff was working on some other implementation of Timestamp?

Comment 2

[Joe Drew (not getting mail)]

Jeff suggests we use PR_Now() instead of Timestamp, which is fast but unfortunately only has 15ms resolution on Windows.

Comment 3

[Brian R. Bondy [:bbondy]]

How about QueryPerformanceCounter?
Here's a sample usage:
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=710935&attachment=601970

Comment 4

[Brian R. Bondy [:bbondy]]

I can do that and time the difference if you want, but it'd be a windows only fix.

Comment 5

[Robert Lickenbrock [:rclick]]

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.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

(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.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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.

Comment 8

[Joe Drew (not getting mail)]

Yikes. I bet that crept in with my endless requests to simplify that loop.

Comment 9

[Benoit Girard (:BenWa)]

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.

Comment 10

[Robert Lickenbrock [:rclick]]

Attachment: 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.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 654767 [details] [diff] [review]
patch

It feels like we could use some more precise names than DecodeSomeOf and DecodeABitOf

Comment 12

[Joe Drew (not getting mail)]

Comment on attachment 654767 [details] [diff] [review]
patch

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

Hoorah! Great.

Comment 13

[Robert Lickenbrock [:rclick]]

Attachment: patch

Comment 14

[Joe Drew (not getting mail)]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

This is already running on Try.
https://tbpl.mozilla.org/?tree=Try&rev=d0696c39e893

Comment 16

[Justin Lebar (not reading bugmail)]

Windows and Linux-32 mochitests were still pending, but the rest looked good.

 https://hg.mozilla.org/integration/mozilla-inbound/rev/3331ec03e60f

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Pro-tip: Remove checkin-needed when landing ;)

Comment 18

[Alex Keybl [:akeybl]]

(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.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3331ec03e60f