Closed Bug 1180931 Opened 4 years ago Closed 4 years ago
Decode image headers on the main thread on single core devices
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.
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.
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 #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
Rebased part 2.
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
You need to log in before you can comment on or make changes to this bug.