Closed Bug 1300543 Opened 3 years ago Closed 3 years ago

Icon task pipeline optimization

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: sebastian, Assigned: sebastian)

References

Details

Attachments

(8 files)

58 bytes, text/x-review-board-request
ahunt
: review+
Details
58 bytes, text/x-review-board-request
ahunt
: review+
Details
58 bytes, text/x-review-board-request
ahunt
: review+
Details
58 bytes, text/x-review-board-request
ahunt
: review+
Details
58 bytes, text/x-review-board-request
ahunt
: review+
Details
58 bytes, text/x-review-board-request
ahunt
: review+
Details
58 bytes, text/x-review-board-request
ahunt
: review+
Details
58 bytes, text/x-review-board-request
ahunt
: review+
Details
According to bug 1300490 the new icon task pipeline introduced a performance regression. There are multiple ways how the current pipeline could be optimized. Let's look at some traces and remove the biggest bottlenecks.
Depends on: 1300569
The alert was:
>  9%  local-nytimes summary android-5-1-armv7-api15 opt      2335.19 -> 2535.65
>  6%  remote-nytimes summary android-5-1-armv7-api15 opt     2502.07 -> 2658.81
Comment on attachment 8788217 [details]
Bug 1300543 - IconRequestExecutor: Add custom thread pool executor and thread factory.

https://reviewboard.mozilla.org/r/76776/#review74886
Attachment #8788217 - Flags: review?(ahunt) → review+
Comment on attachment 8788218 [details]
Bug 1300543 - IconRequestExecutor: Resize image before extracting color.

https://reviewboard.mozilla.org/r/76778/#review74888
Attachment #8788218 - Flags: review?(ahunt) → review+
Comment on attachment 8788219 [details]
Bug 1300543 - LegacyLoader: Skip loading from legacy storage if network download is permitted.

https://reviewboard.mozilla.org/r/76780/#review74890
Attachment #8788219 - Flags: review?(ahunt) → review+
Comment on attachment 8788220 [details]
Bug 1300543 - LegacyLoader: Only load if there's one icon URL left.

https://reviewboard.mozilla.org/r/76782/#review74892
Attachment #8788220 - Flags: review?(ahunt) → review+
Comment on attachment 8788221 [details]
Bug 1300543 - FilterMimeTypes: Continue to filter mime types if one of them is empty.

https://reviewboard.mozilla.org/r/76784/#review74894
Attachment #8788221 - Flags: review?(ahunt) → review+
Comment on attachment 8788433 [details]
Bug 1300543 - IconDownloader.downloadAndDecodeImage(): Correctly assign and close stream.

https://reviewboard.mozilla.org/r/76942/#review75094
Attachment #8788433 - Flags: review?(ahunt) → review+
Comment on attachment 8788434 [details]
Bug 1300543 - IconDownloader: Use final keyword where appropriate.

https://reviewboard.mozilla.org/r/76944/#review75096
Attachment #8788434 - Flags: review?(ahunt) → review+
Comment on attachment 8788362 [details]
Bug 1300543 - Use palette library instead of BitmapUtils.getDominantColor().

https://reviewboard.mozilla.org/r/76870/#review75106

This seems to have some nice advantages over our own library, although I'm slightly worried about losing the ability to customise colour extraction: I was experimenting with a few things in Bug 1299524 (I'll try to upload my experimental code ASAP):

- Prioritising colours over black/white: is handled well in Palette (it doesn't seemt to return black/white ever)
-- Except for the "nzz.ch" favicon: I still get white, even though most of the icon is black. (My getDominantColor() customisations return a black background here)
-- And amazon.com now receives a yellow background, even though there's very little yellow: using white as the background colour works better (in my experimental patch I weight colours vs black/white by a factor of 3, which results in white for the amazon icon).

Overall it seems better to not have to maintain our own colour extraction, and this is definitely better than what's currently in-tree, but I wonder if it's worth the additional cost *if* we can manage to get consistenly better icons using our own colour extraction?
Attachment #8788362 - Flags: review?(ahunt) → review+
(In reply to Andrzej Hunt :ahunt from comment #29)
> Overall it seems better to not have to maintain our own colour extraction,
> and this is definitely better than what's currently in-tree, but I wonder if
> it's worth the additional cost *if* we can manage to get consistenly better
> icons using our own colour extraction?

That's a good point. A solution that could co-exist is the tippy-top collection that desktop has been using (bug 1263707): A catalog of good icons and matching colors for popular pages. We could implement this as downloadable content (on demand updates) and this should be easy to add to the new icon pipeline.
Pushed by s.kaspari@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/dd2d29dbd7c5
IconRequestExecutor: Add custom thread pool executor and thread factory. r=ahunt
https://hg.mozilla.org/integration/autoland/rev/2269f770b2e7
IconRequestExecutor: Resize image before extracting color. r=ahunt
https://hg.mozilla.org/integration/autoland/rev/c5f01980a16a
LegacyLoader: Skip loading from legacy storage if network download is permitted. r=ahunt
https://hg.mozilla.org/integration/autoland/rev/ceb8ca8e9b2d
LegacyLoader: Only load if there's one icon URL left. r=ahunt
https://hg.mozilla.org/integration/autoland/rev/714566fb31ec
FilterMimeTypes: Continue to filter mime types if one of them is empty. r=ahunt
https://hg.mozilla.org/integration/autoland/rev/4e9bf0dca65a
Use palette library instead of BitmapUtils.getDominantColor(). r=ahunt
https://hg.mozilla.org/integration/autoland/rev/d5356db094fd
IconDownloader.downloadAndDecodeImage(): Correctly assign and close stream. r=ahunt
https://hg.mozilla.org/integration/autoland/rev/38cb109c653c
IconDownloader: Use final keyword where appropriate. r=ahunt
You need to log in before you can comment on or make changes to this bug.