Closed Bug 1119158 Opened 6 years ago Closed 6 years ago

Retarget OnDataAvailable for images to a special I/O thread instead of the image decoding thread pool

Categories

(Core :: ImageLib, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(1 file, 3 obsolete files)

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!
Attachment #8545810 - Flags: review?(sworkman)
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 :)
Attachment #8545810 - Flags: review?(sworkman) → review+
(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!
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.
Attachment #8545810 - Attachment is obsolete: true
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.
Attachment #8547942 - Attachment is obsolete: true
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
Attachment #8550768 - Attachment is obsolete: true
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
https://hg.mozilla.org/mozilla-central/rev/8670d0f5ea26
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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.
Attachment #8550844 - Flags: approval-mozilla-aurora?
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.
Attachment #8550844 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.