837674 - [gallery] Implement new startup loading events

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Gallery, 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 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.

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

[David Flanagan [:djf]]

Attachment: link to patch on github

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 4

[:Eli Perelman]

Comment on attachment 8438786 [details] [review]
link to patch on github

After addressing David's questions in IRC, I think the patch looks great!

Comment 5

[: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 6

[:Eli Perelman]

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 [1] and [2]. 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 [3]. Could you get this uplifted to 2.0 once it lands?

[1] https://github.com/mozilla-b2g/gaia/blob/04f6d08b66d28849b8e0920b69633ad656c58a8d/tests/performance/startup_events_test.js#L11
[2] https://github.com/mozilla-b2g/gaia/blob/04f6d08b66d28849b8e0920b69633ad656c58a8d/tests/performance/startup_events_test.js#L21
[3] https://wiki.mozilla.org/FirefoxOS/Performance/Release_Acceptance

Comment 7

[Kevin Grandon :kgrandon]

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.

Comment 8

[Hubert Figuiere [:hub]]

I have a few comments as well on the PR.

Comment 9

[David Flanagan [:djf]]

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.

Comment 10

[:Eli Perelman]

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 11

[David Flanagan [:djf]]

Comment on attachment 8438786 [details] [review]
link to patch on github

Cancelling the review request for Jim because Eli reviewed this pretty carefully.

Comment 12

[David Flanagan [:djf]]

Landed to master: https://github.com/mozilla-b2g/gaia/commit/b964ea0bfe6948da16bdd1b5db7ec93593235fa1

Comment 13

[David Flanagan [:djf]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/b908ea0dc97402c8df72e90bc3914bed5cf0f389