Parses per-tile benches and returns the sum as overall skp bench.

Parses per-tile benches and returns the sum as overall skp bench.
Committed: https://code.google.com/p/skia/source/detail?r=6884

[benchen, 2012-12-18 16:03:23]

Tried gcl upload with -m and -t, and gcl change, but none changed the top level
issue title. So edited the issue instead.
From previous patches, changed to using sum() instead of max() of per-tile data
as overall bench.

[epoger, 2012-12-18 19:29:39]

Seems fine except for the representation naming ('avg' appearing all over the
place).  Suggestion below...

https://codereview.appspot.com/6940071/diff/3002/bench/bench_util.py
File bench/bench_util.py (right):

https://codereview.appspot.com/6940071/diff/3002/bench/bench_util.py#newcode1
bench/bench_util.py:1: '''
Regarding "Tried gcl upload with -m and -t, and gcl change, but none changed the
top level issue title. So edited the issue instead."... yes, as far as I know
you have to "Edit Issue" to change the issue title.  The other options just
label particular patchsets, as you discovered.

https://codereview.appspot.com/6940071/diff/3002/bench/bench_util.py#newcode86
bench/bench_util.py:86: representation: should match one given in class
_ListAlgorithm."""
I think it would be safer to define a set of constants like this, and use them
throughout instead of inline strings:

ALGORITHM_AVERAGE = 'avg'
ALGORITHM_MEDIAN = 'med'
etc.

Then, this can say something like

representation: one of the ALGORITHM_xxx representation types

https://codereview.appspot.com/6940071/diff/3002/bench/bench_util.py#newcode101
bench/bench_util.py:101: def parse(settings, lines, representation='avg'):
Again, it makes me nervous to have these repeated instances of the same string
value.  Please set to some ALGORITHM_xxx constant.

Even better, perhaps, why do we need to set the default to 'avg' in so many
places?  Couldn't it just default to None, and thus receive the default value in
_ListAlgorithm.init() ?

https://codereview.appspot.com/6940071/diff/3002/bench/bench_util.py#newcode109
bench/bench_util.py:109: bench_dic = {}  # [bench][config][time_type] -> [list
of bench values]
Thanks for the comment

https://codereview.appspot.com/6940071/diff/3002/bench/bench_util.py#newcode112
bench/bench_util.py:112: bench_re = 'running bench (?:\[\d+ \d+\] )?\s*(\S+)'
Nothing new here, but I am generally concerned about fragility of this code in
the event that someone makes a seemingly insignificant change to the bench
output.

Is it time to make bench output something better-structured, like JSON?  (No
need to block this CL, but I think we need to consider it soon.)

[benchen, 2012-12-18 19:50:12]

Thanks for the suggestion. Yes I didn't realize there was 'avg' everywhere..
fixed.

https://codereview.appspot.com/6940071/diff/3002/bench/bench_util.py
File bench/bench_util.py (right):

https://codereview.appspot.com/6940071/diff/3002/bench/bench_util.py#newcode86
bench/bench_util.py:86: representation: should match one given in class
_ListAlgorithm."""
On 2012/12/18 19:29:39, epoger wrote:
> I think it would be safer to define a set of constants like this, and use them
> throughout instead of inline strings:
> 
> ALGORITHM_AVERAGE = 'avg'
> ALGORITHM_MEDIAN = 'med'
> etc.
> 
> Then, this can say something like
> 
> representation: one of the ALGORITHM_xxx representation types

Done.

https://codereview.appspot.com/6940071/diff/3002/bench/bench_util.py#newcode101
bench/bench_util.py:101: def parse(settings, lines, representation='avg'):
Good suggestion. Done.
On 2012/12/18 19:29:39, epoger wrote:
> Again, it makes me nervous to have these repeated instances of the same string
> value.  Please set to some ALGORITHM_xxx constant.
> 
> Even better, perhaps, why do we need to set the default to 'avg' in so many
> places?  Couldn't it just default to None, and thus receive the default value
in
> _ListAlgorithm.init() ?

https://codereview.appspot.com/6940071/diff/3002/bench/bench_util.py#newcode109
bench/bench_util.py:109: bench_dic = {}  # [bench][config][time_type] -> [list
of bench values]
On 2012/12/18 19:29:39, epoger wrote:
> Thanks for the comment

Done.

https://codereview.appspot.com/6940071/diff/3002/bench/bench_util.py#newcode112
bench/bench_util.py:112: bench_re = 'running bench (?:\[\d+ \d+\] )?\s*(\S+)'
Agree. Also long term wise, we can just write the benches to a database, like
app engine datastore. Currently the main blocking factor is app engine app
security, and the speed team is submitting something for security review.
On 2012/12/18 19:29:39, epoger wrote:
> Nothing new here, but I am generally concerned about fragility of this code in
> the event that someone makes a seemingly insignificant change to the bench
> output.
> 
> Is it time to make bench output something better-structured, like JSON?  (No
> need to block this CL, but I think we need to consider it soon.)

[epoger, 2012-12-18 20:00:35]

LGTM with suggestions

https://codereview.appspot.com/6940071/diff/9001/bench/bench_util.py
File bench/bench_util.py (right):

https://codereview.appspot.com/6940071/diff/9001/bench/bench_util.py#newcode14
bench/bench_util.py:14: ALGORITHM_25TH = '25th'
maybe ALGORITHM_25TH_PERCENTILE ?

https://codereview.appspot.com/6940071/diff/9001/bench/bench_util.py#newcode83
bench/bench_util.py:83: def _ParseAndStoreTimes(config_re, time_re, line, bench,
dic, representation):
maybe representation=None and the default will be applied in
_ListAlgorithm.init() ?

https://codereview.appspot.com/6940071/diff/9001/bench/bench_util.py#newcode105
bench/bench_util.py:105: def parse(settings, lines, representation):
maybe representation=None and the default will be applied in
_ListAlgorithm.init() ?

[benchen, 2012-12-18 20:05:55]

Done, thanks.
Submitting since this round the changes are minor.

https://codereview.appspot.com/6940071/diff/9001/bench/bench_util.py
File bench/bench_util.py (right):

https://codereview.appspot.com/6940071/diff/9001/bench/bench_util.py#newcode14
bench/bench_util.py:14: ALGORITHM_25TH = '25th'
On 2012/12/18 20:00:36, epoger wrote:
> maybe ALGORITHM_25TH_PERCENTILE ?

Done.

https://codereview.appspot.com/6940071/diff/9001/bench/bench_util.py#newcode83
bench/bench_util.py:83: def _ParseAndStoreTimes(config_re, time_re, line, bench,
dic, representation):
On 2012/12/18 20:00:36, epoger wrote:
> maybe representation=None and the default will be applied in
> _ListAlgorithm.init() ?

Done.

https://codereview.appspot.com/6940071/diff/9001/bench/bench_util.py#newcode105
bench/bench_util.py:105: def parse(settings, lines, representation):
On 2012/12/18 20:00:36, epoger wrote:
> maybe representation=None and the default will be applied in
> _ListAlgorithm.init() ?

Done.

[epoger, 2012-12-18 20:22:13]

On 2012/12/18 20:05:55, benchen wrote:
> Done, thanks.
> Submitting since this round the changes are minor.

Sounds good.  Whenever I put LGTM, I mean I am in favor of committing it whether
you make the noted changes or not, so no need to wait for another LG.

I don't know if this is what everyone means when they put LG with comments, but
I'm willing to assume so. :-)