Add graphing for multiple runs of bench.

[bungeman, 2011-06-16 13:35:07]

This is the new bench_compare and the new bench graph. While looking at this one
may wish to think about what should be added or changed for this to better
display information from the build automatically.

[epoger, 2011-06-16 13:55:38]

Big picture looks good.  I have a few questions/suggestions below.

http://codereview.appspot.com/4539087/diff/15001/bench/bench_compare.py
File bench/bench_compare.py (right):

http://codereview.appspot.com/4539087/diff/15001/bench/bench_compare.py#newco...
bench/bench_compare.py:27: """A compare between data points produced by
bench."""
Please document the parameters.  (Python's wonderful practice of not declaring
parameter types keeps me in the dark.)

http://codereview.appspot.com/4539087/diff/15001/bench/bench_compare.py#newco...
bench/bench_compare.py:119: new_bench_match = [bench for bench in new_benches
How does this statement work?  It's confusing to me.

http://codereview.appspot.com/4539087/diff/15001/bench/bench_graph_svg.py
File bench/bench_graph_svg.py (right):

http://codereview.appspot.com/4539087/diff/15001/bench/bench_graph_svg.py#new...
bench/bench_graph_svg.py:30: """The information in a label."""
please document parameters

http://codereview.appspot.com/4539087/diff/15001/bench/bench_graph_svg.py#new...
bench/bench_graph_svg.py:38: return ("Label("
why not % notation to assemble this string?

http://codereview.appspot.com/4539087/diff/15001/bench/bench_graph_svg.py#new...
bench/bench_graph_svg.py:43: def __str__(self):
please add a newline before the method

http://codereview.appspot.com/4539087/diff/15001/bench/bench_graph_svg.py#new...
bench/bench_graph_svg.py:44: return (str(self.bench)
why not % notation?

http://codereview.appspot.com/4539087/diff/15001/bench/bench_graph_svg.py#new...
bench/bench_graph_svg.py:54: def __hash__(self):
please add a newline before the method

http://codereview.appspot.com/4539087/diff/15001/bench/bench_graph_svg.py#new...
bench/bench_graph_svg.py:86: """Takes <old>(:<new>)? returns (old, new) or (old,
Max)."""
I don't understand the questionmark here.  Maybe you meant '&'?

http://codereview.appspot.com/4539087/diff/15001/bench/bench_graph_svg.py#new...
bench/bench_graph_svg.py:93: """Takes <key>(=<value>)? adds {key:value} or
{key:True} to settings."""
I don't understand the questionmark here.  Maybe you meant '&'?

http://codereview.appspot.com/4539087/diff/15001/bench/bench_graph_svg.py#new...
bench/bench_graph_svg.py:133: #pull all the data from the files in the directory
any reasonable way you could break this section, and sections below, into
separate functions/methods? main() is 527 lines long...

http://codereview.appspot.com/4539087/diff/15001/bench/bench_util.py
File bench/bench_util.py (right):

http://codereview.appspot.com/4539087/diff/15001/bench/bench_util.py#newcode20
bench/bench_util.py:20: return ("BenchDataPoint("
why not % representation?

[bungeman, 2011-06-16 18:54:09]

http://codereview.appspot.com/4539087/diff/15001/bench/bench_compare.py
File bench/bench_compare.py (right):

http://codereview.appspot.com/4539087/diff/15001/bench/bench_compare.py#newco...
bench/bench_compare.py:119: new_bench_match = [bench for bench in new_benches
On 2011/06/16 13:55:38, epoger wrote:
> How does this statement work?  It's confusing to me.

http://docs.python.org/tutorial/datastructures.html#list-comprehensions

http://codereview.appspot.com/4539087/diff/15001/bench/bench_graph_svg.py
File bench/bench_graph_svg.py (right):

http://codereview.appspot.com/4539087/diff/15001/bench/bench_graph_svg.py#new...
bench/bench_graph_svg.py:38: return ("Label("
On 2011/06/16 13:55:38, epoger wrote:
> why not % notation to assemble this string?

I wrote this before I discovered % notation. It would make this somewhat
simpler.

http://codereview.appspot.com/4539087/diff/15001/bench/bench_graph_svg.py#new...
bench/bench_graph_svg.py:86: """Takes <old>(:<new>)? returns (old, new) or (old,
Max)."""
On 2011/06/16 13:55:38, epoger wrote:
> I don't understand the questionmark here.  Maybe you meant '&'?

Regular expression syntax, the (:<new>) is optional. Being part of parsing
command arguments, perhaps '<old>[:<new>]' would be easier to understand?

[bungeman, 2011-06-16 18:57:31]

This addresses most of the comments. The main function is still fairly large. It
might be some work to break up what remains, as it is generating one big page
where many parts depend on other parts.

[epoger, 2011-06-16 21:01:19]

LGTM

A couple of minor suggestions, but generally looks quite good. Thanks for the
cleanup!

http://codereview.appspot.com/4539087/diff/21001/bench/bench_compare.py
File bench/bench_compare.py (right):

http://codereview.appspot.com/4539087/diff/21001/bench/bench_compare.py#newco...
bench/bench_compare.py:29: (BenchDataPoint, BenchDataPoint)"""
Thanks.  For future reference, I am a fan of the argument documentation style
described within the "Functions and Methods" section of
http://www.corp.google.com/eng/doc/pyguide.xml#Comments .  But I don't think
it's essential to change the documentation for this small method.

http://codereview.appspot.com/4539087/diff/21001/bench/bench_compare.py#newco...
bench/bench_compare.py:122: new_bench_match = [bench for bench in new_benches
Maybe add a comment like this beforehand:

filter new_benches for items that pass the conditional below

http://codereview.appspot.com/4539087/diff/26001/bench/bench_graph_svg.py
File bench/bench_graph_svg.py (right):

http://codereview.appspot.com/4539087/diff/26001/bench/bench_graph_svg.py#new...
bench/bench_graph_svg.py:113: and not time_of_interest == point.time_type):
maybe move the "and" up to the previous line just for consistency

[bungeman, 2011-06-17 13:59:38]

Committed revision 1628.

This also updates bench_compare.py so that it works with the new bench output
(and sorts).