Porting relevant legacy conversion code from performance_lp to src side

Porting relevant legacy conversion code from performance_lp to src side

BUG=chromium:633253
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/082a30cca1f30f75f9ef932153d20fa0135ae677
Cr-Commit-Position: refs/heads/master@{#429997}

[eyaich1, 2016-11-03 14:18:49]

eyaich@chromium.org changed reviewers:
+ dtu@chromium.org, eakuefner@chromium.org

[eyaich1, 2016-11-03 14:18:50]

Note: Most of this code is copied from
build/scripts/slave/performance_log_processor.py including the tests.  I
manipulated it slightly to get it in the form to directly use results_dashboard
and pulled in some code from build/scripts/slave/runtest.py as well.

Ethan, I put you on as the main reviewer for the porting of the data.  I am
using the same test examples they used (the files in the data directory) so the
code is generating the same results.

+ kbr for owners approval in script directory

[eakuefner, 2016-11-03 17:03:33]

Should this go in src/tools/perf?

[Ken Russell (switch to Gerrit), 2016-11-03 18:31:41]

On 2016/11/03 17:03:33, eakuefner wrote:
> Should this go in src/tools/perf?

I think it should. testing/scripts/ is supposed to only contain scripts that are
invoked by recipes.

Is moving them to src/tools/perf/ feasible? telemetry_perf_unittests.py shows
how to invoke a script that lives there but it doesn't import scripts from that
directory.

If it's infeasible then I'd put all of the new scripts in subdirectories of
testing/scripts, like testing/scripts/perf/ .

[eyaich1, 2016-11-04 15:07:04]

Description was changed from

==========
Porting relevant legacy conversion code from performance_lp to src side

BUG=chromium:633253
==========

to

==========
Porting relevant legacy conversion code from performance_lp to src side

BUG=chromium:633253
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[eyaich1, 2016-11-04 15:07:49]

On 2016/11/03 18:31:41, Ken Russell wrote:
> On 2016/11/03 17:03:33, eakuefner wrote:
> > Should this go in src/tools/perf?
> 
> I think it should. testing/scripts/ is supposed to only contain scripts that
are
> invoked by recipes.
> 
> Is moving them to src/tools/perf/ feasible? telemetry_perf_unittests.py shows
> how to invoke a script that lives there but it doesn't import scripts from
that
> directory.
> 
> If it's infeasible then I'd put all of the new scripts in subdirectories of
> testing/scripts, like testing/scripts/perf/ .

Done.

+ enne@ for approvals for updates to the gn target for cc_peftests to add the
new script to the isolate.

[eyaich1, 2016-11-04 15:09:18]

Description was changed from

==========
Porting relevant legacy conversion code from performance_lp to src side

BUG=chromium:633253
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Porting relevant legacy conversion code from performance_lp to src side

BUG=chromium:633253
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[eyaich1, 2016-11-04 15:09:36]

On 2016/11/04 15:08:20, eyaich1 wrote:

Ken I will still need owners approval from you in the testing/scripts directory.

[Ken Russell (switch to Gerrit), 2016-11-04 15:16:16]

kbr@chromium.org changed reviewers:
+ kbr@chromium.org

[Ken Russell (switch to Gerrit), 2016-11-04 15:16:16]

Thanks for reorganizing. lgtm

https://codereview.chromium.org/2479543002/diff/40001/testing/scripts/run_gte...
File testing/scripts/run_gtest_perf_test.py (right):

https://codereview.chromium.org/2479543002/diff/40001/testing/scripts/run_gte...
testing/scripts/run_gtest_perf_test.py:39: # Add src/tools/perf wheree
generate_legacy_perf_dashboard_json.py lives
typo: wheree

[eakuefner, 2016-11-04 16:58:56]

Looks good overall; some mostly minor comments.

https://codereview.chromium.org/2479543002/diff/40001/testing/scripts/run_gte...
File testing/scripts/run_gtest_perf_test.py (right):

https://codereview.chromium.org/2479543002/diff/40001/testing/scripts/run_gte...
testing/scripts/run_gtest_perf_test.py:95: env=env, stdoutfile=tempfile_path) ==
0)
style nit: indent +4 spaces instead of +2.

https://codereview.chromium.org/2479543002/diff/40001/testing/scripts/run_gte...
testing/scripts/run_gtest_perf_test.py:99: results_processor = \
style nit: no backslash line continuations in python[0] -- use parens if you
need to.

[0] https://google.github.io/styleguide/pyguide.html#Line_length

https://codereview.chromium.org/2479543002/diff/40001/tools/perf/generate_leg...
File tools/perf/generate_legacy_perf_dashboard_json.py (right):

https://codereview.chromium.org/2479543002/diff/40001/tools/perf/generate_leg...
tools/perf/generate_legacy_perf_dashboard_json.py:2: # Copyright 2015 The
Chromium Authors. All rights reserved.
2016?

https://codereview.chromium.org/2479543002/diff/40001/tools/perf/generate_leg...
tools/perf/generate_legacy_perf_dashboard_json.py:7: Taken from
chromium/build/scripts/slave/performance_log_processory.py, we are
I'd put a link to the revision of performance_log_processor that you took this
from:
https://chromium.googlesource.com/chromium/tools/build/+/ede4a5cb71a8e524c18c...

You can shorten it using goo.gl. I also don't think it's necessary to say the
stuff about the smallest amount of code and so on. This script will eventually
expand beyond its current purpose so I don't think it's necessary to enshrine
the current purpose in the comment.

https://codereview.chromium.org/2479543002/diff/40001/tools/perf/generate_leg...
tools/perf/generate_legacy_perf_dashboard_json.py:61: # TODO(eyaich): Determine
if this format is still used by any perf tests
Do you have a plan for how to do this? Maybe a bug about getting rid of the
format?

https://codereview.chromium.org/2479543002/diff/40001/tools/perf/generate_leg...
tools/perf/generate_legacy_perf_dashboard_json.py:102: def __init__(self):
Something is weird here. You're missing a class definition perhaps?

https://codereview.chromium.org/2479543002/diff/40001/tools/perf/generate_leg...
tools/perf/generate_legacy_perf_dashboard_json.py:150: # Compute the mean and
standard deviation for a multiple-valued item,
Can you just be explicit about this and say "a list or histogram" and "a scalar"
respectively?

https://codereview.chromium.org/2479543002/diff/40001/tools/perf/generate_leg...
tools/perf/generate_legacy_perf_dashboard_json.py:194: # Include a sorted list
of important trace names if there are any.
Why do we need to do this?

https://codereview.chromium.org/2479543002/diff/40001/tools/perf/generate_leg...
tools/perf/generate_legacy_perf_dashboard_json.py:200: #charts[graph_name] =
json.dumps(graph_dict)
oops?

https://codereview.chromium.org/2479543002/diff/40001/tools/perf/generate_leg...
tools/perf/generate_legacy_perf_dashboard_json.py:204: # _CalculateStatistics
needs to be a member function.
This sequence of comments is really unclear to me. You can disable pylint rules
by name rather than number, and that would make this comment much easier to
parse.

https://codereview.chromium.org/2479543002/diff/40001/tools/perf/generate_leg...
tools/perf/generate_legacy_perf_dashboard_json.py:225: return 0.0, 0.0
hm, this isn't a 3-tuple

https://codereview.chromium.org/2479543002/diff/40001/tools/perf/generate_leg...
File tools/perf/generate_legacy_perf_dashboard_json_unittest.py (right):

https://codereview.chromium.org/2479543002/diff/40001/tools/perf/generate_leg...
tools/perf/generate_legacy_perf_dashboard_json_unittest.py:7: """Unit tests for
generating legacy perf dashboard json used by
I think that it is clear what this file does from the filename, no comment
necessary.

[eyaich1, 2016-11-04 17:33:03]

https://codereview.chromium.org/2479543002/diff/40001/testing/scripts/run_gte...
File testing/scripts/run_gtest_perf_test.py (right):

https://codereview.chromium.org/2479543002/diff/40001/testing/scripts/run_gte...
testing/scripts/run_gtest_perf_test.py:39: # Add src/tools/perf wheree
generate_legacy_perf_dashboard_json.py lives
On 2016/11/04 15:16:16, Ken Russell wrote:
> typo: wheree

Done.

https://codereview.chromium.org/2479543002/diff/40001/testing/scripts/run_gte...
testing/scripts/run_gtest_perf_test.py:95: env=env, stdoutfile=tempfile_path) ==
0)
On 2016/11/04 16:58:56, eakuefner wrote:
> style nit: indent +4 spaces instead of +2.

Done.

https://codereview.chromium.org/2479543002/diff/40001/testing/scripts/run_gte...
testing/scripts/run_gtest_perf_test.py:99: results_processor = \
On 2016/11/04 16:58:56, eakuefner wrote:
> style nit: no backslash line continuations in python[0] -- use parens if you
> need to.
> 
> [0] https://google.github.io/styleguide/pyguide.html#Line_length

Done.

https://codereview.chromium.org/2479543002/diff/40001/tools/perf/generate_leg...
File tools/perf/generate_legacy_perf_dashboard_json.py (right):

https://codereview.chromium.org/2479543002/diff/40001/tools/perf/generate_leg...
tools/perf/generate_legacy_perf_dashboard_json.py:2: # Copyright 2015 The
Chromium Authors. All rights reserved.
On 2016/11/04 16:58:56, eakuefner wrote:
> 2016?

Done.

https://codereview.chromium.org/2479543002/diff/40001/tools/perf/generate_leg...
tools/perf/generate_legacy_perf_dashboard_json.py:2: # Copyright 2015 The
Chromium Authors. All rights reserved.
On 2016/11/04 16:58:56, eakuefner wrote:
> 2016?

Done.

https://codereview.chromium.org/2479543002/diff/40001/tools/perf/generate_leg...
tools/perf/generate_legacy_perf_dashboard_json.py:7: Taken from
chromium/build/scripts/slave/performance_log_processory.py, we are
On 2016/11/04 16:58:56, eakuefner wrote:
> I'd put a link to the revision of performance_log_processor that you took this
> from:
>
https://chromium.googlesource.com/chromium/tools/build/+/ede4a5cb71a8e524c18c...
> 
> You can shorten it using goo.gl. I also don't think it's necessary to say the
> stuff about the smallest amount of code and so on. This script will eventually
> expand beyond its current purpose so I don't think it's necessary to enshrine
> the current purpose in the comment.

Done.

https://codereview.chromium.org/2479543002/diff/40001/tools/perf/generate_leg...
tools/perf/generate_legacy_perf_dashboard_json.py:61: # TODO(eyaich): Determine
if this format is still used by any perf tests
On 2016/11/04 16:58:56, eakuefner wrote:
> Do you have a plan for how to do this? Maybe a bug about getting rid of the
> format?

I have to port over the C++ tests one by one since they each need their own
isolate.  If I came across one that output this format I would then port the
histogram code over!  The problem is that there a ton of copy paste because this
uses quite a few utility functions in build/script/slave/chromium_utils that I
didn't want to have to deal with if I didn't have to.

https://codereview.chromium.org/2479543002/diff/40001/tools/perf/generate_leg...
tools/perf/generate_legacy_perf_dashboard_json.py:102: def __init__(self):
On 2016/11/04 16:58:56, eakuefner wrote:
> Something is weird here. You're missing a class definition perhaps?

Its the init for the top level class, LegacyResultsProcessor.  I moved it up.

https://codereview.chromium.org/2479543002/diff/40001/tools/perf/generate_leg...
tools/perf/generate_legacy_perf_dashboard_json.py:150: # Compute the mean and
standard deviation for a multiple-valued item,
On 2016/11/04 16:58:56, eakuefner wrote:
> Can you just be explicit about this and say "a list or histogram" and "a
scalar"
> respectively?

Done.

https://codereview.chromium.org/2479543002/diff/40001/tools/perf/generate_leg...
tools/perf/generate_legacy_perf_dashboard_json.py:194: # Include a sorted list
of important trace names if there are any.
On 2016/11/04 16:58:56, eakuefner wrote:
> Why do we need to do this?

We reference it in MakeListOfPoints in results_dashboard.py, so I assume it is
something with the legacy results format.

https://codereview.chromium.org/2479543002/diff/40001/tools/perf/generate_leg...
tools/perf/generate_legacy_perf_dashboard_json.py:200: #charts[graph_name] =
json.dumps(graph_dict)
On 2016/11/04 16:58:56, eakuefner wrote:
> oops?

Done.

https://codereview.chromium.org/2479543002/diff/40001/tools/perf/generate_leg...
tools/perf/generate_legacy_perf_dashboard_json.py:204: # _CalculateStatistics
needs to be a member function.
On 2016/11/04 16:58:56, eakuefner wrote:
> This sequence of comments is really unclear to me. You can disable pylint
rules
> by name rather than number, and that would make this comment much easier to
> parse.

This is copy and pasted from performacne_log_processor.  I am not sure how to
obtain the names or updates but if you feel strongly about it please point me in
the right direction.

https://codereview.chromium.org/2479543002/diff/40001/tools/perf/generate_leg...
tools/perf/generate_legacy_perf_dashboard_json.py:225: return 0.0, 0.0
On 2016/11/04 16:58:56, eakuefner wrote:
> hm, this isn't a 3-tuple

Done.

https://codereview.chromium.org/2479543002/diff/40001/tools/perf/generate_leg...
File tools/perf/generate_legacy_perf_dashboard_json_unittest.py (right):

https://codereview.chromium.org/2479543002/diff/40001/tools/perf/generate_leg...
tools/perf/generate_legacy_perf_dashboard_json_unittest.py:7: """Unit tests for
generating legacy perf dashboard json used by
On 2016/11/04 16:58:56, eakuefner wrote:
> I think that it is clear what this file does from the filename, no comment
> necessary.

Done.

[eakuefner, 2016-11-04 17:39:38]

lgtm

[aelias_OOO_until_Jul13, 2016-11-04 17:45:55]

aelias@chromium.org changed reviewers:
+ aelias@chromium.org

[aelias_OOO_until_Jul13, 2016-11-04 17:45:56]

cc/ lgtm

[eyaich1, 2016-11-04 17:46:36]

The CQ bit was checked by eyaich@chromium.org

[eyaich1, 2016-11-04 17:46:36]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/2479543002/#ps60001
(title: "Responding to review comments")

[commit-bot: I haz the power, 2016-11-04 17:47:02]

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, 2016-11-04 17:56:13]

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

[commit-bot: I haz the power, 2016-11-04 17:56:14]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[iannucci, 2016-11-04 18:23:33]

iannucci@chromium.org changed reviewers:
+ iannucci@chromium.org

[iannucci, 2016-11-04 18:23:33]

lgtm

[eyaich1, 2016-11-04 18:24:05]

The CQ bit was checked by eyaich@chromium.org

[commit-bot: I haz the power, 2016-11-04 18:24:52]

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, 2016-11-04 18:34:46]

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

[commit-bot: I haz the power, 2016-11-04 18:34:47]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Ken Russell (switch to Gerrit), 2016-11-04 18:40:45]

On 2016/11/04 18:34:47, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Now those failures are real.

[eyaich1, 2016-11-04 18:42:01]

The CQ bit was checked by eyaich@chromium.org

[eyaich1, 2016-11-04 18:42:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
eakuefner@chromium.org, kbr@chromium.org, aelias@chromium.org,
iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/2479543002/#ps80001
(title: "Renaming protected functions")

[commit-bot: I haz the power, 2016-11-04 18:42:41]

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, 2016-11-04 20:21:20]

Description was changed from

==========
Porting relevant legacy conversion code from performance_lp to src side

BUG=chromium:633253
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Porting relevant legacy conversion code from performance_lp to src side

BUG=chromium:633253
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-11-04 20:21:22]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-04 20:24:40]

Description was changed from

==========
Porting relevant legacy conversion code from performance_lp to src side

BUG=chromium:633253
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Porting relevant legacy conversion code from performance_lp to src side

BUG=chromium:633253
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/082a30cca1f30f75f9ef932153d20fa0135ae677
Cr-Commit-Position: refs/heads/master@{#429997}
==========

[commit-bot: I haz the power, 2016-11-04 20:24:41]

Patchset 5 (id:??) landed as
https://crrev.com/082a30cca1f30f75f9ef932153d20fa0135ae677
Cr-Commit-Position: refs/heads/master@{#429997}

[Dirk Pranke, 2016-12-02 21:47:54]

dpranke@chromium.org changed reviewers:
+ dpranke@chromium.org

[Dirk Pranke, 2016-12-02 21:47:55]

lgtm