Fix bisect script to match a results with {mean, stddev} format.

Fix bisect script to match a results with {mean, stddev} format.

Make bisects to look for the pattern RESULT lines with  {mean, stddev} format while parsing results log.

BUG=366431
NOTRY=true

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

[qyearsley, 2014-05-14 22:28:33]

Before this committed, I think it would definitely be a good idea to add a unit
test case to verify that we're matching all the results that we want to. (I
think that the function right now may not match exactly what we want it to...)

Also, does anybody know who invented this output format? I can't quickly find
anything about the "RESULT line" format we're using. (Telemetry just calls it
the "buildbot" format...)

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

https://codereview.chromium.org/288843002/diff/1/tools/bisect-perf-regression...
tools/bisect-perf-regression.py:1979: # <*>RESULT <graph_name>: <trace_name>=
{<mean>, <std deviation>}
It would probably be neater/more readable to put these three regular expressions
up at the top (before the loop), and avoid nesting the else expressions. For
example:

single_result_re = metric_re + '\s*(?P<VALUE>[0-9]+(\.[0-9]*)?)'
multi_results_re = metric_re + '\s*\[\s*(?P<VALUES>[0-9,.]+)\]'
mean_stddev_re = (
    metric_re +
    '\s*\{\s*(?P<MEAN>[0-9]+(\.[0-9]*)?),\s*' +
    '(?P<STDDEV>[0-9]+(\.[0-9]*)?)\}')

single_result_match = re.search(single_result_re, current_line)
multi_results_match = re.search(multi_results_re, current_line)
mean_stddev_match = re.search(mean_stddev_re, current_line)

if single_result_match:
  # ...
elif multi_results_match:
  # ...
elif mean_stddev_match:
  # ...
else:
  # ...

Looking more closely at these regular expressions, I have a few questions:
 (1) For single result lines and square-bracket value list result lines, are we
not allowing negative numbers? Most tests don't output negative numbers but some
might and I think it's valid.
 (2) It looks like we're allowing whitespace after the opening bracket but NOT
before the closing bracket. Is there a reason for this? i.e. we're allowing [
1,2] but not [1,2 ]. (Might as well allow space before the closing bracket as
well).
 (3) The second regex is a too accepting. It would accept:
     [123.123.123,.3..3] or [.]
     (But it would not accept: [1, 2, 3] or [-1,2].

[shatch, 2014-05-14 23:01:41]

On 2014/05/14 22:28:33, qyearsley wrote:
> Before this committed, I think it would definitely be a good idea to add a
unit
> test case to verify that we're matching all the results that we want to. (I
> think that the function right now may not match exactly what we want it to...)
> 
> Also, does anybody know who invented this output format? I can't quickly find
> anything about the "RESULT line" format we're using. (Telemetry just calls it
> the "buildbot" format...)
> 
>
https://codereview.chromium.org/288843002/diff/1/tools/bisect-perf-regression.py
> File tools/bisect-perf-regression.py (right):
> 
>
https://codereview.chromium.org/288843002/diff/1/tools/bisect-perf-regression...
> tools/bisect-perf-regression.py:1979: # <*>RESULT <graph_name>: <trace_name>=
> {<mean>, <std deviation>}
> It would probably be neater/more readable to put these three regular
expressions
> up at the top (before the loop), and avoid nesting the else expressions. For
> example:
> 
> single_result_re = metric_re + '\s*(?P<VALUE>[0-9]+(\.[0-9]*)?)'
> multi_results_re = metric_re + '\s*\[\s*(?P<VALUES>[0-9,.]+)\]'
> mean_stddev_re = (
>     metric_re +
>     '\s*\{\s*(?P<MEAN>[0-9]+(\.[0-9]*)?),\s*' +
>     '(?P<STDDEV>[0-9]+(\.[0-9]*)?)\}')
> 
> single_result_match = re.search(single_result_re, current_line)
> multi_results_match = re.search(multi_results_re, current_line)
> mean_stddev_match = re.search(mean_stddev_re, current_line)
> 
> if single_result_match:
>   # ...
> elif multi_results_match:
>   # ...
> elif mean_stddev_match:
>   # ...
> else:
>   # ...
> 
> Looking more closely at these regular expressions, I have a few questions:
>  (1) For single result lines and square-bracket value list result lines, are
we
> not allowing negative numbers? Most tests don't output negative numbers but
some
> might and I think it's valid.
>  (2) It looks like we're allowing whitespace after the opening bracket but NOT
> before the closing bracket. Is there a reason for this? i.e. we're allowing [
> 1,2] but not [1,2 ]. (Might as well allow space before the closing bracket as
> well).
>  (3) The second regex is a too accepting. It would accept:
>      [123.123.123,.3..3] or [.]
>      (But it would not accept: [1, 2, 3] or [-1,2].

Happy when Quinten is happy with this.

+1 to adding a test and simplifying the method.

[prasadv, 2014-05-15 19:49:09]

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

https://codereview.chromium.org/288843002/diff/1/tools/bisect-perf-regression...
tools/bisect-perf-regression.py:1979: # <*>RESULT <graph_name>: <trace_name>=
{<mean>, <std deviation>}
Agreed, tried to trim this method and make it more readable.
Also added tests for each of the results patterns.

On 2014/05/14 22:28:33, qyearsley wrote:
> It would probably be neater/more readable to put these three regular
expressions
> up at the top (before the loop), and avoid nesting the else expressions. For
> example:
> 
> single_result_re = metric_re + '\s*(?P<VALUE>[0-9]+(\.[0-9]*)?)'
> multi_results_re = metric_re + '\s*\[\s*(?P<VALUES>[0-9,.]+)\]'
> mean_stddev_re = (
>     metric_re +
>     '\s*\{\s*(?P<MEAN>[0-9]+(\.[0-9]*)?),\s*' +
>     '(?P<STDDEV>[0-9]+(\.[0-9]*)?)\}')
> 
> single_result_match = re.search(single_result_re, current_line)
> multi_results_match = re.search(multi_results_re, current_line)
> mean_stddev_match = re.search(mean_stddev_re, current_line)
> 
> if single_result_match:
>   # ...
> elif multi_results_match:
>   # ...
> elif mean_stddev_match:
>   # ...
> else:
>   # ...
> 
> Looking more closely at these regular expressions, I have a few questions:
>  (1) For single result lines and square-bracket value list result lines, are
we
> not allowing negative numbers? Most tests don't output negative numbers but
some
> might and I think it's valid.
>  (2) It looks like we're allowing whitespace after the opening bracket but NOT
> before the closing bracket. Is there a reason for this? i.e. we're allowing [
> 1,2] but not [1,2 ]. (Might as well allow space before the closing bracket as
> well).
>  (3) The second regex is a too accepting. It would accept:
>      [123.123.123,.3..3] or [.]
>      (But it would not accept: [1, 2, 3] or [-1,2].

Done.

[qyearsley, 2014-05-15 19:52:26]

LGTM :-)

[prasadv, 2014-05-15 19:52:49]

The CQ bit was checked by prasadv@chromium.org

[commit-bot: I haz the power, 2014-05-15 19:53:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/prasadv@chromium.org/288843002/20001

[commit-bot: I haz the power, 2014-05-15 19:56:25]

Change committed as 270788