1208154 - [Music][NGA] Replace 'apps/music' with 'dev_apps/music-nga'

Status: RESOLVED FIXED

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

Description

[Justin D'Arcangelo [:justindarc]]

After Bug 1208108 lands, Music NGA will be feature complete. At this point, the Music app in 'apps/music' should be replaced with the Music NGA app in 'dev_apps/music-nga'. Additionally, the current (OGA) Music app in 'apps/music' should be moved to 'dev_apps/music-oga' as a reference for comparing behaviors between the two apps.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] justindarc:bug1208154 > mozilla-b2g:master

Comment 2

[Hubert Figuiere [:hub]]

Left a few comments on Github as to why tests are failing. I haven't validated them though.

Comment 3

[Justin D'Arcangelo [:justindarc]]

Addressed :hub's comments and fixed some of the linter errors.

Also, here are the Raptor coldlaunch numbers for OGA vs. NGA using this patch:

OGA:
$ raptor test coldlaunch --app music-oga --runs 5
[Cold Launch: music-oga.gaiamobile.org] Preparing to start testing...
[Cold Launch: music-oga.gaiamobile.org] Priming application
[Cold Launch: music-oga.gaiamobile.org] Starting run 1
[Cold Launch: music-oga.gaiamobile.org] Run 1 complete
[Cold Launch: music-oga.gaiamobile.org] Starting run 2
[Cold Launch: music-oga.gaiamobile.org] Run 2 complete
[Cold Launch: music-oga.gaiamobile.org] Starting run 3
[Cold Launch: music-oga.gaiamobile.org] Run 3 complete
[Cold Launch: music-oga.gaiamobile.org] Starting run 4
[Cold Launch: music-oga.gaiamobile.org] Run 4 complete
[Cold Launch: music-oga.gaiamobile.org] Starting run 5
[Cold Launch: music-oga.gaiamobile.org] Run 5 complete
[Cold Launch: music-oga.gaiamobile.org] Results from music-oga.gaiamobile.org

| Metric                | Mean     | Median | Min    | Max    | StdDev  | p95      |
| --------------------- | -------- | ------ | ------ | ------ | ------- | -------- |
| navigationLoaded      | 3090.800 | 3129   | 2661   | 3378   | 235.869 | 3090.800 |
| navigationInteractive | 3321.600 | 3376   | 2976   | 3546   | 188.100 | 3321.600 |
| visuallyLoaded        | 3537.800 | 3577   | 3243   | 3739   | 163.369 | 3537.800 |
| contentInteractive    | 3537.800 | 3577   | 3243   | 3739   | 163.369 | 3537.800 |
| fullyLoaded           | 4688.800 | 4737   | 4396   | 4915   | 175.409 | 4688.800 |
| uss                   | 20.911   | 20.945 | 20.660 | 21.102 | 0.147   | 20.911   |
| rss                   | 43.276   | 43.305 | 43.023 | 43.469 | 0.148   | 43.276   |
| pss                   | 25.097   | 25.129 | 24.846 | 25.288 | 0.146   | 25.097   |

[Cold Launch: music-oga.gaiamobile.org] Testing complete


NGA:
$ raptor test coldlaunch --app music --runs 5
[Cold Launch: music.gaiamobile.org] Preparing to start testing...
[Cold Launch: music.gaiamobile.org] Priming application
[Cold Launch: music.gaiamobile.org] Starting run 1
[Cold Launch: music.gaiamobile.org] Run 1 complete
[Cold Launch: music.gaiamobile.org] Starting run 2
[Cold Launch: music.gaiamobile.org] Run 2 complete
[Cold Launch: music.gaiamobile.org] Starting run 3
[Cold Launch: music.gaiamobile.org] Run 3 complete
[Cold Launch: music.gaiamobile.org] Starting run 4
[Cold Launch: music.gaiamobile.org] Run 4 complete
[Cold Launch: music.gaiamobile.org] Starting run 5
[Cold Launch: music.gaiamobile.org] Run 5 complete
[Cold Launch: music.gaiamobile.org] Results from music.gaiamobile.org

| Metric                | Mean     | Median | Min    | Max    | StdDev  | p95      |
| --------------------- | -------- | ------ | ------ | ------ | ------- | -------- |
| navigationLoaded      | 3128.800 | 3186   | 2751   | 3308   | 195.190 | 3128.800 |
| navigationInteractive | 3150.200 | 3206   | 2768   | 3337   | 197.928 | 3150.200 |
| visuallyLoaded        | 4053.600 | 4109   | 3740   | 4212   | 163.307 | 4053.600 |
| contentInteractive    | 4058.400 | 4114   | 3756   | 4212   | 157.731 | 4058.400 |
| fullyLoaded           | 4059.600 | 4115   | 3759   | 4213   | 156.945 | 4059.600 |
| uss                   | 28.072   | 27.922 | 27.312 | 29.348 | 0.678   | 28.072   |
| rss                   | 50.415   | 50.254 | 49.660 | 51.695 | 0.679   | 50.415   |
| pss                   | 32.229   | 32.075 | 31.471 | 33.506 | 0.678   | 32.229   |

[Cold Launch: music.gaiamobile.org] Testing complete

NOTE: It currently looks like about a 600ms improvement with the NGA app. However, there is currently an OS-wide performance regression that is drastically impacting startup time across the board. I am unclear as to if the regression affects all apps equally. If so, then it looks like we may see a nice startup time improvement once NGA lands. The regression is being tracked in Bug 1204837.

Comment 4

[[:fabrice] Fabrice Desré]

Is fullyLoaded the interesting metric though, or contentInteractive? The memory usage really worse too, with a 7MB regression in uss.

Comment 5

[Justin D'Arcangelo [:justindarc]]

In NGA, the two perf marks happen at the same time. I'd have to take a closer look to see when exactly the OGA Music app is triggering them.

Also, we haven't spent much effort heavily optimizing yet, but I'm optimistic that there's lots of easy wins for us to get.

Comment 6

[Justin D'Arcangelo [:justindarc]]

Tests are now passing. This is ready to land after review.

Comment 7

[Justin D'Arcangelo [:justindarc]]

Landed on master:

https://github.com/mozilla-b2g/gaia/commit/33381d5a9136e673e50b798ffb7727c2b5c23496

Comment 8

[[:fabrice] Fabrice Desré]

fwiw, I'm not happy with landing such a big memory regression. What is the plan to fix it?

Comment 9

[Jim Porter (:squib)]

The plan is to work on optimizing stuff as we find issues, and if things don't work out, we ship the old version of the music app instead. Making this swap gives us better test coverage, since dogfooders will be using the new app and presumably finding lots of bugs.

Comment 10

[Justin D'Arcangelo [:justindarc]]

(In reply to [:fabrice] Fabrice Desré from comment #8)
> fwiw, I'm not happy with landing such a big memory regression. What is the
> plan to fix it?

First, I want to take some more realistic measurements to see how big the difference is. The numbers above were simply after launching the app and sitting at the tiles view. I want to see what they look like when actually navigating around the app and starting music playback. In the new app, each "view" is a separate <iframe> and they are disposed of when not needed anymore. We are also taking advantage of <gaia-fast-list> which allows us to show an infinite list of songs without filling up the DOM. So, I think if we do a more real-world comparison between the apps, we may actually be a lot closer than it first looks.

Also, as Jim said, we are now beginning the optimization and bugfixes phase, so I expect that we will see that startup memory number go down soon.

Comment 11

[Francesco Lodolo [:flod]]

There's one change for the string with ID 'playlists', "Playlist" becomes "Playlists". 

What's the reasoning behind this change? Just trying to figure out if we should file a follow-up bug to rename the string, and make sure localizers are aware of the change.

Comment 12

[Justin D'Arcangelo [:justindarc]]

(In reply to Francesco Lodolo [:flod] from comment #11)
> There's one change for the string with ID 'playlists', "Playlist" becomes
> "Playlists". 
> 
> What's the reasoning behind this change? Just trying to figure out if we
> should file a follow-up bug to rename the string, and make sure localizers
> are aware of the change.

It was incorrect before.

Before:
playlists => "Playlist"

After:
playlists => "Playlists"

Comment 13

[Francesco Lodolo [:flod]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #12)
> It was incorrect before.
> 
> Before:
> playlists => "Playlist"
> 
> After:
> playlists => "Playlists"

Thanks, then I think having a new string ID is the right way to go.