Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Description • 8 years ago

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.

	Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Comment 1 • 8 years ago

Before: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbaab1612a24

	Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Comment 2 • 8 years ago

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

	Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Comment 3 • 8 years ago

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

	Andrzej Hunt :ahunt
	Comment 11 • 8 years ago
mozreview-review

Comment on attachment 8788217 [details]
Bug 1300543 - IconRequestExecutor: Add custom thread pool executor and thread factory.

https://reviewboard.mozilla.org/r/76776/#review74886

	Andrzej Hunt :ahunt
	Comment 12 • 8 years ago
mozreview-review

Comment on attachment 8788218 [details]
Bug 1300543 - IconRequestExecutor: Resize image before extracting color.

https://reviewboard.mozilla.org/r/76778/#review74888

	Andrzej Hunt :ahunt
	Comment 13 • 8 years ago
mozreview-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

	Andrzej Hunt :ahunt
	Comment 14 • 8 years ago
mozreview-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

	Andrzej Hunt :ahunt
	Comment 15 • 8 years ago
mozreview-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

	Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Comment 16 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=89df401e44e7

	Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Comment 23 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=489543568c33

	Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Comment 26 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=86e9e9051458

	Andrzej Hunt :ahunt
	Comment 27 • 8 years ago
mozreview-review

Comment on attachment 8788433 [details]
Bug 1300543 - IconDownloader.downloadAndDecodeImage(): Correctly assign and close stream.

https://reviewboard.mozilla.org/r/76942/#review75094

	Andrzej Hunt :ahunt
	Comment 28 • 8 years ago
mozreview-review

Comment on attachment 8788434 [details]
Bug 1300543 - IconDownloader: Use final keyword where appropriate.

https://reviewboard.mozilla.org/r/76944/#review75096

	Andrzej Hunt :ahunt
	Comment 29 • 8 years ago
mozreview-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?

	Sebastian Kaspari (:sebastian; :pocmo) Assignee
	Comment 30 • 8 years ago

(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.

	Pulsebot
	Comment 34 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 35 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/dd2d29dbd7c5
https://hg.mozilla.org/mozilla-central/rev/2269f770b2e7
https://hg.mozilla.org/mozilla-central/rev/c5f01980a16a
https://hg.mozilla.org/mozilla-central/rev/ceb8ca8e9b2d
https://hg.mozilla.org/mozilla-central/rev/714566fb31ec
https://hg.mozilla.org/mozilla-central/rev/4e9bf0dca65a
https://hg.mozilla.org/mozilla-central/rev/d5356db094fd
https://hg.mozilla.org/mozilla-central/rev/38cb109c653c