1219176 - Make it possible to dynamically preload external apps to be run with Raptor

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Oleg Zasypkin [:azasypkin]]

This is follow-up for the discussion happened in bug 1133711.

In the scope of this bug I'd like to try:

* Modify preloadapp-toolkit [1] to support "Last Modified" HTTP header in addition to "E-Tag" so that it work for the GitHub Pages (GH doesn't support ETag yet);
* Modify preload.py [2] and utils-xpc.js [3] to support predefined origin for the "external" apps.

[1] https://github.com/mozilla-b2g/preload-app-toolkit
[2] https://github.com/mozilla-b2g/gaia/blob/9908a086b2819e6a3c76de937e3958177b5933b7/tools/preload.py#L316
[3] https://github.com/mozilla-b2g/gaia/blob/9908a086b2819e6a3c76de937e3958177b5933b7/build/utils-xpc.js#L408

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] azasypkin:bug-1219176-raptor-load-apps > mozilla-b2g:master

Comment 2

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8683810 [details] [review]
[gaia] azasypkin:bug-1219176-raptor-load-apps > mozilla-b2g:master

Hey Eli,

How does it look?

I use dedicated domains for every test app as I didn't find an easy and nice way to make different apps hosted on the same domain work with Raptor. I've left few more comments at Github.

Any ideas are highly appreciated!

Thanks!

Comment 3

[Oleg Zasypkin [:azasypkin]]

Forgot to mention that tests for the outoftree apps I've included in this WIP patch run as follows:

* raptor test coldlaunch --app raptor-shared-worker-memory.github.io
* raptor test coldlaunch --app raptor-empty-service-worker.github.io

Comment 4

[:Eli Perelman]

Comment on attachment 8683810 [details] [review]
[gaia] azasypkin:bug-1219176-raptor-load-apps > mozilla-b2g:master

> I didn't find an easy and nice way to make different apps hosted on the same domain work with Raptor. I've left few more comments at Github.

I thought we discussed over IRC that as long as they had a different path, specified using --entry-point, you could work around that?

All in all though, the patch seems to be OK. It's nice that there is a place to set settings and prefs, and I wonder if more can be moved into there, like PERF_LOGGING [1] and only the required ones discussed elsewhere [2].

[1] this.userPrefs['dom.performance.enable_user_timing_logging'] = true;
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1219301

Comment 5

[Oleg Zasypkin [:azasypkin]]

(In reply to :Eli Perelman from comment #4)
> Comment on attachment 8683810 [details] [review]
> [gaia] azasypkin:bug-1219176-raptor-load-apps > mozilla-b2g:master
> 
> > I didn't find an easy and nice way to make different apps hosted 
> on the same domain work with Raptor. I've left few more comments at Github.
> 
> I thought we discussed over IRC that as long as they had a different path,
> specified using --entry-point, you could work around that?

The problem is the following: let's say we have two apps, domain.com/app1/manifest.webapp(1) and
domain.com/app2/manifest.webapp(2) and they are preloaded with apppreload toolkit.

(1) and (2) are used to identify apps on the homescreen - to run them with raptor we use "--app domain.com --entry-point app1" and "--app domain.com --entry-point app2".

But for Raptor we need that app has fixed origin that is mapped to one we passed to --app - and this is the problem, that means that both apps should have the same origin defined in manifest - and it's not allowed by FxOS (FxOS uses app origin as a folder name where to put app package, so one app will be just overridden by another).

Can Raptor live without this mapping and just extract origin from the first appLaunch perf mark after it tapped on the app icon?

> 
> All in all though, the patch seems to be OK. It's nice that there is a place
> to set settings and prefs, and I wonder if more can be moved into there,
> like PERF_LOGGING [1] and only the required ones discussed elsewhere [2].

settings.json can only contain _settings_ and not prefs, unfortunately "dom.performance.enable_user_timing_logging" pref as well as "dom.serviceWorkers.enabled" doesn't have corresponding setting (should be defined in http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/settings.js).

But honestly, I'd rather map some settings to these prefs. What do you think?

> 
> [1] this.userPrefs['dom.performance.enable_user_timing_logging'] = true;
> [2] https://bugzilla.mozilla.org/show_bug.cgi?id=1219301

Comment 6

[:Eli Perelman]

My mistake, I thought I saw a file for preferences as well as settings. If you want to generate settings for some of the prefs, I have no arguments against it.

Regarding the domains, I'm still not following the problem. Is this a problem with the toolkit, or Raptor, or the manifests being used?

Comment 7

[Oleg Zasypkin [:azasypkin]]

(In reply to :Eli Perelman from comment #6)
> My mistake, I thought I saw a file for preferences as well as settings. If
> you want to generate settings for some of the prefs, I have no arguments
> against it.

Ok, I'll prepare list of the pref we want/can move to the settings and try to push this forward.

> Regarding the domains, I'm still not following the problem. Is this a
> problem with the toolkit, or Raptor, or the manifests being used?

Well, it's a good question, and I'm not 100% sure. But let's me try to think aloud:

* Everything seems OK with the FxOS build system and manifest that both enforce apps to have different origins;

* The following thing in apppreload-toolkit + utils-xpc.js (build subsystem) + Homescreen bundle is not clear to me: when app is preloaded by apppreload toolkit metadata.json is created, this file contains "manifestURL" field that points to the _remote_ manifest URL that apppreload toolkit used to download package - e.g. domain.com/app1/package.manifest - aka mini-manifest. The package itself contains full manifest with predefined "origin" - e.g. app1.domain.com. When we flash such package to the device, home screen app icon is identified by manifestURL mentioned in metadata.json, but the app itself has origin defined in the full in-package manifest.

Should Homescreen app icon be identified by the manifest located in the package instead (e.g. data-identifier="app://app1.domain.com/manifest.webapp" or "app://GUID/manifest.webapp" if app didn't specify origin) or it works as expected?

Ricky, could you please help to understand if the way manifestURL from manifest.json is treated correctly by the build subsystem?

Chris, could you please point out how it's decided which URL to use in data-identifier attribute for the app icon?

* If the answer for the point above is "it works as expected", then I think the problem is in Raptor that forces app to have origin that directly maps to the host mentioned in Homescreen's data-identifier.

Comment 8

[:Eli Perelman]

Raptor using the origin for 2 main purposes:

1. Launching apps

In the CLI the user tells us what app to cold launch for running performance tests. We need to map this with an icon we can tap on the Homescreen. Using the origin and entry point in verticalhome seemed to work well as we can't launch an app using the standard Marionette interface because it has negative performance implications. By looking up and caching the coordinates of the app on the Homescreen, we can launch it later by just tapping on the coordinates instead of a software launch.

2. Getting performance entries

The PerformanceBase logger writes out performance entries with the context of the entry encoded in the log entry. We use these contexts to associate marks with particular apps, Gecko, or even cross-application contexts like Homescreen does with its appLaunch@ mark. In Raptor we just take the appLaunch URL and discard everything but the host [2].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsPerformance.cpp?from=PERFLOG#1041
[2] https://github.com/mozilla-raptor/raptor-cli/blob/bf0ad70ce05b9df0e5b531ef25114839e9b602ec/lib/parsers/performance.js#L36

Comment 9

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8683810 [details] [review]
[gaia] azasypkin:bug-1219176-raptor-load-apps > mozilla-b2g:master

Hey Eli,

I think this PR is ready for the first review :) Here are some quick notes:

* PR for bug 1223295 is landed so preload.py is ready to preload apps from GitHub Pages;

* PR for bug 1220744 is landed few days ago - so I've moved Raptor specific env variables to distros/raptor/distro.mk;

* After a deeper investigation I found that there is a way to have distro-specific preference as well (in addition to settings) - so I've removed DEVICE_DEBUG=1 from "make raptor" to fix bug 1219301 and added "b2g.adb.timeout" to distros/raptor/partner-prefs.js (however I'd rather rename this file to custom-prefs.js, but it's out of scope of this PR), and I tend to agree with Julien in bug 1219301 comment 8 that we don't need both "dom.apps.developer_mode" and "dom.webcomponents.enabled".

* I've removed service worker specific prefs and settings from this PR as well as test remote apps - I'll have dedicated PR for this in bug 1210704;

* I've tried to leverage customization subsystem to get rid of [1], but without success so I left it alone for now :)

Also please tell me if you prefer replace "distros/raptor/distro.mk(PERF_LOGGING=1)" with "distros/raptor/partner-prefs.js(this.userPrefs['dom.performance.enable_user_timing_logging'] = true;)"

----

Hey Tim,

I believe we also need your stamp on this gaia/general PR. How does it look to you?

Thanks!

[1] https://github.com/mozilla-b2g/gaia/blob/ed49c7b2f4d983198dd1dcb4cebddc14fb9a47a2/apps/ftu/build/build.js#L19

Comment 10

[Chris Lord [:cwiiis]]

Sorry for the delay on this - this is the code you're looking for:

https://github.com/gaia-components/gaia-site-icon/blob/master/script.js#L491

This is only used for marionette tests, so if there's a more convenient thing we can put there without breaking those tests, it's no big deal to change it.

Comment 11

[:Eli Perelman]

Comment on attachment 8683810 [details] [review]
[gaia] azasypkin:bug-1219176-raptor-load-apps > mozilla-b2g:master

Code changes look good, I like the new make command. I'm wondering, have you run a Raptor test using the old and new make to see if the prefs+settings cause a change in performance numbers?

Also, can we get this uplifted to v2.5 as test-only changes?

Comment 12

[Oleg Zasypkin [:azasypkin]]

(In reply to :Eli Perelman from comment #11)
> Comment on attachment 8683810 [details] [review]
> [gaia] azasypkin:bug-1219176-raptor-load-apps > mozilla-b2g:master
> 
> Code changes look good, I like the new make command. I'm wondering, have you
> run a Raptor test using the old and new make to see if the prefs+settings
> cause a change in performance numbers?

Nope, not yet, ni? myself to do the comparison.

> 
> Also, can we get this uplifted to v2.5 as test-only changes?

Yeah, I'd say that we should do that, otherwise v2.5 perf numbers will not be 100% comparable with master ones. I'll request uplift approval once we land it.


(In reply to Chris Lord [:cwiiis] from comment #10)
> Sorry for the delay on this - this is the code you're looking for:
> 
> https://github.com/gaia-components/gaia-site-icon/blob/master/script.js#L491
> 
> This is only used for marionette tests, so if there's a more convenient
> thing we can put there without breaking those tests, it's no big deal to
> change it.

Thanks Chris, I'll check if we can do anything safe here.

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8683810 [details] [review]
[gaia] azasypkin:bug-1219176-raptor-load-apps > mozilla-b2g:master

Ricky should review this.

Comment 14

[Oleg Zasypkin [:azasypkin]]

Okay here are the results, I've made two _sets_ of measurements (30 runs without PR, 30 runs with PR and again 30 runs without PR and 30 runs with PR) to be sure that there is a difference:

Gaia revision 426ee5e5582083e124ec95e180eb78a7d3fe8256, Build Identifier 20151111150236

Raptor@3.3.2
Raptor-compare@0.2.1
$ raptor test coldlaunch --app sms --runs 30

Attempt #1

Before PR
| Metric                | Mean     | Median   | Min    | Max    | StdDev  | 95% Bound |
| --------------------- | -------- | -------- | ------ | ------ | ------- | --------- |
| navigationLoaded      | 925.700  | 917      | 842    | 1025   | 43.344  | 941.210   |
| willRenderThreads     | 971.567  | 963.500  | 885    | 1097   | 45.396  | 987.811   |
| navigationInteractive | 974.600  | 966.500  | 887    | 1100   | 45.166  | 990.762   |
| visuallyLoaded        | 1258.067 | 1251.500 | 1175   | 1399   | 52.924  | 1277.005  |
| contentInteractive    | 1830.600 | 1820.500 | 1717   | 2039   | 82.973  | 1860.291  |
| objectsInitEnd        | 1861.167 | 1848.500 | 1743   | 2089   | 84.865  | 1891.535  |
| fullyLoaded           | 3386.667 | 3330     | 3114   | 4012   | 221.158 | 3465.807  |
| uss                   | 20.701   | 20.039   | 19.527 | 22.566 | 1.097   | 21.094    |
| pss                   | 25.241   | 24.577   | 24.068 | 27.106 | 1.097   | 25.633    |
| rss                   | 41.388   | 40.721   | 40.223 | 43.262 | 1.097   | 41.781    |

After PR
| Metric                | Mean     | Median   | Min    | Max    | StdDev | 95% Bound |
| --------------------- | -------- | -------- | ------ | ------ | ------ | --------- |
| navigationLoaded      | 852.767  | 855.500  | 795    | 902    | 26.886 | 862.388   |
| willRenderThreads     | 894.133  | 897      | 835    | 947    | 26.722 | 903.696   |
| navigationInteractive | 896.833  | 900      | 837    | 949    | 26.797 | 906.422   |
| visuallyLoaded        | 1170.100 | 1169     | 1095   | 1265   | 32.888 | 1181.869  |
| contentInteractive    | 1700.200 | 1693.500 | 1621   | 1911   | 49.636 | 1717.962  |
| objectsInitEnd        | 1733.067 | 1727.500 | 1648   | 1950   | 51.507 | 1751.498  |
| fullyLoaded           | 3208.400 | 3187.500 | 3107   | 3592   | 87.158 | 3239.589  |
| uss                   | 19.331   | 19.328   | 18.887 | 20.684 | 0.350  | 19.456    |
| pss                   | 23.804   | 23.801   | 23.361 | 25.172 | 0.352  | 23.930    |
| rss                   | 39.639   | 39.633   | 39.195 | 41.016 | 0.354  | 39.766    |

Raptor-compare
sms.gaiamobile.org     base: mean  1: mean  1: delta  1: p-value
---------------------  ----------  -------  --------  ----------
navigationLoaded              926      853       -73      * 0.00
willRenderThreads             972      894       -77      * 0.00
navigationInteractive         975      897       -78      * 0.00
visuallyLoaded               1258     1170       -88      * 0.00
contentInteractive           1831     1700      -130      * 0.00
objectsInitEnd               1861     1733      -128      * 0.00
fullyLoaded                  3387     3208      -178      * 0.00
uss                        20.701   19.331    -1.370      * 0.00
pss                        25.241   23.804    -1.437      * 0.00
rss                        41.388   39.639    -1.749      * 0.00

Attempt#2

Before PR
| Metric                | Mean     | Median   | Min    | Max    | StdDev  | 95% Bound |
| --------------------- | -------- | -------- | ------ | ------ | ------- | --------- |
| navigationLoaded      | 916.333  | 908      | 841    | 1142   | 57.557  | 936.930   |
| willRenderThreads     | 961.333  | 952.500  | 885    | 1186   | 58.407  | 982.234   |
| navigationInteractive | 964.600  | 956      | 888    | 1188   | 58.159  | 985.412   |
| visuallyLoaded        | 1244.033 | 1226.500 | 1159   | 1496   | 68.863  | 1268.676  |
| contentInteractive    | 1818.333 | 1787     | 1702   | 2293   | 119.723 | 1861.175  |
| objectsInitEnd        | 1851.200 | 1816     | 1729   | 2381   | 128.146 | 1897.056  |
| fullyLoaded           | 3350.433 | 3263     | 3177   | 4231   | 233.775 | 3434.089  |
| uss                   | 20.258   | 19.959   | 19.605 | 22.547 | 0.860   | 20.566    |
| pss                   | 24.803   | 24.502   | 24.147 | 27.091 | 0.860   | 25.110    |
| rss                   | 40.951   | 40.651   | 40.293 | 43.242 | 0.860   | 41.259    |

After PR
| Metric                | Mean     | Median   | Min    | Max    | StdDev | 95% Bound |
| --------------------- | -------- | -------- | ------ | ------ | ------ | --------- |
| navigationLoaded      | 860.800  | 858      | 809    | 934    | 24.219 | 869.467   |
| willRenderThreads     | 902.833  | 900.500  | 849    | 979    | 24.787 | 911.703   |
| navigationInteractive | 905.433  | 902.500  | 851    | 982    | 25.095 | 914.414   |
| visuallyLoaded        | 1186.267 | 1181     | 1126   | 1280   | 32.203 | 1197.791  |
| contentInteractive    | 1711.500 | 1705     | 1636   | 1835   | 42.941 | 1726.866  |
| objectsInitEnd        | 1740.633 | 1732.500 | 1664   | 1866   | 43.238 | 1756.106  |
| fullyLoaded           | 3213.467 | 3191.500 | 3133   | 3451   | 73.240 | 3239.675  |
| uss                   | 19.294   | 19.273   | 18.902 | 20.613 | 0.358  | 19.422    |
| pss                   | 23.770   | 23.746   | 23.384 | 25.052 | 0.352  | 23.896    |
| rss                   | 39.601   | 39.586   | 39.211 | 40.785 | 0.341  | 39.723    |

Raptor-compare
sms.gaiamobile.org     base: mean  1: mean  1: delta  1: p-value
---------------------  ----------  -------  --------  ----------
navigationLoaded              916      861       -56      * 0.00
willRenderThreads             961      903       -59      * 0.00
navigationInteractive         965      905       -59      * 0.00
visuallyLoaded               1244     1186       -58      * 0.00
contentInteractive           1818     1712      -107      * 0.00
objectsInitEnd               1851     1741      -111      * 0.00
fullyLoaded                  3350     3213      -137      * 0.00
uss                        20.258   19.294    -0.964      * 0.00
pss                        24.803   23.770    -1.032      * 0.00
rss                        40.951   39.601    -1.350      * 0.00

So yeah, looks like there is a difference especially in memory numbers :)

Comment 15

[Oleg Zasypkin [:azasypkin]]

Hey Tim,

Could not reach out to Ricky via IRC for quite a long time already, he's likely on PTO or something :)

Is anyone else on your team can help with the review here (~20 lines PR)?

Thanks!

Comment 16

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8683810 [details] [review]
[gaia] azasypkin:bug-1219176-raptor-load-apps > mozilla-b2g:master

Sorry for the late reply, patch looks good to me and I don't see any potential impact on gaia infra.

Comment 17

[Oleg Zasypkin [:azasypkin]]

Thanks all for reviews!

Okay, treeherder is green (except for the unrelated GU20 failure) and Raptor@4.0.0 worked fine for the Messages app locally, let's see if it sticks and works well with the Raptor dashboard.

Master: https://github.com/mozilla-b2g/gaia/commit/c66ca0bc6b6ad9c0d96485f3b0647e808c637953

Comment 18

[Oleg Zasypkin [:azasypkin]]

I'll wait for few more days to see how well it works and if everything is OK, will ask for uplift to v2.5.

Comment 19

[Oleg Zasypkin [:azasypkin]]

Hey Eli,

I was monitoring Raptor Dashboard expecting to see any difference after this patch (as I saw locally for memory consumed by Messages app), but I don't see anything like that :)

So just wanted to confirm if you use "make raptor" to gather data for dashboard or you directly use something like "GAIA_OPTIMIZE=1 DEVICE_DEBUG=1 PERF_LOGGING=1 RAPTOR=1 NO_LOCK_SCREEN=1 NOFTU=1 SCREEN_TIMEOUT=0 make reset-gaia" for that?

Thanks!

Comment 21

[:Eli Perelman]

Hey, you are correct in that we didn't use `make raptor` in automation, this is what we are currently using:

PERF_LOGGING=1 RAPTOR=1 DEVICE_DEBUG=1 GAIA_OPTIMIZE=1 NOFTU=1 SCREEN_TIMEOUT=0 make reset-gaia

Are we OK, to move to `make raptor`?

Comment 22

[Oleg Zasypkin [:azasypkin]]

(In reply to :Eli Perelman from comment #21)
> Are we OK, to move to `make raptor`?

Yeah, I'd love to! In this case we'll have less variables between dashboard and local runs.

Please, let me know if it doesn't work for dashboard for some reason and I'll try to fix that :)

Thanks!

Comment 23

[:Eli Perelman]

OK, it's switched. It will probably be a few hours before any changes are noticed.

Comment 24

[Oleg Zasypkin [:azasypkin]]

(In reply to :Eli Perelman from comment #23)
> OK, it's switched. It will probably be a few hours before any changes are
> noticed.

Thanks!

It looks like the most recent drop in memory usage for Flame [1] is caused by this \0/ However nothing changed for Aries, weird :)

[1] https://raptor.mozilla.org/dashboard/script/memory?var-device=flame-kk&var-memory=512&var-branch=master&var-test=cold-launch&from=1448869058276&to=1448962658276&var-metric=All&var-aggregate=MEAN(value)%20%2B%20(1.96%20*%20STDDEV(value)%20%2F%205.477)&var=test

Comment 25

[:Eli Perelman]

Hey gents, could one of you make this change to the Aries jobs as well? Need to switch to using `make raptor` against the devices.

Comment 26

[Mike Lien[:mlien]]

The reason is due to make raptor failed from Nov. 28 on Aries, it shows I should use v4.2 nodejs since Nov. 28.

Comment 27

[Oleg Zasypkin [:azasypkin]]

(In reply to Mike Lien[:mlien] from comment #26)
> The reason is due to make raptor failed from Nov. 28 on Aries, it shows I
> should use v4.2 nodejs since Nov. 28.

Should not we all update our Node installation to v4.2 since Nov. 21 per [1] and [2]?

[1] https://groups.google.com/d/msg/mozilla.dev.fxos/0KDx55nzcTU/PlBqMQrHAwAJ
[2] https://github.com/mozilla-b2g/gaia/commit/07cac3b26ee06c7dd30eeac43571c5f2dfc18947

Comment 28

[Mike Lien[:mlien]]

I just point out why recent days the make raptor is failed and I have resumed it. Now the Aries' environment is on nodejs 4.2 and raptor 4.0.0

Comment 29

[Oleg Zasypkin [:azasypkin]]

(In reply to Mike Lien[:mlien] from comment #28)
> I just point out why recent days the make raptor is failed and I have
> resumed it. Now the Aries' environment is on nodejs 4.2 and raptor 4.0.0

Ah, nice, thanks!