Closed Bug 1180931 Opened 4 years ago Closed 4 years ago

Decode image headers on the main thread on single core devices

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(2 files, 4 obsolete files)

We saw a regression on single core devices after landing bug 1163856. Bug 1163856 makes us always decode image headers off-main-thread, but on single core devices we don't gain any performance from parallelism. Instead, we lose performance because decoding the image headers asynchronously means we wait longer for them, delaying page load.

As outlined in bug 1120511 comment 32 and below, let's use Image::INIT_FLAG_SYNC_LOAD on single core devices, so that image headers are decoded on the main thread. This eliminates most of the regression we saw due to bug 1163856 on single core devices. (And possibly all of it, since there's some noise to these measurements.) As stated in bug 1120511 comment 38:

"Based on those results (6646ms vs 6769ms before), it appears that the INIT_FLAG_SYNC_LOAD approach gives us a ~1.8% improvement. That's 123ms, which is most of the 170ms regression discussed in [bug 1120511 comment 32]."
I also think we should use this approach for multipart images, so we'll start using INIT_FLAG_SYNC_LOAD for them as well in this patch.
Component: General → ImageLib
Product: Firefox for Android → Core
Here's the patch.
Attachment #8630179 - Flags: review?(tnikkel)
Attachment #8630179 - Attachment is obsolete: true
Attachment #8630179 - Flags: review?(tnikkel)
Per IRC discussion: rather than use INIT_FLAG_SYNC_DECODE, which forces us to
always do a synchronous size decode when images finish loading, with this patch
we still launch the async size decoder but we'll do a synchronous size decode if
we haven't gotten a result from the async size decoder by the time
OnImageDataComplete is called.

The advantage is that we get the size earlier for large images, which can in
turn let layout reach a stable state quicker. The disadvantage is that we'll do
more work if do we decide to perform the sync size decode, since the async size
decode will still run (in fact, it may well already have run - we may just have
not gotten the event yet).

Which approach is better depends on the workload, but after talking with Timothy
on IRC I think this is the safer choice.
Attachment #8630235 - Flags: review?(tnikkel)
In part 2 we make the same change in part 1, but for transient images - which
means, for frames of multipart images. I think this is another case where doing
a sync size decode is worthwhile.
Attachment #8630238 - Flags: review?(tnikkel)
To see your results remember to click the show/hide excluded jobs toggle:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ebb29f82e79&exclusion_profile=false&filter-searchStr=autophone

The issue with the unit tests has been fixed, but if you don't need to run all of the tests please don't use autophone-tests. It will cause the devices to attempt to run all of the smoketests, s1s2 tests, webappstartup tests plus all of the reftest, jsreftests, crashtests and mochitests and cause the devices to work on your try build for several days to the exclusion of other builds.

This should work for you:
try: -b o -p android-api-9,android-api-11 -u autophone-s1s2,autophone-webapp -t none
Comment on attachment 8630235 [details] [diff] [review]
(Part 1) - Allow sync size decoding for single core devices

DecodePool::DecodePool also calls PR_GetNumberOfProcessors and it uses a different algorithm to determine the number of threads than just strictly the number of processors. So DecodePool::ThreadCount is badly named.
Attachment #8630235 - Flags: review?(tnikkel)
Attachment #8630238 - Flags: review?(tnikkel) → review+
I agree, the terminology choice in the previous version of part 1 was quite
poor. I've renamed that method to NumberOfCores() and added more information in
the associated comment in this version of the patch.
Attachment #8630806 - Flags: review?(tnikkel)
Attachment #8630235 - Attachment is obsolete: true
Attachment #8630238 - Attachment is obsolete: true
Comment on attachment 8630806 [details] [diff] [review]
(Part 1) - Allow sync size decoding for single core devices

Can you just use NumberOfCores instead of PR_GetNumberOfProcessors in DecodePool::DecodePool? May as well use it.
Attachment #8630806 - Flags: review?(tnikkel) → review+
Thanks for the review, Timothy!

(In reply to Bob Clary [:bc:] from comment #7)
> This should work for you:
> try: -b o -p android-api-9,android-api-11 -u autophone-s1s2,autophone-webapp
> -t none

Thanks Bob. Still figuring out the best way to use Autophone.
Updated the patch per the review comments.
Attachment #8630806 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/6454f52da8b3
https://hg.mozilla.org/mozilla-central/rev/7a8c177d73bd
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.