Closed
Bug 1137019
Opened 9 years ago
Closed 9 years ago
Make the imgRequest methods that forward to the image threadsafe
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: seth, Assigned: seth)
References
Details
Attachments
(3 files)
1.73 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
5.24 KB,
patch
|
baku
:
review+
tnikkel
:
feedback+
|
Details | Diff | Splinter Review |
16.84 KB,
patch
|
Details | Diff | Splinter Review |
Right now imgRequest has two sets of methods that forward to the image: LockImage/UnlockImage and RequestDecode/StartDecoding. These methods interact with imgRequest fields that are mutated off-main-thread in imgRequest::OnDataAvailable, but they are called from the main thread. We need to make these threadsafe.
Assignee | ||
Comment 1•9 years ago
|
||
LockImage and UnlockImage are easy, because nothing ever calls them. We can just remove them.
Attachment #8569556 -
Flags: review?(amarchesini)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → seth
Status: NEW → ASSIGNED
Comment 2•9 years ago
|
||
Comment on attachment 8569556 [details] [diff] [review] (Part 1) - Get rid of unused LockImage forwarding methods on imgRequest Review of attachment 8569556 [details] [diff] [review]: ----------------------------------------------------------------- This seems an easy review :)
Attachment #8569556 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 3•9 years ago
|
||
RequestDecode and StartDecoding aren't quite as simple. We *could* get rid of them, and just handle this in imgRequestProxy::SetHasImage. But I don't think we should do that, because SetHasImage runs on the main thread, while we create images off-main-thread. That means that relying on imgRequestProxy::SetHasImage will introduce additional latency before we start decoding, and we really want to avoid that because it affects page load performance. Instead, let's reduce RequestDecode and StartDecoding to the bare minimum: a single method that updates an atomic. This ensures they are trivially threadsafe. Both of these methods used to try the image first if it exists, but we don't need to do that in imgRequest; that code can live just as easily in imgRequestProxy's corresponding methods. We still need to make imgRequest::GetProgressTracker threadsafe (which will implicitly make imgRequestProxy::GetImage() threadsafe), but we'll do that in another bug. It's worth considering an alternative design: if we're going to introduce a lock for imgRequest::GetProgressTracker, why don't we use the same lock for imgRequest::mDecodeRequested? The answer is that we'd then before forced to either hold that lock while calling RasterImage::StartDecoding, which is potentially a very expensive call, or we'd be forced to release the lock and take it again, which would be more expensive than just taking a lock and updating an atomic.
Attachment #8569563 -
Flags: review?(amarchesini)
Attachment #8569563 -
Flags: feedback?(tnikkel)
Comment 4•9 years ago
|
||
Comment on attachment 8569563 [details] [diff] [review] (Part 2) - Replace imgRequest's image decoding methods with a single minimal method that updates an atomic Review of attachment 8569563 [details] [diff] [review]: ----------------------------------------------------------------- I'm not so familiar with this code. I see that basically it doesn't change the current logic too much. The only main difference is that if GetOwner() returns nullptr, now we don't return an error. But it seems fine because we don't check the error code anywhere :)
Attachment #8569563 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 5•9 years ago
|
||
Thanks for the review! (In reply to Andrea Marchesini (:baku) from comment #4) > The only main difference is that if GetOwner() returns nullptr, now we don't > return an error. > But it seems fine because we don't check the error code anywhere :) Typical ImageLib error handling. =)
Updated•9 years ago
|
Attachment #8569563 -
Flags: feedback?(tnikkel) → feedback+
Assignee | ||
Comment 6•9 years ago
|
||
Thanks for the feedback! Looks like this is good to go if try looks good. https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b5f234d60cc
Assignee | ||
Comment 7•9 years ago
|
||
Try looks good. Pushed: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d834e57b0352 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/febbccd1cd1e
Comment 8•9 years ago
|
||
Something in this push made bug 1123563 permafail. Backed out. https://hg.mozilla.org/integration/mozilla-inbound/rev/2fa9d9762eec
Assignee | ||
Comment 9•9 years ago
|
||
OK, that was definitely unexpected. (And it didn't get picked up by the try job because that job didn't include Android.) I've pushed a new try job to investigate: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b14bc822e877
Assignee | ||
Comment 10•9 years ago
|
||
I'm concerned that that try job isn't starting the Android jobs. Pushed a new one here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3afb5ea78cd3
Assignee | ||
Comment 11•9 years ago
|
||
So apparently my problem is that the trychooser Mercurial extension was using the wrong platform names. Fortunately someone fixed it recently. Hopefully *this* push will actually work: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1128bd7eef76
Assignee | ||
Comment 12•9 years ago
|
||
Adding a new part 0 here that backs out bug 1142849, which is necessary for this series of patches to land.
Assignee | ||
Comment 13•9 years ago
|
||
Pushed to inbound: 97aacb3e7b80 Seth Fowler — Bug 1137019 (Part 2) - Replace imgRequest's image decoding methods with a single minimal method that updates an atomic. r=baku 3143c0a3e1a5 Seth Fowler — Bug 1137019 (Part 1) - Get rid of unused LockImage forwarding methods on imgRequest. r=baku fde190586b1b Seth Fowler — Bug 1137019 (Part 0) - Back out bug 1142849.
Comment 14•9 years ago
|
||
(In reply to Seth Fowler [:seth] from comment #13) > Pushed to inbound: > > 97aacb3e7b80 Seth Fowler — Bug 1137019 (Part 2) - Replace imgRequest's image > decoding methods with a single minimal method that updates an atomic. r=baku > 3143c0a3e1a5 Seth Fowler — Bug 1137019 (Part 1) - Get rid of unused > LockImage forwarding methods on imgRequest. r=baku > fde190586b1b Seth Fowler — Bug 1137019 (Part 0) - Back out bug 1142849. Seth, are these messages automatically generated? They don't include the a link to hg.mozilla.org, which is unfortunate because it makes it hard to see the exact changesets that landed, thus breaking the standard paper trail.
Flags: needinfo?(seth)
Comment 15•9 years ago
|
||
The numbers are a revision, so you can just hack up an hg URL yourself. Like for part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/97aacb3e7b80
Comment 16•9 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #15) > The numbers are a revision, so you can just hack up an hg URL yourself. Like > for part 2: > https://hg.mozilla.org/integration/mozilla-inbound/rev/97aacb3e7b80 Sure. But everybody else just posts an actual link -- Mercurial even gives them to you when you push -- so having to do that is a pain. Also, the order of Seth's lines is the opposite to the standard order.
Comment 17•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fde190586b1b https://hg.mozilla.org/mozilla-central/rev/3143c0a3e1a5 https://hg.mozilla.org/mozilla-central/rev/97aacb3e7b80
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Assignee | ||
Comment 18•9 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #16) > Sure. But everybody else just posts an actual link -- Mercurial even gives > them to you when you push -- so having to do that is a pain. Also, the order > of Seth's lines is the opposite to the standard order. The fault lies with the hook which gives you those revision links, which switches to giving you only a pushlog when you push more than a certain number of commits. The threshold is way too low currently. I've filed bug 1147087 about fixing this.
Flags: needinfo?(seth)
You need to log in
before you can comment on or make changes to this bug.
Description
•