1300490 - 6.26 - 8.58% android-5-1 local/remote-nytimes regression September 2 from autoland push 38d0defa2db7

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: Favicon Handling, defect, P2)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

Perfherder has found a regression from the Autophone framework:

== Change summary for alert #2947 (as of September 02 2016 02:51 UTC) ==

Regressions:

  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

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=2947

after some retriggers this has been determined to be caused by bug 1290014:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=347ed1510d82b1963573344a70e40f0903309b02&tochange=38d0defa2db77fa8bf209cf60bde4232896660cc

There are 10 patches on the bug, it is hard to tell without bisecting the patches which specific patch is the root cause.

I believe the proper try syntax to get this test to run on try server is:
try: -b o -p android-api-15 -u autophone-s1s2 -t none

:sebastian, can you take a look at this and help us determine the root cause.  We merge to Aurora in 1 week, so it would be nice to have this understood or fixed prior to the merge.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

I'll have a look. Thanks for filing! It looks like the refactoring of the favicon code (bug 1290014) introduced a performance bottle neck.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

In bug 1300543 I landed some optimizations for the new icon code. However looking at the latest results on treeherder this doesn't seem to have helped much. I'll look more closely at the nytimes test and compare the results of the old and new code locally.

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

thanks for working on this.  Please don't hesitate to ask for help as needed!

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: performance-analysis.png

I did some local analysis with the nytimes files, but unfortunately I couldn't really measure an obvious difference between old and new code (see attachment). Results seem to be roughly in the same range for both implementation for me.

I'll be on PTO next week and the beginning of the week after that. But I'll continue to look at this once I'm back.

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Comparing some results on try:

State before the code landed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbaab1612a24
* remote-nytimes: 2715.17
* local-nytimes: 2659.91

With new code and optimizations landed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=89df401e44e7
* remote-nytimes: 2632.71
* local-nytimes: 2521.96

With icon loading disabled:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=97b4e6cdd578
* remote-nytimes: 2721.97
* local-nytimes: 2491.45

remote-nytimes doesn't really change much. local-nytimes looks like it could be affected by the changes.

The difference from the icon point of view is that we do not load an icon from the local file system (file://) and instead generate a fallback icon. This did not happen with the previous code that would just fail and return a static default icon. Maybe this is something to look at.

Something to look at too: We start to load the icon after the website has finished loading. In theory this shouldn't affect the measurements here.

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

thanks Sebastian for looking into this- when we do figure out something, we should consider uplifting to Aurora as well since this regressions is merging to Aurora this week.

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

:sebastian, do you have any further progress on this?

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Joel Maher ( :jmaher) from comment #7)
> :sebastian, do you have any further progress on this?

Not yet, first I was a week on PTO, then this week on a work week in Toronto. But starting next week I'll look into this again!

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

How much do we care about this regression?  Something we will fix for sure?

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Milan Sreckovic [:milan] from comment #9)
> How much do we care about this regression?  Something we will fix for sure?

I just updated bug 1309510. Looking more into this I couldn't measure any performance difference caused by the icon code anymore (see other bug). In addition to that we landed other performance improvements and are now much faster than before - even before regression alert (See graph).

So yeah, I think this can be closed because there's no measurable improvement to make. Please re-open if you object.