837675 - [music] Implement new startup loading events

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

We need to measure when the app is usable by the user. For that we'll need to send an event (the moment is specific to the app) to |window| that the performance test will be able to receive.

The event name can be "x-moz-perf-user-ready" amongst all apps so that the performance test can be similar.

For the Music app, it should be triggered when the user can actually manipulate the app.

We should measure 2 runs (cold and warm) for this app: the first run will populate the internal DB, the second run will directly load the internal DB and should be faster.

Comment 1

[:Eli Perelman]

Bug 996038 introduces new events outlining the phases of application startup. Each of these 5 events needs to be implemented.

Comment 2

[Hema Koka [:hema]]

Adding to sprint4 for discussion during planning

Comment 3

[Jim Porter (:squib)]

Taking.

Comment 4

[:Eli Perelman]

As an FYI, this implementation needs to land in 2.0 as it is important for meeting release performance acceptance criteria.

https://wiki.mozilla.org/FirefoxOS/Performance/Release_Acceptance

Comment 5

[Hema Koka [:hema]]

Jim, any update on this? If you are busy with ringtone blockers, NI dominic to see if he can take it

Thanks
Hema

Comment 6

[Jim Porter (:squib)]

I think I'm done with ringtones now (at least, I really hope so), so I can start looking at this.

Comment 7

[Jim Porter (:squib)]

Attachment: Add events

This is still WIP (I haven't even run with it yet), but I'm putting this up for now while I look at it more.

Comment 8

[Jim Porter (:squib)]

Comment on attachment 8446721 [details] [review]
Add events

I think this is right...

Comment 9

[:Eli Perelman]

Comment on attachment 8446721 [details] [review]
Add events

Looks good on the perf side.

Comment 10

[David Flanagan [:djf]]

Comment on attachment 8446721 [details] [review]
Add events

I think this patch sends the visually-complete and content-interactive events at the wrong time: it sends them during the sdcard scanning process instead of the db enumeration process.

Comment 11

[Hema Koka [:hema]]

Jim,

Lets get the review comments addressed and get this into 2.1

Thanks
Hema

Comment 12

[:Eli Perelman]

Any updates on the progress of this?

Comment 13

[Jim Porter (:squib)]

I had reviews to catch up on, but I should be able to finish this up tomorrow.

Comment 14

[Jim Porter (:squib)]

Comment on attachment 8446721 [details] [review]
Add events

Ok, does this look better? I'm not 100% sure I moved the perf events to the right spot, but I *think* I got it right.

Note that I haven't run with this version yet, but I'll definitely do so before landing.

Comment 15

[:Eli Perelman]

Comment on attachment 8446721 [details] [review]
Add events

Meets all the requirements of the performance side.

Comment 16

[David Flanagan [:djf]]

Comment on attachment 8446721 [details] [review]
Add events

r+, but consider addressing the comments I left on github.

Comment 17

[Hema Koka [:hema]]

Jim, can you get this landed into master for 2.1

Comment 18

[:Eli Perelman]

Any updates on being able to get this landed?

Comment 19

[Hubert Figuiere [:hub]]

The patch need to be rebased now with global config stuff from bug 1049031

Comment 20

[:Eli Perelman]

Hub is correct, the patch is good as-is for v2.0, but for master the whitelist has been moved to:

https://github.com/mozilla-b2g/gaia/blob/master/tests/performance/config.json#L23

Comment 21

[Jim Porter (:squib)]

Ok, this should be ready to land. I'm just waiting on Try.

Comment 22

[Hema Koka [:hema]]

(In reply to :Eli Perelman from comment #20)
> Hub is correct, the patch is good as-is for v2.0, but for master the
> whitelist has been moved to:
> 
> https://github.com/mozilla-b2g/gaia/blob/master/tests/performance/config.
> json#L23

Mike/Eli

Can we get this in for 2.1 for music? We are trying to limit code changes at this point for 2.0 given we already have several critical issues to deal with. But if this is absolutely blocking instrumentation measurements for 2.0 comparing previous releases (in other words: identifying perf regressions), we can make a call.

Thanks
Hema

Comment 23

[:Eli Perelman]

Clearing the ni? as I can't make that decision, but I think it's a judgement call. If you want to identify regressions from 2.0 to 2.1, you'll need to take this change into 2.0. If you find that a low priority for the Music app, then you can forgo having it in 2.0.

Comment 24

[Mike Lee [:mlee]]

This work will enable more accurate automated monitoring of Music App Launch Performance. As Eli says in comment 23, landing this is a decision for the fxOS 2.0 Music App and Release Management teams.

Dashboard Metrics for all apps currently tracked for Performance Criteria/Signoff can be found here: https://wiki.mozilla.org/FirefoxOS/Performance/Release_Acceptance#2.0

Comment 25

[:Eli Perelman]

Jim, a Try run never happened for this latest push. Could you re-submit?

Comment 26

[Hema Koka [:hema]]

I can approve this for 2.0 for more accurate measurements and identifying regressions between 2.0 and 2.1. Jim, please work on wrapping this up. 

NI Inder as FYI for this landing into 2.0 (as he requested)

Comment 27

[:Eli Perelman]

Just a reminder that the current patch won't apply cleanly to v2.0. You'll need to add "music" to the two whitelist arrays at the top of tests/performance/startup_events_test.js in the v2.0 branch.

Comment 28

[Jim Porter (:squib)]

Landed. I'll make a 2.0 patch tomorrow.