844029 - [music] Hide unnecessary DOM elements before Music app is launched to improve startup time

Status: VERIFIED FIXED

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

Description

[Dominic Kuo [:dkuo]]

This is a split bug to improve Music startup time of Bug 817115.

I was thinking that if Music app can lazily load some DOM elements to speed up the startup.

1. At first, I try to remove/mark the DOM elements(in index.html) which are not needed in the first view of Music, and yes, it does save 200 ~ 300ms on startup.
2. I try load the CSS which are only needed in the first view, but it doesn't make any differences, the startup time is almost the same.
3. So I was guessing that it's because the elements in DOM tree will trigger gecko to render or apply styles on elements, these actions seems kind of heavy because I just remove/mark some elements than I can get 200 ~ 300ms improvements.
4. Then I try to apply a hidden class which contains - { display: none } to all the unneeded elements, I can still save 200 ~ 300ms, so if I am right, use { display: none } will not trigger gecko to pre-render the DOM elements.

That's what I did in this patch, plus the logic that removing { display: none } when elements are needed.

And I also added some logic to lazily load the JS of music player because the Player.js seems also slow down the startup(20 ~ 50ms), and it's not needed at the first sight.

Comment 1

[Dominic Kuo [:dkuo]]

Attachment: Hide all pages before startup and lazily load Player.js

Comment 2

[David Flanagan [:djf]]

Comment on attachment 717034 [details]
Hide all pages before startup and lazily load Player.js

This makes a noticeable difference in startup time. r+

See my github comments, however: there are places where the code could be simplified, and a couple of possible bugs.

I recommend making the side-to-side transitions even a little faster.

And finally, consider changing the MediaDB() constructor call to pass a wrapper function instead of the actual metadata parser.  In that wrapper function you can use LazyLoader.load() to load blobview.js and metadata.js.  Most of the time when the music app start up, the mediadb scan will not find new music and neither of those files will be required.  Its probably a very easy way to win another 50ms or so.

Comment 3

[David Flanagan [:djf]]

See https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/js/gallery.js#L186 for an example of how I did lazy loading of the metadata parser for the gallery

Comment 4

[Dominic Kuo [:dkuo]]

David, I have followed what you did in the gallery app, to merge the blobview.js and metadata.js into metadata_srcipts.js for lazy loading, and it does save another 50ms for us!

And since this patch is r+, I am merging it to master first.

Comment 5

[David Flanagan [:djf]]

Dominic,

I just noticed that when I did a git pull master.  Since you've concatenated those two files manually, you should probably git rm metadata.js so we don't have two versions of the file.  The downside to that is that your app won't get future bug fixes to shared/js/blobview.js

Alternatively, you can do what I did for Gallery and create an app-specific Makefile to concatenate shared/js/blobview.js and js/metadata.js into metadata_scripts.js at build time. And then create an app-specific .gitignore file to mark metadata_scripts.js as an ignored file.

Comment 6

[Dominic Kuo [:dkuo]]

Oops! I thought that metadata_scripts in gallery was manually concatenated to one, and now I realized how gallery did this, thanks for reminding this. I think it's better to create an app-specific Makefile to achieve this, so I am sending another patch to complete the previous one.

Comment 7

[Dominic Kuo [:dkuo]]

Attachment: use Makefile to concatenate JS scripts

David, 

I used Makefile to concatenate JS files just like Gallery app did for lazy loading. To avoid I do this wrong, I would like another review from you, thanks.

And I found I accidentally broke the open web activity for Music player due to the CSS changes from last patch, so also added few changes to fix it.

Comment 8

[David Flanagan [:djf]]

Comment on attachment 718331 [details]
use Makefile to concatenate JS scripts

Looks good!

Comment 9

[Dominic Kuo [:dkuo]]

master: 41035e8e2dbdbfe60c7df3c0b362544393d69190
master: 2a1e50d4ccdeb4fc752e2ace8e487a5a17d1e113

Comment 10

[Dominic Kuo [:dkuo]]

Please reference to https://datazilla.mozilla.org/b2g

After these two patches are applied, the startup time is reduced from 1029ms to 762ms, that means it's a 25% improvement of music app!

Since this is a performance work and reduced the startup time less than 1 second, I would like to nominate this to tef+ because it's worthy and critical, thanks.

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Given that we're one day away from shipping v1.0.1 we don't have the time to take on the risk of this patch and will have to let it ride the train in v1.1

Comment 12

[Andrew Overholt [:overholt]]

So does this work just need to be uplifted?

Comment 13

[Dominic Kuo [:dkuo]]

Sorry everyone I did not notice this issue was changed to tef+, since it's a performance work and reduced the startup time to less than 1 second, I think it deserved to be uplifted.

Comment 14

[Alex Keybl [:akeybl]]

To be uplifted by our sheriffs, the bug must be marked as Resolved/Fixed.

Comment 15

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted commit 41035e8e2dbdbfe60c7df3c0b362544393d69190 as:
v1-train: 5e6b4a19a2065e20e3ac6bd8e6c128c6069b44d5
v1.0.1: 7ea1776a1ee281e54cd8f90b7d78a1b99bc4e257
Uplifted commit 2a1e50d4ccdeb4fc752e2ace8e487a5a17d1e113 as:
v1-train: 9ccab5adc0dda9740927a26dc7648e8a98c7745c
v1.0.1: 7c2c08c7257fc20c00af2c533f0f04ac8dae1abb

Comment 16

[Julien Wajsberg [:julienw]]

For some reason, the uplift of 2a1e50d4ccdeb4fc752e2ace8e487a5a17d1e113 left js/metadata_scripts.js which led to messages from git stating that this file changed, even if the file is in .gitignore.

These commits remove this build-generated file in v1.0.1 and v1-train.

v1-train: ace1eb32a313da1232bbdf9cff2581a4b036356d
v1.0.1: 5c3583a7a58f0c47da0e48b90273fa36cc508b0b

Comment 17

[amiller]

No need to create a TC in Moztrap for this issue.

Comment 18

[amiller]

Build ID: 20130329070203 
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/5cc5df16447a
Gaia: 26b463f14caa11e0fc64fda09a17054da4bea68b

Verified as fixed in V1 train. Startup time starts in less than 1 second.