Closed Bug 837675 Opened 7 years ago Closed 5 years ago

[music] Implement new startup loading events

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S2 (15aug)

People

(Reporter: julienw, Assigned: squib)

References

Details

(Keywords: perf, Whiteboard: [c=automation p= s= u=])

Attachments

(1 file)

46 bytes, text/x-github-pull-request
djf
: review+
Eli
: review+
Details | Review
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.
No longer depends on: gaia-perf-measure
Summary: "ready to use" perf measurement → [music] "ready to use" perf measurement
Depends on: 837139
Keywords: perf
Whiteboard: [c=instrumentation p=]
Priority: -- → P2
Whiteboard: [c=instrumentation p=] → [c=automation p= s= u=]
Blocks: 996043
Bug 996038 introduces new events outlining the phases of application startup. Each of these 5 events needs to be implemented.
Summary: [music] "ready to use" perf measurement → [music] Implement new startup loading events
Adding to sprint4 for discussion during planning
Target Milestone: --- → 2.0 S4 (20june)
Taking.
Assignee: nobody → squibblyflabbetydoo
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
Jim, any update on this? If you are busy with ringtone blockers, NI dominic to see if he can take it

Thanks
Hema
Flags: needinfo?(squibblyflabbetydoo)
Flags: needinfo?(dkuo)
Target Milestone: 2.0 S4 (20june) → 2.0 S5 (4july)
I think I'm done with ringtones now (at least, I really hope so), so I can start looking at this.
Flags: needinfo?(squibblyflabbetydoo)
Attached file 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 on attachment 8446721 [details] [review]
Add events

I think this is right...
Attachment #8446721 - Flags: review?(eperelman)
Attachment #8446721 - Flags: review?(dflanagan)
Comment on attachment 8446721 [details] [review]
Add events

Looks good on the perf side.
Attachment #8446721 - Flags: review?(eperelman) → review+
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.
Attachment #8446721 - Flags: review?(dflanagan) → review-
Status: NEW → ASSIGNED
Target Milestone: 2.0 S5 (4july) → 2.0 S6 (18july)
Jim,

Lets get the review comments addressed and get this into 2.1

Thanks
Hema
Target Milestone: 2.0 S6 (18july) → 2.1 S1 (1aug)
Flags: needinfo?(dkuo)
Any updates on the progress of this?
I had reviews to catch up on, but I should be able to finish this up tomorrow.
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.
Attachment #8446721 - Flags: review?(eperelman)
Attachment #8446721 - Flags: review?(dflanagan)
Attachment #8446721 - Flags: review-
Attachment #8446721 - Flags: review+
Comment on attachment 8446721 [details] [review]
Add events

Meets all the requirements of the performance side.
Attachment #8446721 - Flags: review?(eperelman) → review+
Comment on attachment 8446721 [details] [review]
Add events

r+, but consider addressing the comments I left on github.
Attachment #8446721 - Flags: review?(dflanagan) → review+
Jim, can you get this landed into master for 2.1
Target Milestone: 2.1 S1 (1aug) → 2.1 S2 (15aug)
Any updates on being able to get this landed?
Flags: needinfo?(squibblyflabbetydoo)
Flags: needinfo?(hkoka)
The patch need to be rebased now with global config stuff from bug 1049031
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
Ok, this should be ready to land. I'm just waiting on Try.
Flags: needinfo?(squibblyflabbetydoo)
(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
Flags: needinfo?(mlee)
Flags: needinfo?(hkoka)
Flags: needinfo?(eperelman)
Flags: needinfo?(bbajaj)
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.
Flags: needinfo?(eperelman)
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
Flags: needinfo?(mlee)
Jim, a Try run never happened for this latest push. Could you re-submit?
Flags: needinfo?(squibblyflabbetydoo)
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)
Flags: needinfo?(bbajaj) → needinfo?(ikumar)
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.
Landed. I'll make a 2.0 patch tomorrow.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: needinfo?(squibblyflabbetydoo) → in-testsuite+
Resolution: --- → FIXED
Flags: needinfo?(ikumar)
Duplicate of this bug: 922610
You need to log in before you can comment on or make changes to this bug.