gen_bench_expectations to generate bench expectations files.

gen_bench_expectations to generate bench expectations files.

BUG=skia:2225
NOTRY=true

Committed: http://code.google.com/p/skia/source/detail?r=13838

[benchen, 2014-03-17 15:06:20]

try to get this in place first.

[borenet, 2014-03-17 17:43:05]

https://codereview.chromium.org/201423002/diff/20001/bench/bench_util.py
File bench/bench_util.py (right):

https://codereview.chromium.org/201423002/diff/20001/bench/bench_util.py#newc...
bench/bench_util.py:151: def parse_skp_bench_data(directory, revision, rep,
default_settings={}):
Using an instance as a default is bad news.  Instead, set it to None and do:

if not default_settings:
  default_settings = {}

https://codereview.chromium.org/201423002/diff/20001/bench/bench_util.py#newc...
bench/bench_util.py:176: file_handle = open(directory + '/' + bench_file, 'r')
Please use the "with" syntax:

with open('/'.join(directory, bench_file), 'r') as file_handle:
  ...

https://codereview.chromium.org/201423002/diff/20001/bench/bench_util.py#newc...
bench/bench_util.py:178: default_settings['scalar'] = scalar_type
Should we be modifying something that the caller has passed in?  If so, we need
to document that this is happening.  Otherwise, let's make a copy or avoid
modifying it.

https://codereview.chromium.org/201423002/diff/20001/bench/gen_bench_expectat...
File bench/gen_bench_expectations.py (right):

https://codereview.chromium.org/201423002/diff/20001/bench/gen_bench_expectat...
bench/gen_bench_expectations.py:56: if (point.time_type or  # Not walltime which
has time_type ''
This seems confusing to me, but I guess it's okay since we're already using this
format...

https://codereview.chromium.org/201423002/diff/20001/bench/gen_bench_expectat...
bench/gen_bench_expectations.py:102: lower_bound, upper_bound))
Can we use a format dict here for clarity?

'%(bench)s_%(config)s_,%(builder)s-%(representation)s,%(expected)s,%(lower_bound)s,%(upper_bound)s'
% {
  'bench': bench,
  ....
}

[benchen, 2014-03-17 19:02:40]

Thanks for the quick review! PTAL.

https://codereview.chromium.org/201423002/diff/20001/bench/bench_util.py
File bench/bench_util.py (right):

https://codereview.chromium.org/201423002/diff/20001/bench/bench_util.py#newc...
bench/bench_util.py:151: def parse_skp_bench_data(directory, revision, rep,
default_settings={}):
On 2014/03/17 17:43:05, borenet wrote:
> Using an instance as a default is bad news.  Instead, set it to None and do:
> 
> if not default_settings:
>   default_settings = {}

Done.

https://codereview.chromium.org/201423002/diff/20001/bench/bench_util.py#newc...
bench/bench_util.py:176: file_handle = open(directory + '/' + bench_file, 'r')
ah just copied it over. Done.
On 2014/03/17 17:43:05, borenet wrote:
> Please use the "with" syntax:
> 
> with open('/'.join(directory, bench_file), 'r') as file_handle:
>   ...

https://codereview.chromium.org/201423002/diff/20001/bench/bench_util.py#newc...
bench/bench_util.py:178: default_settings['scalar'] = scalar_type
Good suggestion. Done.
On 2014/03/17 17:43:05, borenet wrote:
> Should we be modifying something that the caller has passed in?  If so, we
need
> to document that this is happening.  Otherwise, let's make a copy or avoid
> modifying it.

https://codereview.chromium.org/201423002/diff/20001/bench/gen_bench_expectat...
File bench/gen_bench_expectations.py (right):

https://codereview.chromium.org/201423002/diff/20001/bench/gen_bench_expectat...
bench/gen_bench_expectations.py:56: if (point.time_type or  # Not walltime which
has time_type ''
this is legacy format. I think it meant "default". will leave as is now.
On 2014/03/17 17:43:05, borenet wrote:
> This seems confusing to me, but I guess it's okay since we're already using
this
> format...

https://codereview.chromium.org/201423002/diff/20001/bench/gen_bench_expectat...
bench/gen_bench_expectations.py:102: lower_bound, upper_bound))
On 2014/03/17 17:43:05, borenet wrote:
> Can we use a format dict here for clarity?
> 
>
'%(bench)s_%(config)s_,%(builder)s-%(representation)s,%(expected)s,%(lower_bound)s,%(upper_bound)s'
> % {
>   'bench': bench,
>   ....
> }

Done.

[borenet, 2014-03-17 19:14:41]

Just a few more comments.

https://codereview.chromium.org/201423002/diff/40001/bench/bench_util.py
File bench/bench_util.py (right):

https://codereview.chromium.org/201423002/diff/40001/bench/bench_util.py#newc...
bench/bench_util.py:158: default_settings: dictionary of other run settings.
Can you give some examples of what these run settings might be?

https://codereview.chromium.org/201423002/diff/40001/bench/bench_util.py#newc...
bench/bench_util.py:180: settings = default_settings
This will point to the same object and will therefore still modify the input.  I
suggest something like:
settings = dict(default_settings or {})

And then you can remove the "if not default_settings" above.

[benchen, 2014-03-17 19:37:54]

please see replies. Thanks!

https://codereview.chromium.org/201423002/diff/40001/bench/bench_util.py
File bench/bench_util.py (right):

https://codereview.chromium.org/201423002/diff/40001/bench/bench_util.py#newc...
bench/bench_util.py:158: default_settings: dictionary of other run settings.
Added some. Seems like this is currently only used by microbenches, but we may
activate for picture benches, or add microbench in parsing later. Shall I remove
it for now?
On 2014/03/17 19:14:42, borenet wrote:
> Can you give some examples of what these run settings might be?

https://codereview.chromium.org/201423002/diff/40001/bench/bench_util.py#newc...
bench/bench_util.py:180: settings = default_settings
ah right. fixed.
On 2014/03/17 19:14:42, borenet wrote:
> This will point to the same object and will therefore still modify the input. 
I
> suggest something like:
> settings = dict(default_settings or {})
> 
> And then you can remove the "if not default_settings" above.

[borenet, 2014-03-17 20:07:25]

LGTM once the settings are addressed.

https://codereview.chromium.org/201423002/diff/40001/bench/bench_util.py
File bench/bench_util.py (right):

https://codereview.chromium.org/201423002/diff/40001/bench/bench_util.py#newc...
bench/bench_util.py:158: default_settings: dictionary of other run settings.
On 2014/03/17 19:37:54, benchen wrote:
> Added some. Seems like this is currently only used by microbenches, but we may
> activate for picture benches, or add microbench in parsing later. Shall I
remove
> it for now?
> On 2014/03/17 19:14:42, borenet wrote:
> > Can you give some examples of what these run settings might be?
> 

hmm.. Are all of the acceptable settings documented somewhere?  Maybe the best
thing to do is to point to that, if it exists.  I'll leave it up to you whether
to remove it or point to more documentation.

[benchen, 2014-03-17 21:16:09]

The CQ bit was checked by bensong@google.com

[commit-bot: I haz the power, 2014-03-17 21:16:18]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/bensong@google.com/201423002/80001

[commit-bot: I haz the power, 2014-03-17 21:16:34]

Change committed as 13838

[benchen, 2014-03-17 21:17:20]

On 2014/03/17 20:07:25, borenet wrote:
> LGTM once the settings are addressed.
> 
> https://codereview.chromium.org/201423002/diff/40001/bench/bench_util.py
> File bench/bench_util.py (right):
> 
>
https://codereview.chromium.org/201423002/diff/40001/bench/bench_util.py#newc...
> bench/bench_util.py:158: default_settings: dictionary of other run settings.
> On 2014/03/17 19:37:54, benchen wrote:
> > Added some. Seems like this is currently only used by microbenches, but we
may
> > activate for picture benches, or add microbench in parsing later. Shall I
> remove
> > it for now?
> > On 2014/03/17 19:14:42, borenet wrote:
> > > Can you give some examples of what these run settings might be?
> > 
> 
> hmm.. Are all of the acceptable settings documented somewhere?  Maybe the best
> thing to do is to point to that, if it exists.  I'll leave it up to you
whether
> to remove it or point to more documentation.

Done. pointed to the benchmain section.