Enable loading.desktop benchmark with network service enabled on perf fyi bot.

Enable loading.desktop benchmark with network service enabled on perf fyi bot.

BUG=717738
Review-Url: https://codereview.chromium.org/2973733002
Cr-Commit-Position: refs/heads/master@{#485007}
Committed: https://chromium.googlesource.com/chromium/src/+/987b0d546a3b0cc49512a8f2b2fdfe1996c6f0db

[yzshen1, 2017-07-06 00:10:28]

The CQ bit was checked by yzshen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-06 00:10:57]

Dry run: CQ is trying da patch.

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

[yzshen1, 2017-07-06 00:14:58]

yzshen@chromium.org changed reviewers:
+ jam@chromium.org, nednguyen@google.com

[yzshen1, 2017-07-06 00:14:59]

Hi, Ned and John.

Would you please take a look? Thanks!

One thing I am not entirely sure about is chromium.perf.fyi.extras.json: I don't
know how I could test the command locally. Please let me know if you know how to
test it. Thanks!

[commit-bot: I haz the power, 2017-07-06 01:16:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-06 01:16:14]

Dry run: This issue passed the CQ dry run.

[nednguyen, 2017-07-06 01:17:38]

Description was changed from

==========
Enable loading.desktop benchmark with network service enabled on perf fyi bot.

BUG=717738
==========

to

==========
Enable loading.desktop benchmark with network service enabled on perf fyi bot.

BUG=717738
==========

[nednguyen, 2017-07-06 01:17:38]

nednguyen@google.com changed reviewers:
+ jam@google.com, martiniss@chromium.org
- jam@chromium.org

[nednguyen, 2017-07-06 01:18:20]

On 2017/07/06 00:14:59, yzshen1 wrote:
> Hi, Ned and John.
> 
> Would you please take a look? Thanks!
> 
> One thing I am not entirely sure about is chromium.perf.fyi.extras.json: I
don't
> know how I could test the command locally. Please let me know if you know how
to
> test it. Thanks!

Stephen: can you take a pass at tools/perf/core/perf_data_generator.py	? The
rest look reasonable to me.

[martiniss, 2017-07-06 01:30:46]

https://codereview.chromium.org/2973733002/diff/40001/testing/buildbot/chromi...
File testing/buildbot/chromium.perf.fyi.extras.json (right):

https://codereview.chromium.org/2973733002/diff/40001/testing/buildbot/chromi...
testing/buildbot/chromium.perf.fyi.extras.json:1: {
What is this file supposed to be doing? AFAIK, adding this file here won't do
anything at all.

[nednguyen, 2017-07-06 13:15:26]

https://codereview.chromium.org/2973733002/diff/40001/testing/buildbot/chromi...
File testing/buildbot/chromium.perf.fyi.extras.json (right):

https://codereview.chromium.org/2973733002/diff/40001/testing/buildbot/chromi...
testing/buildbot/chromium.perf.fyi.extras.json:1: {
On 2017/07/06 01:30:46, martiniss wrote:
> What is this file supposed to be doing? AFAIK, adding this file here won't do
> anything at all.

This is used for generating the manual entries in
testing/buildbot/chromium.perf.fyi.json	(See change to
tools/perf/core/perf_data_generator.py)

[yzshen1, 2017-07-06 15:37:10]

On 2017/07/06 13:15:26, nednguyen wrote:
>
https://codereview.chromium.org/2973733002/diff/40001/testing/buildbot/chromi...
> File testing/buildbot/chromium.perf.fyi.extras.json (right):
> 
>
https://codereview.chromium.org/2973733002/diff/40001/testing/buildbot/chromi...
> testing/buildbot/chromium.perf.fyi.extras.json:1: {
> On 2017/07/06 01:30:46, martiniss wrote:
> > What is this file supposed to be doing? AFAIK, adding this file here won't
do
> > anything at all.
> 
> This is used for generating the manual entries in
> testing/buildbot/chromium.perf.fyi.json	(See change to
> tools/perf/core/perf_data_generator.py)

Thanks for the comments!

As Ned said, those are manual entries. Because the benchmark is one of those
"third-party contributed" benchmarks in tools/perfs/contrib, it is not
discovered by perf_data_generator automatically. 

(There was a discussion about this solution on chrome-benchmarking-request@
titled "How could I run existing perf tests with some extra browser flags on a
bot?".)

[nednguyen, 2017-07-06 16:36:53]

https://codereview.chromium.org/2973733002/diff/40001/testing/buildbot/chromi...
File testing/buildbot/chromium.perf.fyi.extras.json (right):

https://codereview.chromium.org/2973733002/diff/40001/testing/buildbot/chromi...
testing/buildbot/chromium.perf.fyi.extras.json:1: {
I think we may want to move this file to tools/perf/contrib/ instead. The reason
is tools/perf/PRESUBMIT is only triggered if you touch any file under perf/
directory.

[yzshen1, 2017-07-06 17:41:20]

The CQ bit was checked by yzshen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-06 17:41:37]

Dry run: CQ is trying da patch.

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

[yzshen1, 2017-07-06 17:43:29]

https://codereview.chromium.org/2973733002/diff/40001/testing/buildbot/chromi...
File testing/buildbot/chromium.perf.fyi.extras.json (right):

https://codereview.chromium.org/2973733002/diff/40001/testing/buildbot/chromi...
testing/buildbot/chromium.perf.fyi.extras.json:1: {
On 2017/07/06 16:36:53, nednguyen wrote:
> I think we may want to move this file to tools/perf/contrib/ instead. The
reason
> is tools/perf/PRESUBMIT is only triggered if you touch any file under perf/
> directory.

Done.

One thing is that I got the following Presubmit error:
"""You cannot add files to top level of contrib directory. Please moves these
files to a sub directory:

/usr/local/google/home/yzshen/src/cr1/src/tools/perf/contrib/chromium.perf.fyi.extras.json"""

I think it may be okay if adding such extras.json files is rare and reviewed by
the perf team. WDYT? Thanks!

[nednguyen, 2017-07-06 17:49:35]

On 2017/07/06 17:43:29, yzshen1 wrote:
>
https://codereview.chromium.org/2973733002/diff/40001/testing/buildbot/chromi...
> File testing/buildbot/chromium.perf.fyi.extras.json (right):
> 
>
https://codereview.chromium.org/2973733002/diff/40001/testing/buildbot/chromi...
> testing/buildbot/chromium.perf.fyi.extras.json:1: {
> On 2017/07/06 16:36:53, nednguyen wrote:
> > I think we may want to move this file to tools/perf/contrib/ instead. The
> reason
> > is tools/perf/PRESUBMIT is only triggered if you touch any file under perf/
> > directory.
> 
> Done.
> 
> One thing is that I got the following Presubmit error:
> """You cannot add files to top level of contrib directory. Please moves these
> files to a sub directory:
> 
>
/usr/local/google/home/yzshen/src/cr1/src/tools/perf/contrib/chromium.perf.fyi.extras.json"""
> 
> I think it may be okay if adding such extras.json files is rare and reviewed
by
> the perf team. WDYT? Thanks!

Oh...
Then I think we can just move chromium.perf.fyi.extras.json to
tools/perf/chromium.perf.fyi.extras.json

lgtm but please wait for Stephen

[nednguyen, 2017-07-06 17:49:46]

On 2017/07/06 17:43:29, yzshen1 wrote:
>
https://codereview.chromium.org/2973733002/diff/40001/testing/buildbot/chromi...
> File testing/buildbot/chromium.perf.fyi.extras.json (right):
> 
>
https://codereview.chromium.org/2973733002/diff/40001/testing/buildbot/chromi...
> testing/buildbot/chromium.perf.fyi.extras.json:1: {
> On 2017/07/06 16:36:53, nednguyen wrote:
> > I think we may want to move this file to tools/perf/contrib/ instead. The
> reason
> > is tools/perf/PRESUBMIT is only triggered if you touch any file under perf/
> > directory.
> 
> Done.
> 
> One thing is that I got the following Presubmit error:
> """You cannot add files to top level of contrib directory. Please moves these
> files to a sub directory:
> 
>
/usr/local/google/home/yzshen/src/cr1/src/tools/perf/contrib/chromium.perf.fyi.extras.json"""
> 
> I think it may be okay if adding such extras.json files is rare and reviewed
by
> the perf team. WDYT? Thanks!

Oh...
Then I think we can just move chromium.perf.fyi.extras.json to
tools/perf/chromium.perf.fyi.extras.json

lgtm but please wait for Stephen

[yzshen1, 2017-07-06 17:51:56]

The CQ bit was checked by yzshen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-06 17:52:11]

Dry run: CQ is trying da patch.

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

[yzshen1, 2017-07-06 17:52:19]

On 2017/07/06 17:49:46, nednguyen wrote:
> On 2017/07/06 17:43:29, yzshen1 wrote:
> >
>
https://codereview.chromium.org/2973733002/diff/40001/testing/buildbot/chromi...
> > File testing/buildbot/chromium.perf.fyi.extras.json (right):
> > 
> >
>
https://codereview.chromium.org/2973733002/diff/40001/testing/buildbot/chromi...
> > testing/buildbot/chromium.perf.fyi.extras.json:1: {
> > On 2017/07/06 16:36:53, nednguyen wrote:
> > > I think we may want to move this file to tools/perf/contrib/ instead. The
> > reason
> > > is tools/perf/PRESUBMIT is only triggered if you touch any file under
perf/
> > > directory.
> > 
> > Done.
> > 
> > One thing is that I got the following Presubmit error:
> > """You cannot add files to top level of contrib directory. Please moves
these
> > files to a sub directory:
> > 
> >
>
/usr/local/google/home/yzshen/src/cr1/src/tools/perf/contrib/chromium.perf.fyi.extras.json"""
> > 
> > I think it may be okay if adding such extras.json files is rare and reviewed
> by
> > the perf team. WDYT? Thanks!
> 
> Oh...
> Then I think we can just move chromium.perf.fyi.extras.json to
> tools/perf/chromium.perf.fyi.extras.json
> 
> lgtm but please wait for Stephen

Thanks! Done.

[commit-bot: I haz the power, 2017-07-06 19:55:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-06 19:55:02]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[martiniss, 2017-07-06 22:11:22]

lgtm

https://codereview.chromium.org/2973733002/diff/80001/tools/perf/chromium.per...
File tools/perf/chromium.perf.fyi.extras.json (right):

https://codereview.chromium.org/2973733002/diff/80001/tools/perf/chromium.per...
tools/perf/chromium.perf.fyi.extras.json:1: {
Can you add a comment in this file about what this does?

You can just add a key called "comment". So, something like

{
  "comment": [
    "This file does blah blah",
    "blah blah"
  ],
  "Mojo Linux Perf': {
  ..
}

And then whitelist the comment key in perf_data_generator.py

https://codereview.chromium.org/2973733002/diff/80001/tools/perf/core/perf_da...
File tools/perf/core/perf_data_generator.py (right):

https://codereview.chromium.org/2973733002/diff/80001/tools/perf/core/perf_da...
tools/perf/core/perf_data_generator.py:862: def append_extra_tests(waterfall,
tests):
Can you add a small docstring here?

[martiniss, 2017-07-06 23:03:12]

Oh also, can you add a small test for this in perf_data_generator_unittest.py?
Just test that extra tests get appended to waterfalls. IIRC, there are some
tests which mock out open() in there.

[yzshen1, 2017-07-07 00:03:08]

On 2017/07/06 23:03:12, martiniss wrote:
> Oh also, can you add a small test for this in perf_data_generator_unittest.py?
> Just test that extra tests get appended to waterfalls. IIRC, there are some
> tests which mock out open() in there.

Sure will do.

One native question: how can I run these tests locally?
I think it should be tools/perf/run_telemetry_tests. But this script hangs when
I run it.

Thanks!

[nednguyen, 2017-07-07 00:04:26]

On 2017/07/07 00:03:08, yzshen1 wrote:
> On 2017/07/06 23:03:12, martiniss wrote:
> > Oh also, can you add a small test for this in
perf_data_generator_unittest.py?
> > Just test that extra tests get appended to waterfalls. IIRC, there are some
> > tests which mock out open() in there.
> 
> Sure will do.
> 
> One native question: how can I run these tests locally?
> I think it should be tools/perf/run_telemetry_tests. But this script hangs
when
> I run it.
> 
> Thanks!

The script is tools/perf/run_tests.. To run only perf_data_generator_unittest,
you run:

tools/perf/run_tests perf_data_generator_unittest

[yzshen1, 2017-07-07 00:10:32]

On 2017/07/07 00:04:26, nednguyen wrote:
> On 2017/07/07 00:03:08, yzshen1 wrote:
> > On 2017/07/06 23:03:12, martiniss wrote:
> > > Oh also, can you add a small test for this in
> perf_data_generator_unittest.py?
> > > Just test that extra tests get appended to waterfalls. IIRC, there are
some
> > > tests which mock out open() in there.
> > 
> > Sure will do.
> > 
> > One native question: how can I run these tests locally?
> > I think it should be tools/perf/run_telemetry_tests. But this script hangs
> when
> > I run it.
> > 
> > Thanks!
> 
> The script is tools/perf/run_tests.. To run only perf_data_generator_unittest,
> you run:
> 
> tools/perf/run_tests perf_data_generator_unittest

Thanks Ned!
This also hangs. I will need to look deeper...

[nednguyen, 2017-07-07 00:13:13]

On 2017/07/07 00:10:32, yzshen1 wrote:
> On 2017/07/07 00:04:26, nednguyen wrote:
> > On 2017/07/07 00:03:08, yzshen1 wrote:
> > > On 2017/07/06 23:03:12, martiniss wrote:
> > > > Oh also, can you add a small test for this in
> > perf_data_generator_unittest.py?
> > > > Just test that extra tests get appended to waterfalls. IIRC, there are
> some
> > > > tests which mock out open() in there.
> > > 
> > > Sure will do.
> > > 
> > > One native question: how can I run these tests locally?
> > > I think it should be tools/perf/run_telemetry_tests. But this script hangs
> > when
> > > I run it.
> > > 
> > > Thanks!
> > 
> > The script is tools/perf/run_tests.. To run only
perf_data_generator_unittest,
> > you run:
> > 
> > tools/perf/run_tests perf_data_generator_unittest
> 
> Thanks Ned!
> This also hangs. I will need to look deeper...

It may hangs because it try to download all the WPR & binaries files before
running tests. How about running it with "-v" & wait for a bit?

[yzshen1, 2017-07-07 18:02:44]

Thanks Stephen and Ned!

I have addressed the comments and also added a test.

https://codereview.chromium.org/2973733002/diff/80001/tools/perf/chromium.per...
File tools/perf/chromium.perf.fyi.extras.json (right):

https://codereview.chromium.org/2973733002/diff/80001/tools/perf/chromium.per...
tools/perf/chromium.perf.fyi.extras.json:1: {
On 2017/07/06 22:11:22, martiniss wrote:
> Can you add a comment in this file about what this does?
> 
> You can just add a key called "comment". So, something like
> 
> {
>   "comment": [
>     "This file does blah blah",
>     "blah blah"
>   ],
>   "Mojo Linux Perf': {
>   ..
> }
> 
> And then whitelist the comment key in perf_data_generator.py

Done.

https://codereview.chromium.org/2973733002/diff/80001/tools/perf/core/perf_da...
File tools/perf/core/perf_data_generator.py (right):

https://codereview.chromium.org/2973733002/diff/80001/tools/perf/core/perf_da...
tools/perf/core/perf_data_generator.py:862: def append_extra_tests(waterfall,
tests):
On 2017/07/06 22:11:22, martiniss wrote:
> Can you add a small docstring here?

Done.

[yzshen1, 2017-07-07 18:04:49]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2017-07-07 18:04:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
nednguyen@google.com, martiniss@chromium.org
Link to the patchset: https://codereview.chromium.org/2973733002/#ps100001
(title: ".")

[commit-bot: I haz the power, 2017-07-07 18:05:03]

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-07-07 19:26:41]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1499450689780740,
"parent_rev": "10801c86f051afb7d5194940d40582d8d7691a62", "commit_rev":
"987b0d546a3b0cc49512a8f2b2fdfe1996c6f0db"}

[commit-bot: I haz the power, 2017-07-07 19:26:57]

Description was changed from

==========
Enable loading.desktop benchmark with network service enabled on perf fyi bot.

BUG=717738
==========

to

==========
Enable loading.desktop benchmark with network service enabled on perf fyi bot.

BUG=717738

Review-Url: https://codereview.chromium.org/2973733002
Cr-Commit-Position: refs/heads/master@{#485007}
Committed:
https://chromium.googlesource.com/chromium/src/+/987b0d546a3b0cc49512a8f2b2fd...
==========

[commit-bot: I haz the power, 2017-07-07 19:26:58]

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