1154025 - [Music] update gaia-header, icons and themes to see if it improves startup time

Status: RESOLVED FIXED

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

Description

[David Flanagan [:djf]]

Looks like the Music app got updated to use Bower for managing dependencies, but no one ever updated those dependencies to pull in the latest versions.

I know that speedups have landed in gaia-header at least, so maybe part of our regression is that we are not using the lastest stuff.

The purpose of this bug is to investigate the startup time impact of switching the music app to use the current versions in the shared/ directory instead of the static versions in bower_compoennts/

Comment 1

[David Flanagan [:djf]]

On master currently, I get these startup stats:

t: 1444 t̄: 1454 σ: 31 n: 12 median: 1455 min: 1385 max: 1495 outliers: 1

Comment 2

[David Flanagan [:djf]]

If I switch the bower_components stuff to shared/elements/ to pick up the most current versions, I get:

t: 1380 t̄: 1450 σ: 38 n: 12 median: 1449 min: 1380 max: 1515 outliers: 1 

Startup time is basically unchanged, maybe just slightly faster.  So using out-of-date versions isn't the primary source of the startup time regression.

Comment 3

[David Flanagan [:djf]]

If I remove the gaia-icons stylesheet (which we do not use in 2.1) I get:

t: 1437 t̄: 1394 σ: 60 n: 20 median: 1388 min: 1288 max: 1578 outliers: 1 

So that gives a 50 to 60ms startup time improvement. If the icons are not needed on the first page of the app (and I think they are not) then maybe we can load them after startup.

Comment 4

[David Flanagan [:djf]]

Oops. It turns out that I had not actually removed the icons. The timing in comment #3 was a repeat of the timing in comment #2. So maybe updating to the latest actually makes a bigish difference.  I should do larger trial runs.

Comment 5

[David Flanagan [:djf]]

And when I actually do remove the gaia-icons file, I'm back to a slower startup time, so I don't know what is really going on here.

Comment 6

[David Flanagan [:djf]]

Hmm. Looks like gaia-header loads gaia-icons if it is not already loaded, so just commenting out gaia-icons.css doesn't really change anything.

Comment 7

[David Flanagan [:djf]]

With the icons and the header component commented out:

t: 1396 t̄: 1403 σ: 93 n: 15 median: 1411 min: 1228 max: 1591 outliers: 1 

This just isn't making much sense. I need longer, more automated test runs, I think.

Comment 8

[David Flanagan [:djf]]

I can't get my automated gallery launcher script to work for music, so I modified my startup_time_tester.js script to wait for the app to be fully loaded, then wait 2 seconds, then call location.reload(). I let it reload it self a lot of time and get these stats:

Using bower_components/

t: 1100 t̄: 982 σ: 131 n: 70 median: 967 min: 771 max: 1285 outliers: 3 

Using share/elements/

t: 832 t̄: 960 σ: 123 n: 70 median: 925 min: 793 max: 1244 outliers: 2

There is a lot of variability... the sigma value never goes down even with 70 trials. And just reloading the app over and over like this isn't great testing methodology, but it does seem clear that using up-to-date components will give us a bit of speedup.  40ms if we compare median numbers here.

And it just makes sense that we should be using current versions.  Since we've failed at keeping things up to date with bower, I'm going to submit a patch that removes bower_components/ and uses shared/elements/ instead.

Comment 9

[GitHub Autolander Bot]

Attachment: [gaia] davidflanagan:bug1154025 > mozilla-b2g:master

Comment 10

[David Flanagan [:djf]]

Attachment: link to github

Dominic,

It looks like using the current verisons of the icons, themes and headers gives us a bit of startup time improvement.

Since we haven't been good about keeping bower_compoennts/ up to date, it seems to me that the easiest thing to do is revert to using shared/elements/.  But we could also just update bower_components if you'd prefer to do it that way.

Comment 11

[Dominic Kuo [:dkuo]]

Comment on attachment 8591966 [details] [review]
link to github

David,

The patch looks good to me, though I didn't see much improvement on the startup times, but if your tests confirmed the gaia-header caused regression on startup, then it should make sense to revert to use shared/elements/, just like the other gaia apps.

I am using the Raptor to measure the startup times:
https://developer.mozilla.org/en-US/Firefox_OS/Automated_testing/Raptor

I remember it's Wilson made the gaia-header changes, maybe we should ask him to give some inputs?

Comment 12

[David Flanagan [:djf]]

Dominic,

It sounds like you misunderstood. It is not gaia-header that caused the regression. What I'm concerned about here is that the music app is using an out of date version of gaia-header since we've never updated the version in bower_compoents. So this patch is just modifying the app to use the most current version of gaia-header and related stuff.  I believe that performance improvements have been landed in the current version that do not exist in the old versions that the music app is currently using.

What Raptor commands do you use to measure performance? How many runs? Maybe if you put that information in the parent bug 1153985 we could all use the same technique for measurement.

Comment 13

[Dominic Kuo [:dkuo]]

(In reply to David Flanagan [:djf] from comment #12)
> Dominic,
> 
> It sounds like you misunderstood. It is not gaia-header that caused the
> regression. What I'm concerned about here is that the music app is using an
> out of date version of gaia-header since we've never updated the version in
> bower_compoents. So this patch is just modifying the app to use the most
> current version of gaia-header and related stuff.  I believe that
> performance improvements have been landed in the current version that do not
> exist in the old versions that the music app is currently using.

Thanks David, I didn't misunderstand it but I guess I described it improperly(I shouldn't use "revert"). I found only Music and Camera use the bower_compoennts/ styles, the other apps use shared/elements styles, and this means the style version of Music app is different from the others, because it manages its own bower settings. So what you are doing is to make Music's style update-to-date, also gains the performance improvements which is landed in the current version styles under shared/elements.

I needinfoed Wilson is because I don't know why Music's style version is not sync with the other apps, maybe there is some other reason? or we just forget to update it...

> What Raptor commands do you use to measure performance? How many runs? Maybe
> if you put that information in the parent bug 1153985 we could all use the
> same technique for measurement.

I used:
1. |make raptor|
2. |APP=music RUNS=5 node tests/raptor/launch_test|

I was told(should be the Gaia weekly meeting) Raptor is going to replace |make test-perf| so I use it to start the performance investigation, it's easier to get performance tests results, also, the daily results are online in [1] to replace Datazilla. I will put those information about Raptor in bug 1153985 and hope to help us on finding the root cause.

[1] http://raptor-ui.divshot.io/

Comment 14

[Wilson Page [:wilsonpage]]

We made a few optimizations to gaia-header recently, so updating to latest should improve startup a bit. It's up to the app owner whether or not to use shared or bower, I don't really mind.

You are also able to optimize using `title-start` and `title-end` attribute hints on <gaia-header> if any buttons within are guaranteed not to change size [1]. This avoids the need for the component to take costly measurements from the DOM.

[1] https://github.com/gaia-components/gaia-header#optimizing-with-title-start--title-end

Comment 15

[Wilson Page [:wilsonpage]]

I might also add that if we're looking to move apps into their own repos (as outline in the v3 architecture proposal) we will have to use Bower (or something similar) as `shared/` won't be a thing.

Comment 16

[David Flanagan [:djf]]

Trying raptor on the 2.2 branch without this patch. 30 runs:

Metric                            Mean      Median    Min       Max       StdDev   p95     
--------------------------------  --------  --------  --------  --------  -------  --------
coldlaunch.navigationLoaded       669.600   675.500   605.000   723.000   28.057   720.000 
coldlaunch.navigationInteractive  816.533   807.000   746.000   948.000   44.333   920.000 
coldlaunch.visuallyLoaded         1222.767  1220.000  1107.000  1360.000  50.904   1354.000
coldlaunch.contentInteractive     1223.467  1221.000  1108.000  1361.000  50.985   1355.000
coldlaunch.fullyLoaded            2911.300  2873.000  2647.000  3160.000  118.161  3119.000
coldlaunch.uss                    17.173    17.200    16.700    17.400    0.200    17.400  
coldlaunch.pss                    22.027    22.100    21.500    22.300    0.206    22.300  
coldlaunch.rss                    34.020    34.100    33.400    34.300    0.217    34.300  

And these are the numbers with the patch. 30 runs again:

Metric                            Mean      Median    Min       Max       StdDev   p95     
--------------------------------  --------  --------  --------  --------  -------  --------
coldlaunch.navigationLoaded       688.100   684.000   622.000   878.000   51.417   804.000 
coldlaunch.navigationInteractive  827.667   809.500   762.000   1073.000  64.070   970.000 
coldlaunch.visuallyLoaded         1212.000  1206.500  1073.000  1478.000  85.287   1437.000
coldlaunch.contentInteractive     1212.633  1207.500  1074.000  1478.000  85.368   1441.000
coldlaunch.fullyLoaded            2924.000  2877.000  2645.000  3746.000  207.454  3335.000
coldlaunch.uss                    18.287    18.200    17.900    21.700    0.730    19.900  
coldlaunch.pss                    22.610    22.500    22.200    25.900    0.710    24.200  
coldlaunch.rss                    33.123    33.050    32.700    36.400    0.708    34.700 

Comparing the mean and medians of the visuallyLoaded number, we get a 10 to 14ms improvement.  (Though the 95th percentile number gets worse)

Its not much, but it seems like a real difference, and we need to land this patch anyway so we're not using out of date components.

Comment 17

[David Flanagan [:djf]]

Comment on attachment 8591965 [details] [review]
[gaia] davidflanagan:bug1154025 > mozilla-b2g:master

Carrying Dominic's r+ on to this version of the attachment (the very same patch) so that autolander will (hopefully) land it.

Comment 18

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/ae7527a6e6892b92b67925b3c892e03c6d920ff3

Comment 19

[David Flanagan [:djf]]

Comment on attachment 8591966 [details] [review]
link to github

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): not a regression

[User impact] if declined: music app will start up slightly slower, and will be subject to whatever bugs have been fixed in gaia-header over the last 4 months because we're using an outdated versions.

[Testing completed]: yes

[Risk to taking this patch] (and alternatives if risky): not risky. All the other apps are using the current versions of these components without problems, so I don't expect problems.  Note that this looks like a large patch because it is deleting the out-of-date local version of the components. But all it is doing is replacing that with the up-to-date shared versions. So really it is more like a 3-line patch.

[String changes made]: none

Comment 20

[bhavana bajaj [:bajaj]]

Comment on attachment 8591966 [details] [review]
link to github

approving to see if this will have any perf win, if we have any serious fallouts we should back this out.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/bcaba2b4ab50dd54dc17e58b0ba6e57b6107c852