837669 - [settings] "ready to use" perf measurement

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Settings, 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 Settings app, it should be triggered when the first panel has been displayed and is scrollable/actionnable by the user.

Comment 1

[Julien Wajsberg [:julienw]]

Hey Hubert,

The performance framework is broken these days, I filed Bug 912541 to fix it, and there is even a preliminary patch that make at least it run, even if issues still exist.

I discussed this with James Lal and we agreed the current performance framework needs to be ported to the new marionette runner, that is used with the integraiton tests, and we planned to look this next week.

You're more than welcome to help here !

Comment 2

[Hubert Figuiere [:hub]]

I'll definitely look into that with you next week (in Oslo).

Comment 3

[Hubert Figuiere [:hub]]

I think I need to fix bug 980978 first.

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

It seems to fit into my proposal: https://groups.google.com/forum/#!topic/mozilla.dev.gaia/OBiGuysU_uM

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

Is it a dupe of bug 996038?

Comment 6

[Julien Wajsberg [:julienw]]

Nope, this is the but to add the various events for the "Settings" application

Comment 7

[Hubert Figuiere [:hub]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19331

Reviewers: :julienw for test-perf, :kaze for settings.

Comment 8

[Mike Lee [:mlee]]

Hub & Eli,

Julien's comment 0 seems to indicate this has some relation to the "Interaction Ready" work proposed by bug 996043:

> For the Settings app, it should be triggered when the first panel has
> been displayed and is scrollable/actionnable by the user.

Eli's broader work in bug 996038 should encompass this. Please clarify the difference between this bug's work and the work proposed by bug 996038 and bug 996043.

Are all of the "Ready to use" bugs under bug 837633 really just "Interaction Ready" events that fit within Eli's launch states events?


Thanks,
Mike

Comment 9

[Julien Wajsberg [:julienw]]

Comment on attachment 8423987 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19331

r=me for the performance test part, given that you tested it yourself.

Redirecting the Settings review to Evelyn, Fabien sadly does not work on Settings anymore.

Comment 10

[Julien Wajsberg [:julienw]]

mlee: on a broader point of view, I'd be glad if someone from your team (Eli or someone) could progressively do the reviews for the performance framework. I could still do feedbacks, especially in the first weeks, but it could be great that your team eventually takes this task :)

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Julien Wajsberg [:julienw] from comment #6)
> Nope, this is the but to add the various events for the "Settings"
> application

So, why do you want to add "Settings" specific events that will be matching the ones Eli is designing for all apps in bug 996038?

Comment 12

[Hubert Figuiere [:hub]]

(In reply to Zibi Braniecki [:gandalf] from comment #11)
> > Nope, this is the but to add the various events for the "Settings"
> > application
> 
> So, why do you want to add "Settings" specific events that will be matching
> the ones Eli is designing for all apps in bug 996038?

It is not settings specific. It is the same as the other apps that do it. Just that we need to send it at one point in the app, and that's what we are doing here. Ideally all the app in Gaia would use an "application kit", but nope. We don't even have a standard module loader and duplicate RequireJS (alameda) in several places

% find apps -name alameda.js
apps/clock/js/alameda.js
apps/email/js/alameda.js
apps/camera/js/vendor/alameda.js
apps/settings/js/vendor/alameda.js
%

Will file a bug for that ASAP btw.

Comment 13

[Hubert Figuiere [:hub]]

(In reply to Mike Lee [:mlee] from comment #8)

> Eli's broader work in bug 996038 should encompass this. Please clarify the
> difference between this bug's work and the work proposed by bug 996038 and
> bug 996043.
>

This is for bug 996043 IMHO.

Comment 14

[Hubert Figuiere [:hub]]

(In reply to Hubert Figuiere [:hub] from comment #12)

> % find apps -name alameda.js
> apps/clock/js/alameda.js
> apps/email/js/alameda.js
> apps/camera/js/vendor/alameda.js
> apps/settings/js/vendor/alameda.js
> %
> 
> Will file a bug for that ASAP btw.

It is actually bug 958856

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

So, it seems that we're coming with a lot of events that are overlapping. My understanding was that bug 996038 aims at coming with a set of event states and their names that should be used across all apps.

'startup-path-done' seems to be a dupe of event 'app-ready' or 'entire-ready' (depending on what Eli will settle on).

Comment 16

[Hubert Figuiere [:hub]]

(In reply to Zibi Braniecki [:gandalf] from comment #15)
> So, it seems that we're coming with a lot of events that are overlapping. My
> understanding was that bug 996038 aims at coming with a set of event states
> and their names that should be used across all apps.
> 
> 'startup-path-done' seems to be a dupe of event 'app-ready' or
> 'entire-ready' (depending on what Eli will settle on).

Given that this hasn't be settled yet....

I have no problem changing them all when we actually settle on it. In the mean time, perfect is the enemy of good. That patch had already been bitrotting for a while.

Comment 17

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Hubert Figuiere [:hub] from comment #16)
> Given that this hasn't be settled yet....
> 
> I have no problem changing them all when we actually settle on it. 

Oh, good. Ok, that's what I was trying to verify.

I agree we can change the names once we settle on the scheme.

Comment 18

[Julien Wajsberg [:julienw]]

'startup-path-done' is just a name we picked up more than 1 year ago for some apps, most notably Contacts and Settings. [1] is the commit which introduced that name. So it's not new, it's just that only some apps used it, mainly by lack of time.

[1] https://github.com/mozilla-b2g/gaia/commit/c24074cc9be379334fecc5b83302d5adbc7b2806

Now, I really don't care which name we finally choose. This is bikeshedding. I want the functionality: properly monitor what apps are doing when they're launched, and possibly play scenario using marionette JS while monitoring the apps.

Comment 19

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Julien Wajsberg [:julienw] from comment #18)
> Now, I really don't care which name we finally choose. This is bikeshedding.
> I want the functionality: properly monitor what apps are doing when they're
> launched, and possibly play scenario using marionette JS while monitoring
> the apps.

It is not if we try to systematize the launch stages. Name is of course of secondary importance, but the exact conditions under which the event is fired (a'ka what it *really* means) is important to be able to measure the same state and catch regressions that affect the same state across multiple apps.

That's why I was trying to verify if the event here is one of the bootstrap steps designed by Eli, or is it something entirely different. I'm happy with the answer, we can adjust once we solve that one.

Comment 20

[:Eli Perelman]

(In reply to Mike Lee [:mlee] from comment #8)
> Eli's broader work in bug 996038 should encompass this. Please clarify the
> difference between this bug's work and the work proposed by bug 996038 and
> bug 996043.

The work on this particular bug should be covered by the planned implementations from 996038. The difference between this bug and 996038 is that 996038 will be tracking for all core shared startup events in all applications while this one specifically covers the settings application.

(In reply to Mike Lee [:mlee] from comment #8)
> Are all of the "Ready to use" bugs under bug 837633 really just "Interaction
> Ready" events that fit within Eli's launch states events?

No, in glancing at some of the bugs in 837663, many of the events are custom events for an individual application for measuring a specific metric that is not generic to all applications. The goal of 996038 is to only generate the application startup events that will be generic to all applications.

Comment 21

[:Eli Perelman]

(In reply to Julien Wajsberg [:julienw] from comment #18)
> 'startup-path-done' is just a name we picked up more than 1 year ago for
> some apps, most notably Contacts and Settings. [1] is the commit which
> introduced that name. So it's not new, it's just that only some apps used
> it, mainly by lack of time.

The use of this event after the implementation of 996038 will be up to the application owners to determine. If they feel that 'startup-path-done' represents a different data point than the 'app-ready' event, then it would make sense to keep both.

> Now, I really don't care which name we finally choose. This is bikeshedding.
> I want the functionality: properly monitor what apps are doing when they're
> launched, and possibly play scenario using marionette JS while monitoring
> the apps.

+1, this is the goal of 996038, for this to be possible and standardized across all our applications.

Comment 22

[:Eli Perelman]

(In reply to Julien Wajsberg [:julienw] from comment #10)
> mlee: on a broader point of view, I'd be glad if someone from your team (Eli
> or someone) could progressively do the reviews for the performance
> framework. I could still do feedbacks, especially in the first weeks, but it
> could be great that your team eventually takes this task :)

The route we are planning on taking is that I will perform one of the implementations for an application (quite possibly the settings after this conversation), and then work with the owners of the other applications to refactor to be able to use the new events based on the pattern that we establish.

In any case, I'd be happy to give reviews or feedback where needed.

Comment 23

[Julien Wajsberg [:julienw]]

(In reply to Eli Perelman, :Eli from comment #20)

> No, in glancing at some of the bugs in 837663, many of the events are custom
> events for an individual application for measuring a specific metric that is
> not generic to all applications. The goal of 996038 is to only generate the
> application startup events that will be generic to all applications.

If you look carefully, there is a bug per app for the startup scenario too, some of them are even resolved already! I commented in bug 996038 because I don't want that you redo work that is already done.

Comment 24

[Evelyn Hung]

Comment on attachment 8423987 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19331

I'm fine with the event name after reading the long discussion. ;p Thanks for the patch!

Comment 25

[Evelyn Hung]

merged into master
https://github.com/mozilla-b2g/gaia/commit/3bab8459a6b21923ab2fac2f1633bceb1f2a5a93

Comment 26

[Hubert Figuiere [:hub]]

I need to do a followup patch.

Comment 27

[Hubert Figuiere [:hub]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19486

Comment 28

[:Eli Perelman]

Comment on attachment 8426345 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19486

+1 from me. Curious though what effect it would have if we converted negative values to 0 and pushed those instead of ignoring to push them?

Comment 29

[Hubert Figuiere [:hub]]

Actually values are negative because said event occur BEFORE the start event. In the settings app for example, the wifi list rendering  start long after the startup-path-done.

It is really filtering out noise in that case.

Comment 30

[:Eli Perelman]

Ah right, thanks for the clarification.

Comment 31

[Hubert Figuiere [:hub]]

Merged
https://github.com/mozilla-b2g/gaia/commit/88adffe4c64efb384ebdca13d4ef00ca5da718ca