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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(3 files)

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.
LockImage and UnlockImage are easy, because nothing ever calls them. We can just remove them.
Attachment #8569556 - Flags: review?(amarchesini)
Assignee: nobody → seth
Status: NEW → ASSIGNED
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+
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 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+
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. =)
Attachment #8569563 - Flags: feedback?(tnikkel) → feedback+
Thanks for the feedback! Looks like this is good to go if try looks good.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b5f234d60cc
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
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
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
Adding a new part 0 here that backs out bug 1142849, which is necessary for this
series of patches to land.
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.
(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)
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
(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.
(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)
Depends on: 1150127
No longer depends on: 1150127
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: