Chrome remote control multi-page tests: Enable printing out histograms.

Chrome remote control multi-page tests: Enable printing out histograms.

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

[marja, 2012-10-30 09:19:22]

tonyg, bulach, ptal.

bulach:
build/android/pylib/perf_tests_helper.py

tonyg:
tools/chrome_remote_control/chrome_remote_control/multi_page_benchmark.py

[marja, 2012-10-30 15:18:30]

... forgot to say, that this is needed by
http://codereview.chromium.org/11273081/

[tonyg, 2012-10-30 15:52:59]

http://codereview.chromium.org/11340037/diff/3001/build/android/pylib/perf_te...
File build/android/pylib/perf_tests_helper.py (right):

http://codereview.chromium.org/11340037/diff/3001/build/android/pylib/perf_te...
build/android/pylib/perf_tests_helper.py:49: result_type == 'informational'):
if result_type in ['unimportant', 'default', 'informational']:

http://codereview.chromium.org/11340037/diff/3001/build/android/pylib/perf_te...
build/android/pylib/perf_tests_helper.py:68: avg = None
Is it too much code to go ahead and calculate the average here? It would be
really useful when running the tests manually because the histogram output would
otherwise be impossible to parse manually.

http://codereview.chromium.org/11340037/diff/3001/tools/chrome_remote_control...
File tools/chrome_remote_control/chrome_remote_control/multi_page_benchmark.py
(right):

http://codereview.chromium.org/11340037/diff/3001/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/multi_page_benchmark.py:34:
self.histograms = defaultdict(list)
Did you consider adding a self.field_types for 'histogram' or 'default' and
storing these in self.results_summary? Then PrintSummary wouldn't need the
redundant loop, we could get the benefit of ensuring field_name uniqueness and
the CSV writer could output the average value when type == 'histogram'.

[marja, 2012-10-30 17:18:59]

Thanks for comments!

http://codereview.chromium.org/11340037/diff/3001/build/android/pylib/perf_te...
File build/android/pylib/perf_tests_helper.py (right):

http://codereview.chromium.org/11340037/diff/3001/build/android/pylib/perf_te...
build/android/pylib/perf_tests_helper.py:49: result_type == 'informational'):
On 2012/10/30 15:52:59, tonyg wrote:
> if result_type in ['unimportant', 'default', 'informational']:

Done.

http://codereview.chromium.org/11340037/diff/3001/build/android/pylib/perf_te...
build/android/pylib/perf_tests_helper.py:68: avg = None
On 2012/10/30 15:52:59, tonyg wrote:
> Is it too much code to go ahead and calculate the average here? It would be
> really useful when running the tests manually because the histogram output
would
> otherwise be impossible to parse manually.

Duplicated the code from the page cycler log parser here.

http://codereview.chromium.org/11340037/diff/3001/tools/chrome_remote_control...
File tools/chrome_remote_control/chrome_remote_control/multi_page_benchmark.py
(right):

http://codereview.chromium.org/11340037/diff/3001/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/multi_page_benchmark.py:34:
self.histograms = defaultdict(list)
On 2012/10/30 15:52:59, tonyg wrote:
> Did you consider adding a self.field_types for 'histogram' or 'default' and
> storing these in self.results_summary? Then PrintSummary wouldn't need the
> redundant loop, we could get the benefit of ensuring field_name uniqueness and
> the CSV writer could output the average value when type == 'histogram'.

Done.

[marja, 2012-10-30 17:20:22]

http://codereview.chromium.org/11340037/diff/3001/tools/chrome_remote_control...
File tools/chrome_remote_control/chrome_remote_control/multi_page_benchmark.py
(right):

http://codereview.chromium.org/11340037/diff/3001/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/multi_page_benchmark.py:34:
self.histograms = defaultdict(list)
On 2012/10/30 17:19:01, marja wrote:
> On 2012/10/30 15:52:59, tonyg wrote:
> > Did you consider adding a self.field_types for 'histogram' or 'default' and
> > storing these in self.results_summary? Then PrintSummary wouldn't need the
> > redundant loop, we could get the benefit of ensuring field_name uniqueness
and
> > the CSV writer could output the average value when type == 'histogram'.
> 
> Done.

Oops, csv value writer is not yet outputting the avg value, that part is not yet
done..

[marja, 2012-10-30 17:30:21]

http://codereview.chromium.org/11340037/diff/3001/tools/chrome_remote_control...
File tools/chrome_remote_control/chrome_remote_control/multi_page_benchmark.py
(right):

http://codereview.chromium.org/11340037/diff/3001/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/multi_page_benchmark.py:34:
self.histograms = defaultdict(list)
On 2012/10/30 17:20:22, marja wrote:
> On 2012/10/30 17:19:01, marja wrote:
> > On 2012/10/30 15:52:59, tonyg wrote:
> > > Did you consider adding a self.field_types for 'histogram' or 'default'
and
> > > storing these in self.results_summary? Then PrintSummary wouldn't need the
> > > redundant loop, we could get the benefit of ensuring field_name uniqueness
> and
> > > the CSV writer could output the average value when type == 'histogram'.
> > 
> > Done.
> 
> Oops, csv value writer is not yet outputting the avg value, that part is not
yet
> done..

Now that part is done, too.

[bulach, 2012-10-30 18:46:28]

thanks marja! just some ignorable suggestions, feel free to go ahead once tony
is happy :)

http://codereview.chromium.org/11340037/diff/7002/build/android/pylib/perf_te...
File build/android/pylib/perf_tests_helper.py (right):

http://codereview.chromium.org/11340037/diff/7002/build/android/pylib/perf_te...
build/android/pylib/perf_tests_helper.py:46: sum_of_squares += (bucket['mean'] -
geom_mean) ** 2 * bucket['count']
not sure if this would be any clearer, feel free to ignore... :)

sum_of_squares = sum((b['mean'] - geom_mean) ** 2 * b['count']
    for b in histogram['buckets'] if b['mean'] > 0)

http://codereview.chromium.org/11340037/diff/7002/build/android/pylib/perf_te...
build/android/pylib/perf_tests_helper.py:89: value = values[0]
maybe split 74-89 into GeomMeandAndStdDevFromList ?

[tonyg, 2012-10-30 19:41:38]

lgtm

http://codereview.chromium.org/11340037/diff/7002/tools/chrome_remote_control...
File tools/chrome_remote_control/chrome_remote_control/multi_page_benchmark.py
(right):

http://codereview.chromium.org/11340037/diff/7002/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/multi_page_benchmark.py:42:
def Add(self, name, units, value, data_type = 'default'):
Nit: no spaces around '=' in default values.

[marja, 2012-10-31 09:07:50]

Thanks for review!

http://codereview.chromium.org/11340037/diff/7002/tools/chrome_remote_control...
File tools/chrome_remote_control/chrome_remote_control/multi_page_benchmark.py
(right):

http://codereview.chromium.org/11340037/diff/7002/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/multi_page_benchmark.py:42:
def Add(self, name, units, value, data_type = 'default'):
On 2012/10/30 19:41:38, tonyg wrote:
> Nit: no spaces around '=' in default values.

Done. (I wonder why pylint didn't complain..)

[marja, 2012-10-31 09:15:59]

http://codereview.chromium.org/11340037/diff/7002/build/android/pylib/perf_te...
File build/android/pylib/perf_tests_helper.py (right):

http://codereview.chromium.org/11340037/diff/7002/build/android/pylib/perf_te...
build/android/pylib/perf_tests_helper.py:46: sum_of_squares += (bucket['mean'] -
geom_mean) ** 2 * bucket['count']
On 2012/10/30 18:46:28, bulach wrote:
> not sure if this would be any clearer, feel free to ignore... :)
> 
> sum_of_squares = sum((b['mean'] - geom_mean) ** 2 * b['count']
>     for b in histogram['buckets'] if b['mean'] > 0)

Discussed this offline: This code is copied -> decided to leave it as is, to
make it easier to merge them later..

http://codereview.chromium.org/11340037/diff/7002/build/android/pylib/perf_te...
build/android/pylib/perf_tests_helper.py:89: value = values[0]
On 2012/10/30 18:46:28, bulach wrote:
> maybe split 74-89 into GeomMeandAndStdDevFromList ?

Done.