1180931 - Decode image headers on the main thread on single core devices

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

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]."

Comment 1

[Seth Fowler [:seth] [:s2h]]

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.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: Use sync loading for multipart images and single core devices

Here's the patch.

Comment 3

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d35c1e7891c

Comment 4

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Allow sync size decoding for single core devices

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.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Allow sync size decoding for transient (i.e. multipart) images

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.

Comment 6

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ebb29f82e79

Comment 7

[Bob Clary [:bc] (inactive)]

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 8

[Timothy Nikkel (:tnikkel)]

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.

Comment 9

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Allow sync size decoding for single core devices

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.

Comment 10

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Allow sync size decoding for transient (i.e. multipart) images

Rebased part 2.

Comment 11

[Timothy Nikkel (:tnikkel)]

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.

Comment 12

[Seth Fowler [:seth] [:s2h]]

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.

Comment 13

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Allow sync size decoding for single core devices

Updated the patch per the review comments.

Comment 14

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6454f52da8b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a8c177d73bd

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6454f52da8b3
https://hg.mozilla.org/mozilla-central/rev/7a8c177d73bd