First pass performance try bot.

First pass performance try bot.

Piggy-backed a performance comparison mode onto the existing bisect architecture. The script will build and run the specified performance test, revert any local changes, build, and run again. At the moment, only chromium changes can be perf-tested. Future CL's will add the ability to perf test blink changes.

Waiting on http://codereview.chromium.org/27413002, http://codereview.chromium.org/26179009.

BUG=
TEST=

Modify run-perf-test.cfg, commit, and run git try --user=<user> -b linux_perf_bisect

Expect bot to run with/without patch, ie. http://build.chromium.org/p/tryserver.chromium/builders/linux_perf_bisect/builds/387

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229963

[tonyg, 2013-10-17 01:01:32]

This is awesome. Turned out really clean. A few comments below.

https://codereview.chromium.org/27165006/diff/1/tools/PRESUBMIT.py
File tools/PRESUBMIT.py (right):

https://codereview.chromium.org/27165006/diff/1/tools/PRESUBMIT.py#newcode5
tools/PRESUBMIT.py:5: """Top-level presubmit script for bisect trybot.
Should we call this something other than bisect trybot now?

Given that we now have two uses for this system and we have proposed profiler
trybots too, maybe we should come up with a general name for this system? Got
any catchy naming ideas?

https://codereview.chromium.org/27165006/diff/1/tools/bisect-perf-regression.py
File tools/bisect-perf-regression.py (right):

https://codereview.chromium.org/27165006/diff/1/tools/bisect-perf-regression....
tools/bisect-perf-regression.py:1266: r'tools\\perf\\run_' in command_to_run)
I believe you want the r'' OR the \\s, but not both.

https://codereview.chromium.org/27165006/diff/1/tools/run-bisect-perf-regress...
File tools/run-bisect-perf-regression.py (right):

https://codereview.chromium.org/27165006/diff/1/tools/run-bisect-perf-regress...
tools/run-bisect-perf-regression.py:56: def _GetGOMAExecutable(path_to_goma):
I like the refactor so far, but you've got an easy opportunity here to provide
even more structure to the code if you like:

class Goma(object):

  def __init__(self, path_to_goma):
    self._path_to_goma = path_to_goma

  def __enter__(self):
    self._SetupAndStart()
    return self

  def __exit__(self, _, _, _):
    self._Stop()
  ...

with Goma(path_to_goma) as goma:  # Automatically calls enter/exit
  ...
  RunPerformanceTest()
  ...

Could even go into a new file if you end up making a directory structure for the
bisect stuff.

https://codereview.chromium.org/27165006/diff/1/tools/run-bisect-perf-regress...
tools/run-bisect-perf-regression.py:137: def _RunPerformancetest(config,
path_to_file):
Capitalize Test?

https://codereview.chromium.org/27165006/diff/1/tools/run-bisect-perf-regress...
tools/run-bisect-perf-regression.py:187: cloud_file_link = [t for t in
output.splitlines() if 'View online at' in t]
Just realized a minor wrinkle. It won't be obvious to the user which graph in
the results is ToT and which is with the patch. They will both be labeled with
the latest svn rev.

I can think of two ways to address this (maybe there are others):
1. Add a --results-label flag to Telemetry and teach the bisect script to pass
"ToT" and "Patch" or something to it.
2. Currently Telemetry labels each run with the result of
lastchange.FetchVersionInfo(None).revision. We could teach it to append a string
if the tree is dirty. For instance, return "r123456" and "r123456 - dirty".

https://codereview.chromium.org/27165006/diff/1/tools/run-bisect-perf-regress...
tools/run-bisect-perf-regression.py:195:
bisect_utils.OutputAnnotationStepStart('Results - %s' % cloud_file_link)
I think what we want is:

OutputAnnotationStepStart('Results - X.X% +/- Y.Y% delta')
print '@@@STEP_LINK@HTML Results@%s@@@' % cloud_file_link  # Factor this out to
bisect_utils

I got that syntax from
http://www.chromium.org/developers/testing/chromium-build-infrastructure/buil...
. Not sure if it is up to date given that I don't see SEED_STEP, STEP_CURSOR,
etc (which you are using).

[shatch, 2013-10-17 21:59:08]

New snapshot uploaded.

https://codereview.chromium.org/27165006/diff/1/tools/PRESUBMIT.py
File tools/PRESUBMIT.py (right):

https://codereview.chromium.org/27165006/diff/1/tools/PRESUBMIT.py#newcode5
tools/PRESUBMIT.py:5: """Top-level presubmit script for bisect trybot.
On 2013/10/17 01:01:32, tonyg wrote:
> Should we call this something other than bisect trybot now?
> 
> Given that we now have two uses for this system and we have proposed profiler
> trybots too, maybe we should come up with a general name for this system? Got
> any catchy naming ideas?

We have profiler trybots coming? The bisect bots are a bit like bloodhounds
hunting down suspects, but not sure if that metaphor would extend to
perf/profiler bots. Yeah, a catchy name would be nice but nothing jumps to mind
right now :(

https://codereview.chromium.org/27165006/diff/1/tools/bisect-perf-regression.py
File tools/bisect-perf-regression.py (right):

https://codereview.chromium.org/27165006/diff/1/tools/bisect-perf-regression....
tools/bisect-perf-regression.py:1266: r'tools\\perf\\run_' in command_to_run)
On 2013/10/17 01:01:32, tonyg wrote:
> I believe you want the r'' OR the \\s, but not both.

Done.

https://codereview.chromium.org/27165006/diff/1/tools/run-bisect-perf-regress...
File tools/run-bisect-perf-regression.py (right):

https://codereview.chromium.org/27165006/diff/1/tools/run-bisect-perf-regress...
tools/run-bisect-perf-regression.py:56: def _GetGOMAExecutable(path_to_goma):
On 2013/10/17 01:01:32, tonyg wrote:
> I like the refactor so far, but you've got an easy opportunity here to provide
> even more structure to the code if you like:
> 
> class Goma(object):
> 
>   def __init__(self, path_to_goma):
>     self._path_to_goma = path_to_goma
> 
>   def __enter__(self):
>     self._SetupAndStart()
>     return self
> 
>   def __exit__(self, _, _, _):
>     self._Stop()
>   ...
> 
> with Goma(path_to_goma) as goma:  # Automatically calls enter/exit
>   ...
>   RunPerformanceTest()
>   ...
> 
> Could even go into a new file if you end up making a directory structure for
the
> bisect stuff.

Neat, didn't know about that. Planning on making a directory structure, but
figured it was non-essential, shuttle type of work.

Done.

https://codereview.chromium.org/27165006/diff/1/tools/run-bisect-perf-regress...
tools/run-bisect-perf-regression.py:137: def _RunPerformancetest(config,
path_to_file):
On 2013/10/17 01:01:32, tonyg wrote:
> Capitalize Test?

Done.

https://codereview.chromium.org/27165006/diff/1/tools/run-bisect-perf-regress...
tools/run-bisect-perf-regression.py:187: cloud_file_link = [t for t in
output.splitlines() if 'View online at' in t]
On 2013/10/17 01:01:32, tonyg wrote:
> Just realized a minor wrinkle. It won't be obvious to the user which graph in
> the results is ToT and which is with the patch. They will both be labeled with
> the latest svn rev.
> 
> I can think of two ways to address this (maybe there are others):
> 1. Add a --results-label flag to Telemetry and teach the bisect script to pass
> "ToT" and "Patch" or something to it.
> 2. Currently Telemetry labels each run with the result of
> lastchange.FetchVersionInfo(None).revision. We could teach it to append a
string
> if the tree is dirty. For instance, return "r123456" and "r123456 - dirty".

Sure, I'll get a separate CL going to pass labels for each run and update this
CL once that's done.

https://codereview.chromium.org/27165006/diff/1/tools/run-bisect-perf-regress...
tools/run-bisect-perf-regression.py:195:
bisect_utils.OutputAnnotationStepStart('Results - %s' % cloud_file_link)
On 2013/10/17 01:01:32, tonyg wrote:
> I think what we want is:
> 
> OutputAnnotationStepStart('Results - X.X% +/- Y.Y% delta')
> print '@@@STEP_LINK@HTML Results@%s@@@' % cloud_file_link  # Factor this out
to
> bisect_utils
> 
> I got that syntax from
>
http://www.chromium.org/developers/testing/chromium-build-infrastructure/buil...
> . Not sure if it is up to date given that I don't see SEED_STEP, STEP_CURSOR,
> etc (which you are using).

Done.

[tonyg, 2013-10-18 01:00:30]

lgtm

https://codereview.chromium.org/27165006/diff/15001/tools/run-bisect-perf-reg...
File tools/run-bisect-perf-regression.py (right):

https://codereview.chromium.org/27165006/diff/15001/tools/run-bisect-perf-reg...
tools/run-bisect-perf-regression.py:32: class Goma(object):
Thanks, looks really clean :)

https://codereview.chromium.org/27165006/diff/15001/tools/run-bisect-perf-reg...
tools/run-bisect-perf-regression.py:50: return hasattr(self,
'_abs_path_to_goma')
Probably cleaner to set these to None in __init__ and then just return
bool(self._abs_path_to_goma) here.

https://codereview.chromium.org/27165006/diff/15001/tools/run-bisect-perf-reg...
tools/run-bisect-perf-regression.py:209: cloud_file_link = [t for t in
output.splitlines() if 'View online at' in t]
This is a little brittle.

Is there a better search pattern we could use like
re.match('http://cloudstorage/*')? Then 'View online at' would be free to
change.

If not, or if you don't like that idea, then please make sure Telemetry has a
comment mentioning that this text is scraped.

[shatch, 2013-10-18 17:09:50]

https://codereview.chromium.org/27165006/diff/15001/tools/run-bisect-perf-reg...
File tools/run-bisect-perf-regression.py (right):

https://codereview.chromium.org/27165006/diff/15001/tools/run-bisect-perf-reg...
tools/run-bisect-perf-regression.py:50: return hasattr(self,
'_abs_path_to_goma')
On 2013/10/18 01:00:30, tonyg wrote:
> Probably cleaner to set these to None in __init__ and then just return
> bool(self._abs_path_to_goma) here.

Done.

https://codereview.chromium.org/27165006/diff/15001/tools/run-bisect-perf-reg...
tools/run-bisect-perf-regression.py:209: cloud_file_link = [t for t in
output.splitlines() if 'View online at' in t]
On 2013/10/18 01:00:30, tonyg wrote:
> This is a little brittle.
> 
> Is there a better search pattern we could use like
> re.match('http://cloudstorage/*')? Then 'View online at' would be free to
> change.
> 
> If not, or if you don't like that idea, then please make sure Telemetry has a
> comment mentioning that this text is scraped.

Changed it to search for storage.googleapis.com/chromium-telemetry/html-results,
which seems like it would be pretty reliable.

[tonyg, 2013-10-18 18:24:37]

lgtm

[commit-bot: I haz the power, 2013-10-21 20:32:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonhatch@chromium.org/27165006/31001

[commit-bot: I haz the power, 2013-10-22 00:10:25]

Change committed as 229963