Closed Bug 590260 Opened 9 years ago Closed 9 years ago

Decode more bytes at a time during image redecodes

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta5+

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
> since 20ms is the amount of time a user will notice.

200ms is the number I have usually seen cited for that threshold...
(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?
> 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.  ;)
Added a patch - this dramatically speeds things up, because we don't spend so much time cycling through the event loop.
Attachment #469664 - Flags: review?(joe)
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.
Attachment #469664 - Flags: review?(joe) → review-
Added an updated patch. Flagging joe for review.
Attachment #469664 - Attachment is obsolete: true
Attachment #469900 - Flags: review?(joe)
Attachment #469900 - Flags: review?(joe) → review+
blocking2.0: --- → beta5+
pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/b6e7aea76bc9

resolving fixed.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Duplicate of this bug: 591354
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 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.
Attachment #546530 - Flags: review?(joe) → review+
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.
You need to log in before you can comment on or make changes to this bug.