Refactoring benchmarks for perf bot efficiency.

Refactoring benchmarks for perf bot efficiency.

See also: https://codereview.chromium.org/11316017/ (separate patch to move scrolling logic into scrolling_interaction.py as discussed in https://codereview.chromium.org/11366197/#msg2)

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

[hartmanng, 2012-11-12 16:57:36]

Here's what I was thinking for benchmark refactoring. It allows us to maintain
our individual benchmarks and easily combine them for the waterfall.

Eventually we should probably rename scrolling_benchmark to something more
descriptive, but we'll have to do that on the bot side first.

Also note that we might not want to put the first_paint_time up on the bot quite
yet, as http://crbug.com/160547 shows it's got some problems. It's pretty easy
to temporarily take that part out if we want to land this though.

Please take a look.

[nduca, 2012-11-12 22:57:29]

Recapping from verbal discussion:
- Separate patch to move scrolling logic to
toosl/telemetry/telemetry/scrolling_interaction.py
- Then, this patch can probably unify everything into one
smoothness_benchmark.py, with scrolling_benchmark.py just calling smoothness
_benchmark and doing nothing else. If that combined file feels to big, then
maybe have the individual measurements be called
first_paint_time_measurement.py, smoothness_measurement.py,
paint_measurement.py, etc.

[marja, 2012-11-13 09:42:10]

FYI, I'm adding a memory benchmark here:
http://codereview.chromium.org/11273081/ ; not sure how these CLs interfere with
each other.

[hartmanng, 2012-11-20 23:15:42]

Now that https://codereview.chromium.org/11316017/ landed, here's the other side
of the patch to unify the scrolling benchmarks.

PTAL

[nduca, 2012-11-20 23:32:39]

lgtmish get tonyg to do final

https://codereview.chromium.org/11366197/diff/8002/tools/perf/perf_tools/smoo...
File tools/perf/perf_tools/smoothness_benchmark.py (right):

https://codereview.chromium.org/11366197/diff/8002/tools/perf/perf_tools/smoo...
tools/perf/perf_tools/smoothness_benchmark.py:108: return
erm, you're skipping all further stuff here

[hartmanng, 2012-11-20 23:36:24]

tonyg: please take a look

https://codereview.chromium.org/11366197/diff/8002/tools/perf/perf_tools/smoo...
File tools/perf/perf_tools/smoothness_benchmark.py (right):

https://codereview.chromium.org/11366197/diff/8002/tools/perf/perf_tools/smoo...
tools/perf/perf_tools/smoothness_benchmark.py:108: return
On 2012/11/20 23:32:39, nduca wrote:
> erm, you're skipping all further stuff here

uh... oops...

[tonyg, 2012-11-20 23:58:18]

https://codereview.chromium.org/11366197/diff/15001/tools/perf/perf_tools/smo...
File tools/perf/perf_tools/smoothness_benchmark.py (right):

https://codereview.chromium.org/11366197/diff/15001/tools/perf/perf_tools/smo...
tools/perf/perf_tools/smoothness_benchmark.py:22: # Scrolling
These new comments are a pretty good indicator that this method is too long and
would be better refactored into 3 helper methods -- CalcScrollResults,
CalcPaintingResults, CalcTextureUploadResults.

https://codereview.chromium.org/11366197/diff/15001/tools/perf/perf_tools/smo...
tools/perf/perf_tools/smoothness_benchmark.py:77:
results.Add('average_commit_time', 'ms', averageCommitTimeMs)
While you are adding new results, I wonder if you could take care of a
cleanup...

By default the buildbot puts all results in the summary for the step on the
waterfall. With this many results, it kind of breaks the UI. To suppress
outputting the results in the summary, you can add "data_type='unimportant'" to
the Add() call.

Would you mind doing this for all but the most important metric in each of the 4
categories -- that is all but 'mean_frame_time', 'total_paint_time',
'texture_upload_count', and 'first_paint'.

https://codereview.chromium.org/11366197/diff/15001/tools/perf/perf_tools/smo...
tools/perf/perf_tools/smoothness_benchmark.py:105: # First Paint Time
Recommend moving this out to a CalcFirstPaintTimeResults method.

https://codereview.chromium.org/11366197/diff/15001/tools/perf/perf_tools/smo...
tools/perf/perf_tools/smoothness_benchmark.py:118:
'window.chrome.loadTimes().firstPaintTime - ' +
nit: indent 4 spaces

https://codereview.chromium.org/11366197/diff/15001/tools/perf/perf_tools/smo...
tools/perf/perf_tools/smoothness_benchmark.py:121: results.Add('first_paint',
'seconds', round(first_paint_secs, 1))
All the other results are in ms. Let's convert the output to ms before
outputting.

[hartmanng, 2012-11-21 14:34:55]

PTAL

https://codereview.chromium.org/11366197/diff/15001/tools/perf/perf_tools/smo...
File tools/perf/perf_tools/smoothness_benchmark.py (right):

https://codereview.chromium.org/11366197/diff/15001/tools/perf/perf_tools/smo...
tools/perf/perf_tools/smoothness_benchmark.py:22: # Scrolling
On 2012/11/20 23:58:18, tonyg wrote:
> These new comments are a pretty good indicator that this method is too long
and
> would be better refactored into 3 helper methods -- CalcScrollResults,
> CalcPaintingResults, CalcTextureUploadResults.

Done.

https://codereview.chromium.org/11366197/diff/15001/tools/perf/perf_tools/smo...
tools/perf/perf_tools/smoothness_benchmark.py:77:
results.Add('average_commit_time', 'ms', averageCommitTimeMs)
On 2012/11/20 23:58:18, tonyg wrote:
> While you are adding new results, I wonder if you could take care of a
> cleanup...
> 
> By default the buildbot puts all results in the summary for the step on the
> waterfall. With this many results, it kind of breaks the UI. To suppress
> outputting the results in the summary, you can add "data_type='unimportant'"
to
> the Add() call.
> 
> Would you mind doing this for all but the most important metric in each of the
4
> categories -- that is all but 'mean_frame_time', 'total_paint_time',
> 'texture_upload_count', and 'first_paint'.

Done.

https://codereview.chromium.org/11366197/diff/15001/tools/perf/perf_tools/smo...
tools/perf/perf_tools/smoothness_benchmark.py:105: # First Paint Time
On 2012/11/20 23:58:18, tonyg wrote:
> Recommend moving this out to a CalcFirstPaintTimeResults method.

Done.

https://codereview.chromium.org/11366197/diff/15001/tools/perf/perf_tools/smo...
tools/perf/perf_tools/smoothness_benchmark.py:118:
'window.chrome.loadTimes().firstPaintTime - ' +
On 2012/11/20 23:58:18, tonyg wrote:
> nit: indent 4 spaces

Done.

https://codereview.chromium.org/11366197/diff/15001/tools/perf/perf_tools/smo...
tools/perf/perf_tools/smoothness_benchmark.py:121: results.Add('first_paint',
'seconds', round(first_paint_secs, 1))
On 2012/11/20 23:58:18, tonyg wrote:
> All the other results are in ms. Let's convert the output to ms before
> outputting.

Done.

[tonyg, 2012-11-21 18:00:19]

lgtm

[commit-bot: I haz the power, 2012-11-21 18:01:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hartmanng@chromium.org/11366197/18002

[commit-bot: I haz the power, 2012-11-21 20:11:53]

Change committed as 169110