After bug 1079627 lands, decoders will be able to read image source data at the same time as new source data is being written. This is an opportunity to improve our parallelism in ImageLib, but we aren't getting the full benefit yet because imgRequest retargets OnDataAvailable to the same thread pool the decoders are running on, making it possible for decoders to block OnDataAvailable delivery. Let's move OnDataAvailable delivery to a new I/O thread so it can run independently of the decoders.

This patch does 3 things: - It adds a new I/O thread, owned and managed by DecodePool. We pretty much follow the same pattern that we do for the thread pool. - It renames DecodePool::mThreadPoolMutex to just mMutex, to reflect the fact that it doesn't just protect the thread pool anymore. (Indeed, the name was left over from the time when DecodePool was part of RasterImage.) I also updated and simplified the comment that goes with it. The old one was out of date anyway; RasterImage::mDecodingMonitor doesn't even exist post-bug 1079627. - It adds a new method, DecodePool::GetIOEventTarget, to get the event target for the new I/O thread, and replaces the call to DecodePool::GetEventTarget in imgRequest with a call to GetIOEventTarget. That should be all that's needed!

Comment on attachment 8545810 [details] [diff] [review] Retarget OnDataAvailable to a new I/O thread instead of the image decoding thread pool Review of attachment 8545810 [details] [diff] [review]: ----------------------------------------------------------------- Nice patch. r=me assuming passes on try, of course :)

(In reply to Steve Workman [:sworkman] (please use needinfo) from comment #2) > Nice patch. r=me assuming passes on try, of course :) Of course. =) Thanks for the review!

This looks good to go. Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/a15929ba55cd

Backed out per bug 1118694 comment 3.

So I'm pretty sure that the failure in bug 1118694 comment 3 is actually caused by this patch, because I didn't register the new thread with the Nuwa process, which (I now know) is required on B2G. I've updated the patch to call NuwaMarkCurrentProcess on the new thread.

Here's a new try job, rebased on top of the earlier patches in my patch queue: https://tbpl.mozilla.org/?tree=Try&rev=bc884f6ce3fd

I missed an instance of RIDThreadPoolListener. Looks like we'll need another run. =\ https://tbpl.mozilla.org/?tree=Try&rev=70894e0f14af

I realized that the changes here let me perform a minor cleanup that in turn let me add a new assertion. I think that's worth doing, and it's worth checking that the new assertion doesn't fire.

Here's a try job to check that. I think (hope!) that checking linux and B2G should be adequate. https://tbpl.mozilla.org/?tree=Try&rev=d12be97ad100

OK, Linux looks good, but the B2G build is failing because of some small issues (redefinition of |nsresult rv| in one function and a missing 'return NS_OK;' in another). Those should be fixed now. I pushed another, B2G-only try job: https://tbpl.mozilla.org/?tree=Try&rev=93986df0e1df

That B2G try push looks OK to me, so I went ahead and pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/8670d0f5ea26

Comment on attachment 8550844 [details] [diff] [review] Retarget OnDataAvailable to a new I/O thread instead of the image decoding thread pool Approval Request Comment [Feature/regressing bug #]: Part of fix for bug 1125272 and bug 1119938. [User impact if declined]: Already approved. [Describe test coverage new/current, TreeHerder]: In 38 for a long time. [Risks and why]: Low risk; in 38 for a long time. [String/UUID change made/needed]: None.

Pushed to Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/6b2f603daef5

Comment on attachment 8550844 [details] [diff] [review] Retarget OnDataAvailable to a new I/O thread instead of the image decoding thread pool Aurora approval previously granted for a collection of ImageLib related bugs that Seth worked on and QE verified before uplift.