Detect perf regressions and speedups automatically....

Detect perf regressions and speedups automatically.

Compare the results we get with the results present
in perf_expectations.json.  Update the summary box
according to which result we encounter.

BUG=18597
TEST=Test these changes using a set of tuned perf
expectations files: one null entry, the expected
range covering the perf result, the range below the
perf result, and the range above the perf result.
In the first two cases, no visible changes appear in
the summary box.  In the third case, the summary box
is red and regressions are listed.  In the fourth case,
the summary box is orange and speedups are listed.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=25016

[Nicolas Sylvain, 2009-08-28 23:22:56]

some initial comments. Nothing big. Looks awesome so far ;)

http://codereview.chromium.org/177028/diff/1/2
File log_parser/process_log.py (right):

http://codereview.chromium.org/177028/diff/1/2#newcode32
Line 32: PERF_EXPECTATIONS =
'../scripts/master/log_parser/perf_expectations/perf_expectations.json'
80 chars, might want to split that line in 2.

http://codereview.chromium.org/177028/diff/1/2#newcode122
Line 122: self._expected_performance = perf_expect['expect']
should we break out of the look here%

http://codereview.chromium.org/177028/diff/1/2#newcode137
Line 137: if self._expected_performance == {} or self._actual_performance == {}:
are you sure you need the == {} ?  Maybe "if not self._expected_performance"
would work.

http://codereview.chromium.org/177028/diff/1/2#newcode138
Line 138: return self.PerformanceChangesAsText()
would _regressions and _speedups really contain something here? Could we just
return [""] or something like that ?

http://codereview.chromium.org/177028/diff/1/2#newcode147
Line 147: e = self._expected_performance[type]
I'm afraid "e" and "a" make it a little bit too confusing. maybe we should keep
longer names.

http://codereview.chromium.org/177028/diff/1/2#newcode150
Line 150: high_perf = (e['delta'] + 1.5*e['var'])
here a comment would be good, just to go in a little bit more details about what
there numbers mean.

http://codereview.chromium.org/177028/diff/1/2#newcode156
Line 156: 
this empty line should be removed

http://codereview.chromium.org/177028/diff/1/2#newcode159
Line 159: 
this one too

http://codereview.chromium.org/177028/diff/1/2#newcode486
Line 486: test_name = trace_name
so test name is something like "t" ?

[chase, 2009-08-31 06:45:39]

On 2009/08/28 23:22:56, Nicolas Sylvain wrote:
> http://codereview.chromium.org/177028/diff/1/2
> File log_parser/process_log.py (right):
> 
> http://codereview.chromium.org/177028/diff/1/2#newcode32
> Line 32: PERF_EXPECTATIONS =
> '../scripts/master/log_parser/perf_expectations/perf_expectations.json'
> 80 chars, might want to split that line in 2.

Done.

> http://codereview.chromium.org/177028/diff/1/2#newcode122
> Line 122: self._expected_performance = perf_expect['expect']
> should we break out of the look here%

Good point.  I thought about it some more and preferred to rewrite the perf
regression format to use a hash instead of an array.  The loop is avoided
completely.

> http://codereview.chromium.org/177028/diff/1/2#newcode137
> Line 137: if self._expected_performance == {} or self._actual_performance ==
{}:
> are you sure you need the == {} ?  Maybe "if not self._expected_performance"
> would work.

Done.

> http://codereview.chromium.org/177028/diff/1/2#newcode138
> Line 138: return self.PerformanceChangesAsText()
> would _regressions and _speedups really contain something here? Could we just
> return [""] or something like that ?

Done.  Returning [] since an empty array gives the desired effect.

> http://codereview.chromium.org/177028/diff/1/2#newcode147
> Line 147: e = self._expected_performance[type]
> I'm afraid "e" and "a" make it a little bit too confusing. maybe we should
keep
> longer names.

That's understandable.  At first I used the fully qualified names thinking the
same thing.  Using them, the lines are split up and hard to read.  I changed the
e/a to expect/actual.  If you prefer I use the longer names after seeing this,
just let me know.

> http://codereview.chromium.org/177028/diff/1/2#newcode150
> Line 150: high_perf = (e['delta'] + 1.5*e['var'])
> here a comment would be good, just to go in a little bit more details about
what
> there numbers mean.

Done.

> http://codereview.chromium.org/177028/diff/1/2#newcode156
> Line 156: 
> this empty line should be removed

Done.

> http://codereview.chromium.org/177028/diff/1/2#newcode159
> Line 159: 
> this one too

Done.

> http://codereview.chromium.org/177028/diff/1/2#newcode486
> Line 486: test_name = trace_name
> so test name is something like "t" ?

Yeah, that's what test_name would end up holding.  But it's really a result
type, so I rewrote the code to refer to it as such.  I added a comment to
describe this section of code.

[Nicolas Sylvain, 2009-08-31 19:44:03]

maruel, do you want to comment on this?

LGTM from me.

[M-A Ruel, 2009-08-31 20:31:12]

Sorry for the delay, just nits. lgtm

http://codereview.chromium.org/177028/diff/3001/10
File log_parser/process_log.py (right):

http://codereview.chromium.org/177028/diff/3001/10#newcode137
Line 137: raise
That seems quite useless

http://codereview.chromium.org/177028/diff/3001/10#newcode146
Line 146: perf_file.read().strip()))
I'm not sure it's a good idea to read the file after the exception.

[Nicolas Sylvain, 2009-08-31 20:45:38]

On Mon, Aug 31, 2009 at 1:31 PM, <maruel@chromium.org> wrote:

> Sorry for the delay, just nits. lgtm
>
>
> http://codereview.chromium.org/177028/diff/3001/10
> File log_parser/process_log.py (right):
>
> http://codereview.chromium.org/177028/diff/3001/10#newcode137
> Line 137: raise
> That seems quite useless
>
> http://codereview.chromium.org/177028/diff/3001/10#newcode146
> Line 146: perf_file.read().strip()))
> I'm not sure it's a good idea to read the file after the exception.

the exception was for parsing it. Wrong format most likely.

Since the file got open, it should be ok to read it.

But I agree that it looks weird. I would not change it though

Nicolas



>
>
> http://codereview.chromium.org/177028
>