837666 - [sms] "ready to use" perf measurement

Status: RESOLVED FIXED

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

Comment 1

[Borja Salguero [:borjasalguero]]

Hi Julien! As we are using an 'async' way for rendering the threads, probably 'ready to use' should be dispatched when we have the first thread rendered, or a set of threads. Wdyt?

Comment 2

[Julien Wajsberg [:julienw]]

We probably need several events: one when the user can scroll, one when the first thread is displayed and one when all the threads are displayed.

But you should probably wait until Bug 837673 is landed (Hopefully today...)

Comment 3

[Caio Lima(:caiolima)]

Julien, this idea is still alive? I gess I can work on it.

Comment 4

[Julien Wajsberg [:julienw]]

It's still alive but the perf framework is broken right now...

Comment 5

[Julien Wajsberg [:julienw]]

Attachment: github PR

To check this runs fine, you first need a Marionette-enabled build (look at [1]), plug your device, and then run:

  APP=sms RUNS=1 MARIONETTE_RUNNER_HOST=marionette-device-host make test-perf

You'll see that unfortunately we don't catch the events that happens before the "load" event, but I expect it to work correctly once bug 846909 is fixed.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Platform/Testing/Gaia_performance_tests

---
 .../contacts/test/unit/mock_performance_testing_helper.js     |  5 -----
 apps/communications/contacts/test/unit/views/list_test.js     |  3 +--
 apps/sms/index.html                                           |  1 +
 apps/sms/js/startup.js                                        |  5 ++++-
 apps/sms/js/thread_list_ui.js                                 | 11 ++++++++++-
 apps/sms/test/unit/thread_list_ui_test.js                     |  4 +++-
 build/jshint/xfail.list                                       |  1 -
 shared/test/unit/mocks/mock_performance_testing_helper.js     |  6 ++++++
 tests/performance/startup_events_test.js                      |  2 +-
 9 files changed, 26 insertions(+), 12 deletions(-)
 delete mode 100644 apps/communications/contacts/test/unit/mock_performance_testing_helper.js
 create mode 100644 shared/test/unit/mocks/mock_performance_testing_helper.js

Comment 6

[Steve Chung [:steveck]]

Attachment: myfile.json

I can't see any timing related measurement result and it always shows bounch of error messages... :(  An AssertionError with 'empty results' message in startup_events_test.js:60 might be noticeable. Is there anything I might missed for setup?

Comment 7

[Steve Chung [:steveck]]

I still unable to trigger the test successfully even flash the latest eng build and disable the remote debugger... :(  Here is the error log in console:
(stop) stderr: error: cannot bind socket
Not enough fields given the number of keys.

About the 'marionette.defaultPrefs.enabled' key, I add this in preferance.js prefs.push(['marionette.defaultPrefs.enabled', true]);
Is this the proper way to enable the pref test?

Comment 8

[Gabriele Svelto [:gsvelto]]

(In reply to Steve Chung [:steveck] from comment #7)
> I still unable to trigger the test successfully even flash the latest eng
> build and disable the remote debugger... :(  Here is the error log in
> console:
> (stop) stderr: error: cannot bind socket

I think that error message is coming from the adb logic, check if you can manually forward ports correctly.

Comment 9

[Julien Wajsberg [:julienw]]

You can try to use my "addpref" script from https://github.com/julienw/config-files/blob/master/addpref and run "addpref marionette" :)

The error you have is from https://github.com/mozilla-b2g/gaia/blob/master/tests/performance/meminfo.js#L36 which is quite strange. Can you run "adb shell b2g-ps" properly ?

Comment 10

[Julien Wajsberg [:julienw]]

So, I've seen today that the perf framework may be broken because of the recent system changes... Can you try to flash a system app from before the Lockscreen change (bug 960901)?

Comment 11

[Steve Chung [:steveck]]

Attachment: result after bug 960901 revert

That's the result after reverting the bug 960901 and the failure seems gone. mozPerfDurations exist now, but I'm not sure it's the correct result because you dispatch many events to PerformanceTestingHelper. There seems not many information as I expected(like timestamp for "will-render-threads"?)

Comment 12

[Hubert Figuiere [:hub]]

(In reply to Steve Chung [:steveck] from comment #7)
> I still unable to trigger the test successfully even flash the latest eng
> build and disable the remote debugger... :(  Here is the error log in
> console:
> (stop) stderr: error: cannot bind socket
> Not enough fields given the number of keys.

The "cannot bind socket" error occur AFTER the test has been run. I don't get it here but I have seen it happen on the jenkins jobs.

> Not enough fields given the number of keys.

Do you run it on Buri?
I have seen that one too, and it is harmless as well.

Comment 13

[Hubert Figuiere [:hub]]

(In reply to Julien Wajsberg [:julienw] from comment #10)
> So, I've seen today that the perf framework may be broken because of the
> recent system changes... Can you try to flash a system app from before the
> Lockscreen change (bug 960901)?

I haven't had issues at all with that either.

Comment 14

[Julien Wajsberg [:julienw]]

(In reply to Hubert Figuiere [:hub] from comment #13)
> (In reply to Julien Wajsberg [:julienw] from comment #10)
> > So, I've seen today that the perf framework may be broken because of the
> > recent system changes... Can you try to flash a system app from before the
> > Lockscreen change (bug 960901)?
> 
> I haven't had issues at all with that either.

Basically you need a sync-ed device with your gaia tree. You need either a gaia + system-on-device from before that bug, or from after that bug, but the perf framework from after the bug doesn't work with a system app from before the bug.

(In reply to Steve Chung [:steveck] from comment #11)
> Created attachment 8383143 [details]
> result after bug 960901 revert
> 
> That's the result after reverting the bug 960901 and the failure seems gone.
> mozPerfDurations exist now, but I'm not sure it's the correct result because
> you dispatch many events to PerformanceTestingHelper. There seems not many
> information as I expected(like timestamp for "will-render-threads"?)

Yep, that's what I said, we need bug 846909 to have all the earliest events... currently we get only the latest ones, that is the "above the fold" and the "startup path done" events.

Comment 15

[Steve Chung [:steveck]]

(In reply to Hubert Figuiere [:hub] from comment #13)
> Do you run it on Buri?
> I have seen that one too, and it is harmless as well.

Yes I ran it on Buri, and I also think it's harmless.

(In reply to Julien Wajsberg [:julienw] from comment #14)
Yep, that's what I said, we need bug 846909 to have all the earliest events... currently we get only the latest ones, that is the "above the fold" and the "startup path done" events.

Oh yes I saw it in comment 5, thanks !

Comment 16

[Steve Chung [:steveck]]

Comment on attachment 8381469 [details] [review]
github PR

Except the comments from Hubert and me, please remember fix the major test error(seems we also need mock PerformanceTestingHelper in sms_test.js) before landing, thanks!

Comment 17

[Julien Wajsberg [:julienw]]

Yes, will do, thanks !

Comment 18

[Julien Wajsberg [:julienw]]

Comment on attachment 8381469 [details] [review]
github PR

Fixed the unit test.

I also tried to look up why it was failing locally when run separately but I couldn't find the answer yet. Possibly related to drafts.

Comment 19

[Steve Chung [:steveck]]

Comment on attachment 8381469 [details] [review]
github PR

Looks good and I think existing error in Travis should not related to this patch. Thanks!

Comment 20

[Julien Wajsberg [:julienw]]

Restarted Travis and it was Green :)

master: 9e121be67b1a415fbe646b1fd8999804b48daae8