590260 - Decode more bytes at a time during image redecodes

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Bobby Holley (:bholley)]

On my machine we can be decoding something on the order of 64k in 5ms rather than 4k. I also think that we should bump the timeout to something more like 15ms, since 20ms is the amount of time a user will notice.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> since 20ms is the amount of time a user will notice.

200ms is the number I have usually seen cited for that threshold...

Comment 2

[Bobby Holley (:bholley)]

(In reply to comment #1)
> > since 20ms is the amount of time a user will notice.
> 
> 200ms is the number I have usually seen cited for that threshold...

Ah, that's good to hear. I just did a search and it appears that the number's somewhere between 50-200 milliseconds, which gives us a lot more wiggle-room.

If the event queue purely FIFO? If so, we should also consider the possibility if having 10 images decoding simultaneously. If each of them gives itself 50 milliseconds before yielding, the lower bound on response time would be 500 milliseconds. Or am I simplifying things too much?

Comment 3

[Boris Zbarsky [:bzbarsky]]

> If the event queue purely FIFO?

Yes.

> Or am I simplifying things too much?

I don't think you are, no.  That's a definite danger.  It's a danger with 5ms too; just need 100 images.  ;)

Comment 4

[Bobby Holley (:bholley)]

Attachment: patch v1 - Decode more at a time (200k, 400ms before yielding)

Added a patch - this dramatically speeds things up, because we don't spend so much time cycling through the event loop.

Comment 5

[Joe Drew (not getting mail)]

Comment on attachment 469664 [details] [diff] [review]
patch v1 - Decode more at a time (200k, 400ms before yielding)

Can we make these prefs? Even if they aren't live, that'd be better than hardcoded constants.

Comment 6

[Bobby Holley (:bholley)]

Attachment: v2 - Decode more at a time (200k, 400ms before yielding), and hook up prefs

Added an updated patch. Flagging joe for review.

Comment 7

[Bobby Holley (:bholley)]

pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/b6e7aea76bc9

resolving fixed.

Comment 9

[Justin Lebar (not reading bugmail)]

The UI team's goal is to respond to all user actions within 50ms.  Some actions take multiple trips through the event loop to complete.  This patch moves us much further away from that goal.

See bug 666352.

Comment 10

[Justin Lebar (not reading bugmail)]

Attachment: Backout , v1

Comment 11

[Joe Drew (not getting mail)]

Comment on attachment 546530 [details] [diff] [review]
Backout , v1

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

::: modules/libpref/src/init/all.js
@@ +3208,5 @@
>  // be better.
>  pref("image.mem.min_discard_timeout_ms", 10000);
>  
>  // Chunk size for calls to the image decoders
> +pref("image.mem.decode_bytes_at_a_time", 4092);

We could maybe change this to a more-sensible 4096 instead of just straight backing out the patch, but I'm not picky either way.

Comment 12

[Justin Lebar (not reading bugmail)]

It was in fact 4096 in the original patch.  That is indeed much more sensible!

I meant to attach this patch to bug 666352, but didn't notice that bzexport picked up on the last rather than the first bug number in the comment.  I'll move the patch over and whatnot.