Last Comment Bug 784756 - DecodeABit can spend more time in Timestamp than image decoding on Win7
: DecodeABit can spend more time in Timestamp than image decoding on Win7
Status: RESOLVED FIXED
[sps][qa-]
: perf
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal with 2 votes (vote)
: mozilla17
Assigned To: Robert Lickenbrock [:rclick]
:
Mentors:
Depends on: 685516
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-22 12:24 PDT by Benoit Girard (:BenWa)
Modified: 2012-10-16 16:03 PDT (History)
17 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
patch (1.58 KB, patch)
2012-08-23 13:28 PDT, Robert Lickenbrock [:rclick]
joe: review+
Details | Diff | Review
patch (1.59 KB, patch)
2012-08-23 17:12 PDT, Robert Lickenbrock [:rclick]
rclickenbrock: review+
Details | Diff | Review

Description Benoit Girard (:BenWa) 2012-08-22 12:24:45 PDT
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) 2012-08-22 12:37:46 PDT
IIRC Jeff was working on some other implementation of Timestamp?
Comment 2 Joe Drew (not getting mail) 2012-08-22 12:54:34 PDT
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] 2012-08-22 13:03:02 PDT
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] 2012-08-22 13:09:31 PDT
I can do that and time the difference if you want, but it'd be a windows only fix.
Comment 5 Robert Lickenbrock [:rclick] 2012-08-22 14:29:41 PDT
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] 2012-08-22 14:39:49 PDT
(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 [:khuey] (khuey@mozilla.com) 2012-08-22 14:48:20 PDT
(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) 2012-08-22 18:20:04 PDT
Yikes. I bet that crept in with my endless requests to simplify that loop.
Comment 9 Benoit Girard (:BenWa) 2012-08-23 11:21:15 PDT
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] 2012-08-23 13:28:00 PDT
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.
Comment 11 Jeff Muizelaar [:jrmuizel] 2012-08-23 13:38:16 PDT
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) 2012-08-23 14:05:54 PDT
Comment on attachment 654767 [details] [diff] [review]
patch

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

Hoorah! Great.
Comment 13 Robert Lickenbrock [:rclick] 2012-08-23 17:12:27 PDT
Created attachment 654864 [details] [diff] [review]
patch
Comment 14 Joe Drew (not getting mail) 2012-08-23 19:55:02 PDT
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] 2012-08-23 20:02:15 PDT
This is already running on Try.
https://tbpl.mozilla.org/?tree=Try&rev=d0696c39e893
Comment 16 Justin Lebar (not reading bugmail) 2012-08-24 11:26:45 PDT
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] 2012-08-24 13:15:57 PDT
Pro-tip: Remove checkin-needed when landing ;)
Comment 18 Alex Keybl [:akeybl] 2012-08-24 16:32:26 PDT
(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] 2012-08-24 20:01:17 PDT
https://hg.mozilla.org/mozilla-central/rev/3331ec03e60f

Note You need to log in before you can comment on or make changes to this bug.