Closed Bug 837677 Opened 7 years ago Closed 5 years ago
[e-mail] 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 Email app, it should be triggered when the user can actually manipulate the app. We should measure 2 runs (cold and warm) for this app: - in the first run we'll measure when we arrive to the "set up an account" page and can manipulate it. - in the second run an account should be set up in advance, and we'll measure when the user can manipulate the display of the mails that are already stored locally.
Summary: "ready to use" perf measurement → [e-mail] "ready to use" perf measurement
I am willing to add this for the email app. From what I can tell, it means emitting an event in mail-app.js in "showMessageViewOrSetup" after the final card pushes, and then testing it out to make sure it lines up to the user seeing something, and if not, adjusting the location of the event being emitted to match. Julien, if you think it needs more than this, or if you were going to look into it, please feel free to let me know.
James, I refined some code in Bug 837673 (patch ready in r?) so I'd suggest you take a look there and even |git am| the patch on your local tree so that you can use the shared objects that I made. Basically you can emit different events at different moments if necessary (but if only one is necessary then just emit one of course). The important thing is, as you said, to find the correct moment. For me, it is the moment when the user can start interacting with the app (so for example he can open a mail). In the future this may need refining, with or without network, etc, but for now we can start with a first simple measurement.
also, so that you don't lose time as I did ;) you need a very recent b2g18 tree with the fix to Bug 842841.
Pointer to Github pull-request
Comment on attachment 745230 [details] Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9544 This wires up the PerformanceHelper to email. However it also includes some changes to the performance helper code. The executeAsyncScript code to register the mozPerfHasListener was getting stuck in the event loop after the places in the code where PerformanceTestingHelper.dispatch() was called in the email app. I even saw this occasionally in the communications/contacts app. So I modified shared/js/performance_testing_helper.js to store on to the dispatch data and check periodically for mozPerfHasListener. The dispatch data also uses a time offset relative to navigation.performance.fetchStart, which is closer to when the current app starts loading. Since the event data may not be sent until later, I modified tests/performance/performance_helper_atom.js to use the timestamp in the data instead of calling performance.now() manually. Asking julienw for a review of the performance helper changes, asuth for mail measuring points.
Rik, if you find yourself having some free time, please go ahead and steal this review from me.
Pointer to Github pull-request
Stole the review per comment 6.
Comment on attachment 745230 [details] Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9544 (This patched was mooted by rik's review comment that cited bug 846909 as the right place to address this. This but should potentially be resolved as invalid.)
Bug 996038 introduces new events outlining the phases of application startup. Each of these 5 events needs to be implemented.
Summary: [e-mail] "ready to use" perf measurement → [e-mail] Implement new startup loading events
Whiteboard: [c=automation p= s= u=] → [c=automation p= s= u=][priority]
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
Pull request, asking :mcav since :asuth will be in an afk window soon. Also asking :Eli for feedback, although he will also be afk for a bit, so will not hold up the pull request if r+ lands while in the afk window.
Comment on attachment 8439582 [details] [review] GitHub pull request Quite the PR, and everything is on the up-and-up for the performance side. Looks good.
Attachment #8439582 - Flags: feedback?(eperelman) → feedback+
Comment on attachment 8439582 [details] [review] GitHub pull request Looks good, the comments are clear.
Attachment #8439582 - Flags: review?(m) → review+
Merged in master: https://github.com/mozilla-b2g/gaia/commit/f93e6fa38b3f328df82a493406c8ec52a890c2c5 from pull request: https://github.com/mozilla-b2g/gaia/pull/20464
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
James, could you mark your patch for uplift to 2.0? 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
Asking for 2.0? as per comment 16. It may not uplift cleanly via automation, due to a possible conflict in compose.js, but if so, just ping me and I will manually prep the patch.
blocking-b2g: --- → 2.0?
(In reply to James Burke [:jrburke] from comment #17) > Asking for 2.0? as per comment 16. It may not uplift cleanly via automation, > due to a possible conflict in compose.js, but if so, just ping me and I will > manually prep the patch. This should go through approval, it seems like we are trying to add new measurements to better get perf results, not something we would block a release on. Feel free to request approval-gaia-v2.0 on this.
blocking-b2g: 2.0? → -
Comment on attachment 8439582 [details] [review] GitHub pull request Filing on behalf of :Eli, but if there are follow up questions on the importance of this landing in 2.0, :Eli should field those questions. [Bug caused by] (feature/regressing bug #): none, feature request by the performance team. See comment #16, measuring startup events part of acceptance criteria. [User impact] if declined: none [Testing completed]: Running the perf commands to collect the metrics. [Risk to taking this patch] (and alternatives if risky): Low, just emits some events that are tracked by performance tools. [String changes made]: none
Attachment #8439582 - Flags: approval-gaia-v2.0?
Attachment #8439582 - Flags: approval-gaia-v2.0? → approval-gaia-v2.0+
You need to log in before you can comment on or make changes to this bug.