Closed Bug 837674 Opened 7 years ago Closed 6 years ago
[gallery] Implement new startup loading events
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 Gallery, it should be triggered when the list can be scrolled. 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.
Summary: "ready to use" perf measurement → [gallery] "ready to use" perf measurement
Bug 996038 introduces new events outlining the phases of application startup. Each of these 5 events needs to be implemented.
Summary: [gallery] "ready to use" perf measurement → [gallery] Implement new startup loading events
Adding to sprint4 for discussion during planning
Target Milestone: --- → 2.0 S4 (20june)
Jim: I'm asking for your review since you're doing the Music app and I imagine the mediadb startup issues will be similar for you. Eli, Could you take a look and tell me if this is what you have in mind? Note that I'm loading performance-testing-helper.js with defer because I don't have any non-deferred scripts in this app and I think it would impact startup time to add one. I've also got three specific questions for you: 1) Every time a media app starts up, it scans the entire sdcard looking for new media files, and then scans again to make sure that all of the files it already knows about are still there. Even if nothing has changed on the card, this process can take 10 or 20 seconds. It doesn't seem right to me to wait that long before sending moz-app-loaded. Is that okay? Would you consider adding another moz-app-stable event to handle the case of long-running background tasks like this? 2) Can I just call PerformanceTestingHelper.dispatch('moz-app-loaded') instead of creating and dispatching an event? 3) Would you consider adding (or would you allow me to add) methods like PerformanceTestingHelper.mozAppLoaded() (or even performance.mozAppLoaded()) to simplify adding this instrumentation and to minimize the dependency on typing these string constants correctly?
Comment on attachment 8438786 [details] [review] link to patch on github After addressing David's questions in IRC, I think the patch looks great!
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 on attachment 8438786 [details] [review] link to patch on github David, I am switching back to r? as I hadn't noticed that we are missing the whitelist entry at  and . If you could revise to add that, that would be most appreciated. Also, this implementation needs to land in 2.0 as it is important for us to determine whether we are meeting release performance acceptance criteria . Could you get this uplifted to 2.0 once it lands?  https://github.com/mozilla-b2g/gaia/blob/04f6d08b66d28849b8e0920b69633ad656c58a8d/tests/performance/startup_events_test.js#L11  https://github.com/mozilla-b2g/gaia/blob/04f6d08b66d28849b8e0920b69633ad656c58a8d/tests/performance/startup_events_test.js#L21  https://wiki.mozilla.org/FirefoxOS/Performance/Release_Acceptance
Attachment #8438786 - Flags: review+ → review?(eperelman)
We can request approval for 2.0 after it lands, but at this point unless we're considering these issues blocking, it's unlikely you're going to get these events into 2.0 for all apps.
I have a few comments as well on the PR.
Hub: I've addressed your comment. Eli: I've added the changes you asked for to the startup events test. And yes, we plan to uplift this to 2.0. Adding these events to the media apps is a 2.0 feature we've committed to. It just didn't make it into the first 3 sprints, so we'll have to uplift. Eli: this is ready for your review again. Since the r? flag is already set, though, I'll use needinfo to get your attention.
Flags: needinfo?(dflanagan) → needinfo?(eperelman)
Comment on attachment 8438786 [details] [review] link to patch on github Thanks for the update David, and sorry for the confusion. Everything seems to be in order.
Comment on attachment 8438786 [details] [review] link to patch on github Cancelling the review request for Jim because Eli reviewed this pretty carefully.
Landed to master: https://github.com/mozilla-b2g/gaia/commit/b964ea0bfe6948da16bdd1b5db7ec93593235fa1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Comment on attachment 8438786 [details] [review] link to patch on github NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] [Bug caused by] (feature/regressing bug #): [User impact] if declined: none, but the perf team will be mad [Testing completed]: performance team will presumably be testing it [Risk to taking this patch] (and alternatives if risky): not risky. The patch just emits harmless events [String changes made]: none
Attachment #8438786 - Flags: approval-gaia-v2.0?(hkoka)
Whiteboard: [c=automation p= s= u=] → [c=automation p= s=2014.06.20.t u=]
Attachment #8438786 - Flags: approval-gaia-v2.0?(hkoka) → approval-gaia-v2.0+
You need to log in before you can comment on or make changes to this bug.