Make VR latency results compatible with perf dashboard, refactor into classes

Make VR latency results compatible with perf dashboard, refactor into classes

Makes the following changes to the automated VR latency testing script:
- Adds the functionality to save latency and correlation results in a format
    compatible with the Chrome performance dashboard
- Refactors the code into separate classes and files for improved readability
    and to better support future Windows latency testing
- Runs the test multiple times (default 10) to get an average and standard deviation

BUG=708747
Review-Url: https://codereview.chromium.org/2823883003
Cr-Commit-Position: refs/heads/master@{#466147}
Committed: https://chromium.googlesource.com/chromium/src/+/a15848198deb716d77753d297355bac34cd33b96

[bsheedy, 2017-04-17 17:21:47]

bsheedy@chromium.org changed reviewers:
+ leilei@chromium.org

[bsheedy, 2017-04-17 17:21:47]

+leilei@ for initial review before adding OWNERS

[Lei Lei, 2017-04-17 22:57:13]

https://codereview.chromium.org/2823883003/diff/40001/chrome/test/vr/perf/lat...
File chrome/test/vr/perf/latency/BUILD.gn (right):

https://codereview.chromium.org/2823883003/diff/40001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/BUILD.gn:13: "./webvr_latency_test.py",
You can add all the files in one folder as data, but I am not sure how to add
current folder, maybe
data = [
  "./",
]

See https://cs.chromium.org/chromium/src/chrome/test/media_router/BUILD.gn?l=10

https://codereview.chromium.org/2823883003/diff/40001/chrome/test/vr/perf/lat...
File chrome/test/vr/perf/latency/android_webvr_latency_test.py (right):

https://codereview.chromium.org/2823883003/diff/40001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/android_webvr_latency_test.py:43:
'https://weableandbob.github.io/Motopho/flicker_apps/webvr/webvr-flicker-app-klaus.html?polyfill=0\&canvasClickPresents=1'])
This line is too long, can you split it into multiple lines?

https://codereview.chromium.org/2823883003/diff/40001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/android_webvr_latency_test.py:48: str(height/2)])
Is there any way to verify if Chrome is in VR mode just in case tapping doesn't
work?

https://codereview.chromium.org/2823883003/diff/40001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/android_webvr_latency_test.py:64: # TODO(bsheedy):
Maybe migrate to use Devil (overkill?)
I think it is better to migrate to use Devil, so we don't need to update adb
binary to the latest version manually.

https://codereview.chromium.org/2823883003/diff/40001/chrome/test/vr/perf/lat...
File chrome/test/vr/perf/latency/webvr_latency_test.py (right):

https://codereview.chromium.org/2823883003/diff/40001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/webvr_latency_test.py:20: """Find all devices
connected to tty that match a pattern and device id.
s/Find/Finds

https://codereview.chromium.org/2823883003/diff/40001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/webvr_latency_test.py:155: self.device_name: {
I thought we got agreement to use bot name for device name, how about
constructing the charts json data as following to keep consistent with other
perf data from telemetry tests?

'charts' : {
  'correlation': {
    '<flicker app url>': {
       .....
    }
    'summary' : {
      same as above
    }
  }
  'latency' : {
     '<flicker app url>': {
       .....
    }
    'summary' : {
      same as above
    }
  }
}

Moreover, can we run latency test multiple times and generate average value and
std deviation? Similar to
https://omnibot-isolate-server.appspot.com/browse?namespace=default-gzip&dige...

[bsheedy, 2017-04-18 20:21:42]

Description was changed from

==========
Make VR latency results compatible with perf dashboard, refactor into classes

Makes the following changes to the automated VR latency testing script:
- Adds the functionality to save latency and correlation results in a format
    compatible with the Chrome performance dashboard
- Refactors the code into separate classes and files for improved readability
    and to better support future Windows latency testing

BUG=708747
==========

to

==========
Make VR latency results compatible with perf dashboard, refactor into classes

Makes the following changes to the automated VR latency testing script:
- Adds the functionality to save latency and correlation results in a format
    compatible with the Chrome performance dashboard
- Refactors the code into separate classes and files for improved readability
    and to better support future Windows latency testing
- Runs the test multiple times (default 10) to get an average and standard
deviation

BUG=708747
==========

[bsheedy, 2017-04-18 21:17:26]

https://codereview.chromium.org/2823883003/diff/40001/chrome/test/vr/perf/lat...
File chrome/test/vr/perf/latency/BUILD.gn (right):

https://codereview.chromium.org/2823883003/diff/40001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/BUILD.gn:13: "./webvr_latency_test.py",
On 2017/04/17 22:57:13, Lei Lei wrote:
> You can add all the files in one folder as data, but I am not sure how to add
> current folder, maybe
> data = [
>   "./",
> ]
> 
> See
https://cs.chromium.org/chromium/src/chrome/test/media_router/BUILD.gn?l=10

./ works, and I had that before, but changed it to individual files so that the
BUILD.gn file wouldn't be included. Including it doesn't cause any issues, but
might as well skip it if it's easy to do so.

We could move the BUILD.gn file to chrome/test/vr/perf, which would allow the
inclusion of the entire directory, plus it'd allow us to have a single BUILD.gn
file in the future when other types of perf tests are added.

https://codereview.chromium.org/2823883003/diff/40001/chrome/test/vr/perf/lat...
File chrome/test/vr/perf/latency/android_webvr_latency_test.py (right):

https://codereview.chromium.org/2823883003/diff/40001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/android_webvr_latency_test.py:43:
'https://weableandbob.github.io/Motopho/flicker_apps/webvr/webvr-flicker-app-klaus.html?polyfill=0\&canvasClickPresents=1'])
On 2017/04/17 22:57:13, Lei Lei wrote:
> This line is too long, can you split it into multiple lines?

Done.

https://codereview.chromium.org/2823883003/diff/40001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/android_webvr_latency_test.py:48: str(height/2)])
On 2017/04/17 22:57:13, Lei Lei wrote:
> Is there any way to verify if Chrome is in VR mode just in case tapping
doesn't
> work?

There's no way to query it directly. However, VrCore does output some things to
logcat when entering VR, so we could clear logcat before entering VR, attempt to
enter, and check whether the logcat output contains the string we're looking
for.

I've gone ahead and added that to the script - I haven't had issues with it not
entering VR before, but adding this won't (shouldn't?) hurt.

https://codereview.chromium.org/2823883003/diff/40001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/android_webvr_latency_test.py:64: # TODO(bsheedy):
Maybe migrate to use Devil (overkill?)
On 2017/04/17 22:57:13, Lei Lei wrote:
> I think it is better to migrate to use Devil, so we don't need to update adb
> binary to the latest version manually.

I got it working with Devil, but its dependencies caused the isolated test to be
quite large (uploading to the isolation server stated a push state size of
1403267784, so over 1.4 GB?), which significantly impacted how long it took to
isolate, upload, and fully run the test. If we need Devil in the future, it's
still an option, but for now, I've started including and using the adb in
//third_party/android_tools/sdk/platform-tools, which is regularly updated.

https://codereview.chromium.org/2823883003/diff/40001/chrome/test/vr/perf/lat...
File chrome/test/vr/perf/latency/webvr_latency_test.py (right):

https://codereview.chromium.org/2823883003/diff/40001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/webvr_latency_test.py:20: """Find all devices
connected to tty that match a pattern and device id.
On 2017/04/17 22:57:13, Lei Lei wrote:
> s/Find/Finds

Done.

https://codereview.chromium.org/2823883003/diff/40001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/webvr_latency_test.py:155: self.device_name: {
On 2017/04/17 22:57:13, Lei Lei wrote:
> I thought we got agreement to use bot name for device name, how about
> constructing the charts json data as following to keep consistent with other
> perf data from telemetry tests?
> 
> 'charts' : {
>   'correlation': {
>     '<flicker app url>': {
>        .....
>     }
>     'summary' : {
>       same as above
>     }
>   }
>   'latency' : {
>      '<flicker app url>': {
>        .....
>     }
>     'summary' : {
>       same as above
>     }
>   }
> }
> 
> Moreover, can we run latency test multiple times and generate average value
and
> std deviation? Similar to
>
https://omnibot-isolate-server.appspot.com/browse?namespace=default-gzip&dige...

Done. Sample dashboard is
https://chromeperf.appspot.com/report?sid=2e9079f933c1bd542c9a87bdf05718b3fb6...

[Lei Lei, 2017-04-20 04:56:20]

lgtm

https://codereview.chromium.org/2823883003/diff/40001/chrome/test/vr/perf/lat...
File chrome/test/vr/perf/latency/BUILD.gn (right):

https://codereview.chromium.org/2823883003/diff/40001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/BUILD.gn:13: "./webvr_latency_test.py",
On 2017/04/18 21:17:25, bsheedy wrote:
> On 2017/04/17 22:57:13, Lei Lei wrote:
> > You can add all the files in one folder as data, but I am not sure how to
add
> > current folder, maybe
> > data = [
> >   "./",
> > ]
> > 
> > See
> https://cs.chromium.org/chromium/src/chrome/test/media_router/BUILD.gn?l=10
> 
> ./ works, and I had that before, but changed it to individual files so that
the
> BUILD.gn file wouldn't be included. Including it doesn't cause any issues, but
> might as well skip it if it's easy to do so.
> 
> We could move the BUILD.gn file to chrome/test/vr/perf, which would allow the
> inclusion of the entire directory, plus it'd allow us to have a single
BUILD.gn
> file in the future when other types of perf tests are added.

Yep, I think it is better to move the BUILD.gn file to chrome/test/vr/perf, so
we don't need to update BDUILD.gn file each time when add new files for perf
tests.

It is fine to do it in another patch.

https://codereview.chromium.org/2823883003/diff/40001/chrome/test/vr/perf/lat...
File chrome/test/vr/perf/latency/android_webvr_latency_test.py (right):

https://codereview.chromium.org/2823883003/diff/40001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/android_webvr_latency_test.py:48: str(height/2)])
On 2017/04/18 21:17:25, bsheedy wrote:
> On 2017/04/17 22:57:13, Lei Lei wrote:
> > Is there any way to verify if Chrome is in VR mode just in case tapping
> doesn't
> > work?
> 
> There's no way to query it directly. However, VrCore does output some things
to
> logcat when entering VR, so we could clear logcat before entering VR, attempt
to
> enter, and check whether the logcat output contains the string we're looking
> for.
> 
> I've gone ahead and added that to the script - I haven't had issues with it
not
> entering VR before, but adding this won't (shouldn't?) hurt.

I am not sure if this will be an issue, but I remember we had this issue in
daydream tests, sometimes we got invalid results because Chrome never entered to
VR mode. It is better to add this check and exclude those invalid results.

https://codereview.chromium.org/2823883003/diff/40001/chrome/test/vr/perf/lat...
File chrome/test/vr/perf/latency/webvr_latency_test.py (right):

https://codereview.chromium.org/2823883003/diff/40001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/webvr_latency_test.py:155: self.device_name: {
On 2017/04/18 21:17:25, bsheedy wrote:
> On 2017/04/17 22:57:13, Lei Lei wrote:
> > I thought we got agreement to use bot name for device name, how about
> > constructing the charts json data as following to keep consistent with other
> > perf data from telemetry tests?
> > 
> > 'charts' : {
> >   'correlation': {
> >     '<flicker app url>': {
> >        .....
> >     }
> >     'summary' : {
> >       same as above
> >     }
> >   }
> >   'latency' : {
> >      '<flicker app url>': {
> >        .....
> >     }
> >     'summary' : {
> >       same as above
> >     }
> >   }
> > }
> > 
> > Moreover, can we run latency test multiple times and generate average value
> and
> > std deviation? Similar to
> >
>
https://omnibot-isolate-server.appspot.com/browse?namespace=default-gzip&dige...
> 
> Done. Sample dashboard is
>
https://chromeperf.appspot.com/report?sid=2e9079f933c1bd542c9a87bdf05718b3fb6...

Nice!

https://codereview.chromium.org/2823883003/diff/60001/chrome/test/vr/perf/lat...
File chrome/test/vr/perf/latency/webvr_latency_test.py (right):

https://codereview.chromium.org/2823883003/diff/60001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/webvr_latency_test.py:159: std_correlation)
Nit: we can log everything together.

logging.info('Raw latencies %s\nRaw correlations: %s\nAvg latency = %f +/-
%f\nAvg correlation = %f +/- %f', (latencies, ....))

[bsheedy, 2017-04-20 17:34:55]

bsheedy@chromium.org changed reviewers:
+ thakis@chromium.org

[bsheedy, 2017-04-20 17:34:56]

+thakis@ for OWNERS

https://codereview.chromium.org/2823883003/diff/40001/chrome/test/vr/perf/lat...
File chrome/test/vr/perf/latency/BUILD.gn (right):

https://codereview.chromium.org/2823883003/diff/40001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/BUILD.gn:13: "./webvr_latency_test.py",
On 2017/04/20 04:56:20, Lei Lei wrote:
> On 2017/04/18 21:17:25, bsheedy wrote:
> > On 2017/04/17 22:57:13, Lei Lei wrote:
> > > You can add all the files in one folder as data, but I am not sure how to
> add
> > > current folder, maybe
> > > data = [
> > >   "./",
> > > ]
> > > 
> > > See
> > https://cs.chromium.org/chromium/src/chrome/test/media_router/BUILD.gn?l=10
> > 
> > ./ works, and I had that before, but changed it to individual files so that
> the
> > BUILD.gn file wouldn't be included. Including it doesn't cause any issues,
but
> > might as well skip it if it's easy to do so.
> > 
> > We could move the BUILD.gn file to chrome/test/vr/perf, which would allow
the
> > inclusion of the entire directory, plus it'd allow us to have a single
> BUILD.gn
> > file in the future when other types of perf tests are added.
> 
> Yep, I think it is better to move the BUILD.gn file to chrome/test/vr/perf, so
> we don't need to update BDUILD.gn file each time when add new files for perf
> tests.
> 
> It is fine to do it in another patch.

Done.

https://codereview.chromium.org/2823883003/diff/60001/chrome/test/vr/perf/lat...
File chrome/test/vr/perf/latency/webvr_latency_test.py (right):

https://codereview.chromium.org/2823883003/diff/60001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/webvr_latency_test.py:159: std_correlation)
On 2017/04/20 04:56:20, Lei Lei wrote:
> Nit: we can log everything together.
> 
> logging.info('Raw latencies %s\nRaw correlations: %s\nAvg latency = %f +/-
> %f\nAvg correlation = %f +/- %f', (latencies, ....))

Done.

[Nico, 2017-04-20 17:51:21]

lgtm

https://codereview.chromium.org/2823883003/diff/80001/chrome/test/vr/perf/BUI...
File chrome/test/vr/perf/BUILD.gn (right):

https://codereview.chromium.org/2823883003/diff/80001/chrome/test/vr/perf/BUI...
chrome/test/vr/perf/BUILD.gn:8: "./latency/",
If it's just a couple files, consider listing them explicitly instead of going
by dir

https://codereview.chromium.org/2823883003/diff/80001/chrome/test/vr/perf/lat...
File chrome/test/vr/perf/latency/android_webvr_latency_test.py (right):

https://codereview.chromium.org/2823883003/diff/80001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/android_webvr_latency_test.py:15: 
nit: two blank lines between toplevel things

https://codereview.chromium.org/2823883003/diff/80001/chrome/test/vr/perf/lat...
File chrome/test/vr/perf/latency/motopho_thread.py (right):

https://codereview.chromium.org/2823883003/diff/80001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/motopho_thread.py:8: 
likewise

https://codereview.chromium.org/2823883003/diff/80001/chrome/test/vr/perf/lat...
File chrome/test/vr/perf/latency/robot_arm.py (right):

https://codereview.chromium.org/2823883003/diff/80001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/robot_arm.py:7: 
likewise

https://codereview.chromium.org/2823883003/diff/80001/chrome/test/vr/perf/lat...
File chrome/test/vr/perf/latency/webvr_latency_test.py (right):

https://codereview.chromium.org/2823883003/diff/80001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/webvr_latency_test.py:17: 
ditto

https://codereview.chromium.org/2823883003/diff/80001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/webvr_latency_test.py:19: 
ditto

https://codereview.chromium.org/2823883003/diff/80001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/webvr_latency_test.py:58: 
ditto

https://codereview.chromium.org/2823883003/diff/80001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/webvr_latency_test.py:195: 
nit: no trailing blank line

[bsheedy, 2017-04-20 20:05:01]

https://codereview.chromium.org/2823883003/diff/80001/chrome/test/vr/perf/BUI...
File chrome/test/vr/perf/BUILD.gn (right):

https://codereview.chromium.org/2823883003/diff/80001/chrome/test/vr/perf/BUI...
chrome/test/vr/perf/BUILD.gn:8: "./latency/",
On 2017/04/20 17:51:21, Nico wrote:
> If it's just a couple files, consider listing them explicitly instead of going
> by dir

Done.

https://codereview.chromium.org/2823883003/diff/80001/chrome/test/vr/perf/lat...
File chrome/test/vr/perf/latency/android_webvr_latency_test.py (right):

https://codereview.chromium.org/2823883003/diff/80001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/android_webvr_latency_test.py:15: 
On 2017/04/20 17:51:21, Nico wrote:
> nit: two blank lines between toplevel things

Done.

https://codereview.chromium.org/2823883003/diff/80001/chrome/test/vr/perf/lat...
File chrome/test/vr/perf/latency/motopho_thread.py (right):

https://codereview.chromium.org/2823883003/diff/80001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/motopho_thread.py:8: 
On 2017/04/20 17:51:21, Nico wrote:
> likewise

Done.

https://codereview.chromium.org/2823883003/diff/80001/chrome/test/vr/perf/lat...
File chrome/test/vr/perf/latency/robot_arm.py (right):

https://codereview.chromium.org/2823883003/diff/80001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/robot_arm.py:7: 
On 2017/04/20 17:51:21, Nico wrote:
> likewise

Done.

https://codereview.chromium.org/2823883003/diff/80001/chrome/test/vr/perf/lat...
File chrome/test/vr/perf/latency/webvr_latency_test.py (right):

https://codereview.chromium.org/2823883003/diff/80001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/webvr_latency_test.py:17: 
On 2017/04/20 17:51:21, Nico wrote:
> ditto

Done.

https://codereview.chromium.org/2823883003/diff/80001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/webvr_latency_test.py:19: 
On 2017/04/20 17:51:21, Nico wrote:
> ditto

Done.

https://codereview.chromium.org/2823883003/diff/80001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/webvr_latency_test.py:58: 
On 2017/04/20 17:51:21, Nico wrote:
> ditto

Done.

https://codereview.chromium.org/2823883003/diff/80001/chrome/test/vr/perf/lat...
chrome/test/vr/perf/latency/webvr_latency_test.py:195: 
On 2017/04/20 17:51:21, Nico wrote:
> nit: no trailing blank line

Done.

[bsheedy, 2017-04-20 20:05:08]

The CQ bit was checked by bsheedy@chromium.org

[bsheedy, 2017-04-20 20:05:09]

The patchset sent to the CQ was uploaded after l-g-t-m from leilei@chromium.org,
thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2823883003/#ps100001
(title: "Address nits")

[commit-bot: I haz the power, 2017-04-20 20:06:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-20 21:35:34]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492718708756850,
"parent_rev": "f6cbd1a7c6fc9970840ad5242dad4cfb55753630", "commit_rev":
"a15848198deb716d77753d297355bac34cd33b96"}

[commit-bot: I haz the power, 2017-04-20 21:35:41]

Description was changed from

==========
Make VR latency results compatible with perf dashboard, refactor into classes

Makes the following changes to the automated VR latency testing script:
- Adds the functionality to save latency and correlation results in a format
    compatible with the Chrome performance dashboard
- Refactors the code into separate classes and files for improved readability
    and to better support future Windows latency testing
- Runs the test multiple times (default 10) to get an average and standard
deviation

BUG=708747
==========

to

==========
Make VR latency results compatible with perf dashboard, refactor into classes

Makes the following changes to the automated VR latency testing script:
- Adds the functionality to save latency and correlation results in a format
    compatible with the Chrome performance dashboard
- Refactors the code into separate classes and files for improved readability
    and to better support future Windows latency testing
- Runs the test multiple times (default 10) to get an average and standard
deviation

BUG=708747

Review-Url: https://codereview.chromium.org/2823883003
Cr-Commit-Position: refs/heads/master@{#466147}
Committed:
https://chromium.googlesource.com/chromium/src/+/a15848198deb716d77753d297355...
==========

[commit-bot: I haz the power, 2017-04-20 21:35:45]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/a15848198deb716d77753d297355...