[sms] "ready to use" perf measurement

RESOLVED FIXED

Status

P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: julienw, Assigned: julienw)

Tracking

({perf})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [c=automation p= s=2014.03.14 u=])

Attachments

(3 attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Blocks: 837633
No longer depends on: 837633
(Assignee)

Updated

6 years ago
Summary: "ready to use" perf measurement → [sms] "ready to use" perf measurement
(Assignee)

Updated

6 years ago
Depends on: 806598
(Assignee)

Updated

6 years ago
Depends on: 837139
(Assignee)

Updated

6 years ago
Assignee: nobody → felash
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?
(Assignee)

Comment 2

6 years ago
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...)
(Assignee)

Updated

5 years ago
Blocks: 888135

Updated

5 years ago
Keywords: perf
Whiteboard: [c=instrumentation p=]
Priority: -- → P2
(Assignee)

Updated

5 years ago
Blocks: 914910

Updated

5 years ago
Whiteboard: [c=instrumentation p=] → [c=automation p= s= u=]

Comment 3

5 years ago
Julien, this idea is still alive? I gess I can work on it.
(Assignee)

Comment 4

5 years ago
It's still alive but the perf framework is broken right now...
(Assignee)

Comment 5

5 years ago
Created attachment 8381469 [details] [review]
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
Attachment #8381469 - Flags: review?(schung)
Created attachment 8381905 [details]
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?

Updated

5 years ago
Flags: needinfo?(felash)
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?
(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.
(Assignee)

Comment 9

5 years ago
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 ?
Flags: needinfo?(felash)
(Assignee)

Comment 10

5 years ago
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)?
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"?)
(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.
(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.
(Assignee)

Comment 14

5 years ago
(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.
(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 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!
Attachment #8381469 - Flags: review?(schung)
(Assignee)

Comment 17

5 years ago
Yes, will do, thanks !
(Assignee)

Comment 18

5 years ago
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.
Attachment #8381469 - Flags: review?(schung)
Comment on attachment 8381469 [details] [review]
github PR

Looks good and I think existing error in Travis should not related to this patch. Thanks!
Attachment #8381469 - Flags: review?(schung) → review+
(Assignee)

Comment 20

5 years ago
Restarted Travis and it was Green :)

master: 9e121be67b1a415fbe646b1fd8999804b48daae8
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Whiteboard: [c=automation p= s= u=] → [c=automation p= s=2014.03.14 u=]
You need to log in before you can comment on or make changes to this bug.