1175045 - [Messages][NGA] Add serviceworkerware to our codebase and update the script for external lib

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Steve Chung [:steveck]]

Once the serviceworkerware[1] is ready, we will need to utilize it in our codebase and update the script for updating all the external lib including threads and this one.

[1] https://github.com/gaia-components/serviceworkerware

Comment 1

[Steve Chung [:steveck]]

Since using bower install will wrap the source into dist/sww.js because bower.json already defined it, do we only need to copy the packaged sww.js to lib folder after bower install in the script, or you would prefer to keep the original source in the bower component?

Comment 2

[Oleg Zasypkin [:azasypkin]]

(In reply to Steve Chung [:steveck] from comment #1)
> Since using bower install will wrap the source into dist/sww.js because
> bower.json already defined it, do we only need to copy the packaged sww.js
> to lib folder after bower install in the script, or you would prefer to keep
> the original source in the bower component?

As for now, I'm fine with "dist/sww.js" only.

Comment 3

[Julien Wajsberg [:julienw]]

Both work for me, but I suggest to keep consistency and copy it in lib/

Comment 4

[Oleg Zasypkin [:azasypkin]]

I think my previous comment was a bit ambiguous, so let me clarify what I meant :)

I think we need: "bower_components/serviceworkerware/dist/sww.js" ---> "lib/sww.js" (no need in sww.js.map for now);

not: "bower_components/serviceworkerware/dist/sww.js" ---> "dist/sww.js" :)

Comment 5

[Steve Chung [:steveck]]

Attachment: Link to github

Hey,
I added the serviceworkerware into and update the threads, please tell me if you have any thought about updating the libs, thanks!

Comment 6

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8624021 [details] [review]
Link to github

Looks good! The only nit left at GitHub, i.e. we need more generic name for our shell script now.

Thanks!

Comment 7

[Steve Chung [:steveck]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #6)
> Comment on attachment 8624021 [details] [review]
> Link to github
> 
> Looks good! The only nit left at GitHub, i.e. we need more generic name for
> our shell script now.
> 
Thanks for the reminder, I update the name to 'update_nga_libs.sh'

Comment 8

[Robert Wood [:rwood]]

Hi :steveck

I am pretty confident that this patch introduces an app start-up performance regression in the SMS app. I noticed it via the Raptor performance tests (not posting to treeherder, but still running). I retriggered the raptor launch test suite several times on your PR, and each time a significant SMS app launch time regression was detected:

http://docs.taskcluster.net/tools/task-inspector/#4BjvmHYtRuuR3DcFhZuPug/0

http://docs.taskcluster.net/tools/task-inspector/#9ksEdtyVQe-QqL40L8PlOg/2

http://docs.taskcluster.net/tools/task-inspector/#TAeau0nmT0WjBq8GPMsNLw/2

https://tools.taskcluster.net/task-inspector/#I6W3XFPrR0umHCcAwPMNlg/0

Flagging as :needinfo so you see these results, thanks

Comment 9

[Julien Wajsberg [:julienw]]

I think this comes from the bridge change in

https://github.com/mozilla-b2g/gaia/pull/30642/files#diff-89cb758a8e79fdb633ccb78053c9dd3eL1732

Calling setReady is calling serviceReady which is sending an event which in turns do more stuff.

Steve maybe you can remove the change to the bridge from this PR for now?

NI Wilson so that he's aware of the issue.

Comment 10

[Wilson Page [:wilsonpage]]

(In reply to Julien Wajsberg [:julienw] from comment #9)
> I think this comes from the bridge change in
> 
> https://github.com/mozilla-b2g/gaia/pull/30642/files#diff-
> 89cb758a8e79fdb633ccb78053c9dd3eL1732
> 
> Calling setReady is calling serviceReady which is sending an event which in
> turns do more stuff.
> 
> Steve maybe you can remove the change to the bridge from this PR for now?
> 
> NI Wilson so that he's aware of the issue.

Interesting. I thought that removing the setTimeout, would speed up connection times as we're no longer deferring any handshake logic. IIRC Oleg wanted it removing too for some other reason.

Comment 11

[Oleg Zasypkin [:azasypkin]]

Well, all this sounds very strange to me, when Messages app is run from HomeScreen (I think it's the case for raptor tests), we don't create any bridge service or client yet, so bridge should not affect anything here unless it does something on the JS file parsing stage.

I'll try to dig into it today and see what is really happening.

Comment 12

[Oleg Zasypkin [:azasypkin]]

Here is what I have locally using the following commands (for each revision):

1. "make raptor";
2. "RUNS=10 APP=sms node tests/raptor/launch_test".

With the latest PR (cc3c5537a5876ccd2eac3bd0dc1edc774d5507d0)
Metric                            Mean      Median    Min       Max       StdDev  p95     
--------------------------------  --------  --------  --------  --------  ------  --------
coldlaunch.navigationLoaded       881.400   900.000   830.000   915.000   33.664  915.000 
coldlaunch.willRenderThreads      915.100   931.000   862.000   949.000   32.709  949.000 
coldlaunch.navigationInteractive  918.000   933.500   864.000   952.000   32.066  952.000 
coldlaunch.visuallyLoaded         1039.600  1050.500  994.000   1071.000  28.200  1071.000
coldlaunch.fullyLoaded            1164.700  1168.000  1115.000  1209.000  29.692  1209.000
coldlaunch.uss                    14.780    14.800    14.300    15.100    0.244   15.100  
coldlaunch.rss                    33.800    33.900    32.700    34.100    0.405   34.100  
coldlaunch.pss                    19.260    19.300    18.500    19.600    0.320   19.600  
coldlaunch.contentInteractive     1493.300  1495.000  1446.000  1545.000  30.695  1545.000
coldlaunch.objectsInitEnd         1536.167  1544.500  1499.000  1563.000  26.630  n/a

With the PR's HEAD~1 (2b75846f9b35620ddf8ffadc66ef2d2a650b65db), aka base revision
Metric                            Mean      Median    Min       Max       StdDev  p95     
--------------------------------  --------  --------  --------  --------  ------  --------
coldlaunch.navigationLoaded       888.000   884.500   830.000   972.000   38.486  972.000 
coldlaunch.willRenderThreads      923.900   921.500   863.000   1009.000  39.175  1009.000
coldlaunch.navigationInteractive  926.600   924.000   865.000   1013.000  39.543  1013.000
coldlaunch.visuallyLoaded         1049.400  1044.500  996.000   1136.000  38.352  1136.000
coldlaunch.fullyLoaded            1178.200  1168.500  1130.000  1249.000  34.333  1249.000
coldlaunch.uss                    14.720    14.750    14.400    15.000    0.218   15.000  
coldlaunch.pss                    19.190    19.100    19.000    19.500    0.207   19.500  
coldlaunch.rss                    33.720    33.700    33.300    34.100    0.244   34.100  
coldlaunch.contentInteractive     1505.800  1500.000  1448.000  1566.000  31.818  1566.000
coldlaunch.objectsInitEnd         1550.625  1547.000  1490.000  1617.000  36.602  n/a  

I don't see almost any difference between raptor results.


Hey Robert, am I doing something wrong or interpret results incorrectly?

Comment 13

[Robert Wood [:rwood]]

Hi Oleg,

We are using the p95 value of the coldlaunch.visuallyLoaded metric, for 30 RUNS. There is a difference:

Base rev:  1136.000 ms
Patch rev: 1071.000 ms
Diff: 65 ms or 5.7% faster launch time

The values are lower on device, on gaia-ci / treeherder the raptor tests run on emulator which is alot slower and has greater variance.

I would try running with RUNS=30, and do that a few times and you should see the consistent improvement.

Comment 14

[Oleg Zasypkin [:azasypkin]]

Attachment: Local raptor results (30x3 RUNS)

(In reply to Robert Wood [:rwood] from comment #13)
> Hi Oleg,
> 
> We are using the p95 value of the coldlaunch.visuallyLoaded metric, for 30
> RUNS. There is a difference:
> 
> Base rev:  1136.000 ms
> Patch rev: 1071.000 ms
> Diff: 65 ms or 5.7% faster launch time

Mmm, that means that with the patch it's even faster, right? :)

> 
> The values are lower on device, on gaia-ci / treeherder the raptor tests run
> on emulator which is alot slower and has greater variance.
> 
> I would try running with RUNS=30, and do that a few times and you should see
> the consistent improvement.

So I tried 3 times with RUNS=30, without data and with light workload. Here are results (see attached file for full results) for p95, coldlaunch.visuallyLoaded metric:

Latest PR (cc3c5537a5876ccd2eac3bd0dc1edc774d5507d0), without any data:
[1086.000, 1069.000, 1070.000]

PR's HEAD~1 (2b75846f9b35620ddf8ffadc66ef2d2a650b65db), aka base revision, no data
[1058.000, 1059.000, 1123.000]

Latest PR (cc3c5537a5876ccd2eac3bd0dc1edc774d5507d0), with light workload
[1308.000, 1337.000, 1372.000]

PR's HEAD~1 (2b75846f9b35620ddf8ffadc66ef2d2a650b65db), aka base revision, with light workload
[1363.000, 1301.000, 1295.000]

So the max difference I can see here is about 6% that looks like margin of error rather than regression... Maybe we need to trigger tests on Treeherder again and see if something has changed.

Comment 15

[Julien Wajsberg [:julienw]]

To get the most accurate results you should actually merge all your runs in one run and do the calculation.

Having 3 different runs does not give more confidence :)

That said, I agree there does not look to have an issue here. I think this was a false positive. Moreover we _know_ that working on NGA will give performance regression and that we'll work on this later on.

So let's move forward.

Thanks Robert for the heads up though, please report again if you see something in the future, that's really appreciated !

Comment 16

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #9)
> I think this comes from the bridge change in
> 
> https://github.com/mozilla-b2g/gaia/pull/30642/files#diff-
> 89cb758a8e79fdb633ccb78053c9dd3eL1732
> 
> Calling setReady is calling serviceReady which is sending an event which in
> turns do more stuff.
> 
> Steve maybe you can remove the change to the bridge from this PR for now?
> 

Anyway I still split the patch to simply including the sww lib in this patch, and we can create another bug for tracking the possible regression for the latest threads lib.

Comment 17

[Julien Wajsberg [:julienw]]

Comment on attachment 8624021 [details] [review]
Link to github

r=me

let's move forward here !

Comment 18

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] from comment #15)
> To get the most accurate results you should actually merge all your runs in
> one run and do the calculation.
> 
> Having 3 different runs does not give more confidence :)

Well it should have made you more confident! I separated it intentionally to do the same thing as Treeherder does when raptor job is re-run 3 times: it runs tests 30 times for PR revision, then 30 times for base revision, compares the difference and fails the job if performance regression exceeds "acceptable threshold".

So none of those 3 local runs would trigger alarm :)

Comment 19

[Steve Chung [:steveck]]

Let's land the sww first and track the regression later.

In master: https://github.com/mozilla-b2g/gaia/commit/7d2e76e37919f85f4883efbcccf503ffae2e02c1