Make the imgRequest methods that forward to the image threadsafe

RESOLVED FIXED in Firefox 39

Status

()

Core
ImageLib
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

unspecified
mozilla39
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

3 years ago
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

3 years ago
Created attachment 8569556 [details] [diff] [review]
(Part 1) - Get rid of unused LockImage forwarding methods on imgRequest

LockImage and UnlockImage are easy, because nothing ever calls them. We can just remove them.
Attachment #8569556 - Flags: review?(amarchesini)
(Assignee)

Updated

3 years ago
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+
(Assignee)

Comment 3

3 years ago
Created attachment 8569563 [details] [diff] [review]
(Part 2) - Replace imgRequest's image decoding methods with a single minimal method that updates an atomic

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+
(Assignee)

Comment 5

3 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. =)
Attachment #8569563 - Flags: feedback?(tnikkel) → feedback+
(Assignee)

Comment 6

3 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 9

3 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

3 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

3 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

3 years ago
Created attachment 8578985 [details] [diff] [review]
(Part 0) - Back out bug 1142849

Adding a new part 0 here that backs out bug 1142849, which is necessary for this
series of patches to land.
(Assignee)

Comment 13

3 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.
(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.
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
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(Assignee)

Comment 18

3 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)
Depends on: 1150127
(Assignee)

Updated

3 years ago
No longer depends on: 1150127
You need to log in before you can comment on or make changes to this bug.