Changes bench to output with git hash filename; cleans up bench_graph_svg to only handle bench aler…

Changes bench to output with git hash filename; cleans up bench_graph_svg to only handle bench alerts; renames it to check_bench_regressions.
Committed: https://code.google.com/p/skia/source/detail?r=11765

[benchen, 2013-10-10 21:01:10]

This replaces 26481004 and aims for taking care of the whole transition. Thanks.

[borenet, 2013-10-11 17:56:28]

Thanks for the redo Ben.  You should be aware that the bots sync the buildbot
code and core Skia code separately, and the buildbot code is always synced to
tip-of-tree while Skia is synced to the requested revision.  This means that
some builds may fail if they are staggered.  I *think* everything should work
fine if there are no builds running when you submit this.

https://codereview.chromium.org/26592010/diff/8001/buildbot/slave/skia_slave_...
File buildbot/slave/skia_slave_scripts/check_for_regressions.py (right):

https://codereview.chromium.org/26592010/diff/8001/buildbot/slave/skia_slave_...
buildbot/slave/skia_slave_scripts/check_for_regressions.py:28: print 'Skip due
to missing expectations: %s' % path_to_bench_expectations
Nothing to do with your change, but this should be an error sometime soon...

https://codereview.chromium.org/26592010/diff/8001/trunk/bench/check_bench_re...
File trunk/bench/check_bench_regressions.py (right):

https://codereview.chromium.org/26592010/diff/8001/trunk/bench/check_bench_re...
trunk/bench/check_bench_regressions.py:23: print '-b <builder> string to extract
builder name from buildbot slaves.'
I think this explanation could be clearer. What about something like, "name of
the builder whose bench data we're checking"?

https://codereview.chromium.org/26592010/diff/8001/trunk/bench/check_bench_re...
trunk/bench/check_bench_regressions.py:83: else:
It's not clear what you're doing here.  It looks like we assume that a revision
with more than 5 characters is a git commit hash and it's an svn revision
otherwise?  It seems like we could do more checking, for example we know that
the svn revision is also all numeric characters.  I'm also confused by the 7. 
Is that supposed to be the length of "bench_r"? If so, scalar_type will be wrong
if we're given a git commit hash, since "bench_" is 6 characters.  Can you
replace the magic numbers, either with well-explained constants or something
like len('bench_r')?  And please add comments to explain what's going on.

Example might be:

# Maximum expected number of characters we expect in an svn revision.
MAX_SVN_REV_LENGTH = 5

.....

if (len(revision) > MAX_SVN_REV_LENGTH and
    bench_file.startswith('bench_' + revision + '_')):
  scalar_type = bench_file[len(revision) + len('bench_'):]
elif (bench_file.startswith('bench_r' + revision + '_') and
      stringIsAllNumbers(revision)):
  scalar_type = bench_file[len(revision) + len('bench_r'):]
else:
  continue

https://codereview.chromium.org/26592010/diff/8001/trunk/bench/check_bench_re...
trunk/bench/check_bench_regressions.py:110: bench_dict[point_name] = point.time
Are we worried about overwriting data?  Do we ever get multiple lines of data
with the same label?

https://codereview.chromium.org/26592010/diff/8001/trunk/bench/check_bench_re...
trunk/bench/check_bench_regressions.py:131: def read_expectations(expectations,
filename):
Is there any reason that this function and check_expectations need to be defined
inside main()?  It seems less clear this way.

[benchen, 2013-10-12 01:01:19]

Thanks Eric. Please take another look. I'll be careful when committing this -
maybe sometime before going to bed next week.

https://codereview.chromium.org/26592010/diff/8001/buildbot/slave/skia_slave_...
File buildbot/slave/skia_slave_scripts/check_for_regressions.py (right):

https://codereview.chromium.org/26592010/diff/8001/buildbot/slave/skia_slave_...
buildbot/slave/skia_slave_scripts/check_for_regressions.py:28: print 'Skip due
to missing expectations: %s' % path_to_bench_expectations
OK, thx.
On 2013/10/11 17:56:29, borenet wrote:
> Nothing to do with your change, but this should be an error sometime soon...

https://codereview.chromium.org/26592010/diff/8001/trunk/bench/check_bench_re...
File trunk/bench/check_bench_regressions.py (right):

https://codereview.chromium.org/26592010/diff/8001/trunk/bench/check_bench_re...
trunk/bench/check_bench_regressions.py:23: print '-b <builder> string to extract
builder name from buildbot slaves.'
On 2013/10/11 17:56:29, borenet wrote:
> I think this explanation could be clearer. What about something like, "name of
> the builder whose bench data we're checking"?

Done.

https://codereview.chromium.org/26592010/diff/8001/trunk/bench/check_bench_re...
trunk/bench/check_bench_regressions.py:83: else:
Agreed. Done.
On 2013/10/11 17:56:29, borenet wrote:
> It's not clear what you're doing here.  It looks like we assume that a
revision
> with more than 5 characters is a git commit hash and it's an svn revision
> otherwise?  It seems like we could do more checking, for example we know that
> the svn revision is also all numeric characters.  I'm also confused by the 7. 
> Is that supposed to be the length of "bench_r"? If so, scalar_type will be
wrong
> if we're given a git commit hash, since "bench_" is 6 characters.  Can you
> replace the magic numbers, either with well-explained constants or something
> like len('bench_r')?  And please add comments to explain what's going on.
> 
> Example might be:
> 
> # Maximum expected number of characters we expect in an svn revision.
> MAX_SVN_REV_LENGTH = 5
> 
> .....
> 
> if (len(revision) > MAX_SVN_REV_LENGTH and
>     bench_file.startswith('bench_' + revision + '_')):
>   scalar_type = bench_file[len(revision) + len('bench_'):]
> elif (bench_file.startswith('bench_r' + revision + '_') and
>       stringIsAllNumbers(revision)):
>   scalar_type = bench_file[len(revision) + len('bench_r'):]
> else:
>   continue

https://codereview.chromium.org/26592010/diff/8001/trunk/bench/check_bench_re...
trunk/bench/check_bench_regressions.py:110: bench_dict[point_name] = point.time
My intention was to always use the first one if there are duplicates. But on
second thought, this should raise an exception. Added codes to do that. Thanks.
On 2013/10/11 17:56:29, borenet wrote:
> Are we worried about overwriting data?  Do we ever get multiple lines of data
> with the same label?

https://codereview.chromium.org/26592010/diff/8001/trunk/bench/check_bench_re...
trunk/bench/check_bench_regressions.py:131: def read_expectations(expectations,
filename):
The reason is that bungeman started it :) Now that there are no other embedded
functions, I moved them out as suggested. Done.
On 2013/10/11 17:56:29, borenet wrote:
> Is there any reason that this function and check_expectations need to be
defined
> inside main()?  It seems less clear this way.

[borenet, 2013-10-14 12:48:01]

This is looking good.  Just a couple of minor comments.

https://codereview.chromium.org/26592010/diff/12001/buildbot/slave/skia_slave...
File buildbot/slave/skia_slave_scripts/bench_pictures.py (right):

https://codereview.chromium.org/26592010/diff/12001/buildbot/slave/skia_slave...
buildbot/slave/skia_slave_scripts/bench_pictures.py:9: from run_bench import
BenchArgs, GetSvnRevision
Need to remove this reference to GetSvnRevision.

https://codereview.chromium.org/26592010/diff/12001/trunk/bench/check_bench_r...
File trunk/bench/check_bench_regressions.py (right):

https://codereview.chromium.org/26592010/diff/12001/trunk/bench/check_bench_r...
trunk/bench/check_bench_regressions.py:32: print 'bench values.'
Nit: Please indent "bench values" to match the other lines.

https://codereview.chromium.org/26592010/diff/12001/trunk/bench/check_bench_r...
trunk/bench/check_bench_regressions.py:81: scalar_type = None
I might also add a short comment explaining that this just determines whether
we're dealing with a commit hash or revision number and pulls out the config
accordingly.

[benchen, 2013-10-14 19:10:28]

Thank you Eric! Addressed all comments in patch 4. Please take another look.

https://codereview.chromium.org/26592010/diff/12001/buildbot/slave/skia_slave...
File buildbot/slave/skia_slave_scripts/bench_pictures.py (right):

https://codereview.chromium.org/26592010/diff/12001/buildbot/slave/skia_slave...
buildbot/slave/skia_slave_scripts/bench_pictures.py:9: from run_bench import
BenchArgs, GetSvnRevision
On 2013/10/14 12:48:01, borenet wrote:
> Need to remove this reference to GetSvnRevision.

Done.

https://codereview.chromium.org/26592010/diff/12001/trunk/bench/check_bench_r...
File trunk/bench/check_bench_regressions.py (right):

https://codereview.chromium.org/26592010/diff/12001/trunk/bench/check_bench_r...
trunk/bench/check_bench_regressions.py:32: print 'bench values.'
On 2013/10/14 12:48:01, borenet wrote:
> Nit: Please indent "bench values" to match the other lines.

Done.

https://codereview.chromium.org/26592010/diff/12001/trunk/bench/check_bench_r...
trunk/bench/check_bench_regressions.py:81: scalar_type = None
On 2013/10/14 12:48:01, borenet wrote:
> I might also add a short comment explaining that this just determines whether
> we're dealing with a commit hash or revision number and pulls out the config
> accordingly.

Done.

[borenet, 2013-10-14 19:22:46]

LGTM

[benchen, 2013-10-15 00:08:25]

Committed patchset #5 manually as r11765 (presubmit successful).