Telemetry / Memory benchmark fix: Separate histograms for different tests.

Telemetry / Memory benchmark fix: Separate histograms for different tests.

We can't assume that all histograms are reset between test cases. We need to
remember start values for histograms before the test run, and subtract them from the
results.

memory_benchmark results before and after this CL are not comparable.

R=nduca
BUG=NONE
NOTRY=true


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

[marja, 2013-02-12 11:42:45]

nduca, ptal

[nduca, 2013-02-12 18:33:15]

https://codereview.chromium.org/12221137/diff/1/tools/perf/perf_tools/memory_...
File tools/perf/perf_tools/memory_benchmark.py (right):

https://codereview.chromium.org/12221137/diff/1/tools/perf/perf_tools/memory_...
tools/perf/perf_tools/memory_benchmark.py:21: def WillNavigateToPage(self, page,
tab):
not sure i'd call this baselines. The word 'Baselines' usually is used in the
context of test expectations. These are start values, more like.

https://codereview.chromium.org/12221137/diff/1/tools/perf/perf_tools/memory_...
tools/perf/perf_tools/memory_benchmark.py:23: name = histogram['name']
How about a histogram measurement object?

HistogramMeasurement(hsto name)

willmeasure:
m.start()

didmeasure:
m.getvalue()

https://codereview.chromium.org/12221137/diff/1/tools/telemetry/telemetry/uti...
File tools/telemetry/telemetry/util.py (right):

https://codereview.chromium.org/12221137/diff/1/tools/telemetry/telemetry/uti...
tools/telemetry/telemetry/util.py:71: def SubtractHistogram(histogram_json,
baseline_json):
i'd rather this wasn't in telemetry. Histograms are chrome sepcific. You can
have a perf_lib util if you wanted...

[nduca, 2013-02-12 19:31:53]

+vmpstr, you shoudl probably leverage this for your patch somehow. Or fuse or
something. Dunno.

[marja, 2013-02-13 16:09:30]

Thanks for comments!

+bulach, can you review the build/android/pylib/perf_tests_helper.py additions.

https://codereview.chromium.org/12221137/diff/1/tools/perf/perf_tools/memory_...
File tools/perf/perf_tools/memory_benchmark.py (right):

https://codereview.chromium.org/12221137/diff/1/tools/perf/perf_tools/memory_...
tools/perf/perf_tools/memory_benchmark.py:21: def WillNavigateToPage(self, page,
tab):
On 2013/02/12 18:33:15, nduca wrote:
> not sure i'd call this baselines. The word 'Baselines' usually is used in the
> context of test expectations. These are start values, more like.

Done.

https://codereview.chromium.org/12221137/diff/1/tools/perf/perf_tools/memory_...
tools/perf/perf_tools/memory_benchmark.py:23: name = histogram['name']
On 2013/02/12 18:33:15, nduca wrote:
> How about a histogram measurement object?
> 
> HistogramMeasurement(hsto name)
> 
> willmeasure:
> m.start()
> 
> didmeasure:
> m.getvalue()

Done.

https://codereview.chromium.org/12221137/diff/1/tools/telemetry/telemetry/uti...
File tools/telemetry/telemetry/util.py (right):

https://codereview.chromium.org/12221137/diff/1/tools/telemetry/telemetry/uti...
tools/telemetry/telemetry/util.py:71: def SubtractHistogram(histogram_json,
baseline_json):
On 2013/02/12 18:33:15, nduca wrote:
> i'd rather this wasn't in telemetry. Histograms are chrome sepcific. You can
> have a perf_lib util if you wanted...

perf_test_helpers.py seems like a logical place, since it already has other
histogram funcs. Moved it there.

[bulach, 2013-02-13 18:36:48]

lgtm for android

probably a follow up but hopefully in line with nat's comment, Histograms are
not android specific either :)

maybe we could move all of this somewhere else in a nice encapsulated and tested
"histogram.py" class? wdyt?

[nduca, 2013-02-13 18:45:55]

https://codereview.chromium.org/12221137/diff/7001/build/android/pylib/perf_t...
File build/android/pylib/perf_tests_helper.py (left):

https://codereview.chromium.org/12221137/diff/7001/build/android/pylib/perf_t...
build/android/pylib/perf_tests_helper.py:59: sqdiffs = [(float(v) - avg) ** 2
for v in values]
yeah, this shouldn't be in android... this library doesn't import cleanly on
windows for instance. perf only accidentally has a dependency on build/android
--- it shouuld only ever be *used* via telemetry.

can you copy the needed funcs from here into histogram.py direct?

https://codereview.chromium.org/12221137/diff/7001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/histogram_measurement.py (right):

https://codereview.chromium.org/12221137/diff/7001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/histogram_measurement.py:7: def __init__(self,
histogram, is_browser_histogram=False):
at the module level

BROWSER_HISTOGRAM = 'browser_histogram'
RENDERER_HISTOGRAM = 'renderer_histogram'

then have this argument be histogram_type=RENDERER_HISTOGRAM

makes the call site clear what its saying

[nduca, 2013-02-13 20:18:16]

lgtm with tweaks discussed offline

[marja, 2013-02-14 13:06:53]

Thanks for review! I did the modifications as discussed.

bulach: I undid the changes to build/android. I left the existing
histogram-handling funcs in place, because PrintPerfResults needs them. When
that mess (Telemetry using build/android) is cleaned up, I hope the rest of the
histogram mess is also cleaned up as a side product.

I'm committing this now, as always, if I did something else than what you though
we agreed, I'm happy to do follow-up changes..

https://codereview.chromium.org/12221137/diff/7001/build/android/pylib/perf_t...
File build/android/pylib/perf_tests_helper.py (left):

https://codereview.chromium.org/12221137/diff/7001/build/android/pylib/perf_t...
build/android/pylib/perf_tests_helper.py:59: sqdiffs = [(float(v) - avg) ** 2
for v in values]
On 2013/02/13 18:45:55, nduca wrote:
> yeah, this shouldn't be in android... this library doesn't import cleanly on
> windows for instance. perf only accidentally has a dependency on build/android
> --- it shouuld only ever be *used* via telemetry.
> 
> can you copy the needed funcs from here into histogram.py direct?

This function is used by PrintPerfResult, it needs to somehow be able to print
histogram results... Not sure what to do about this function. PrintPerfResult
shouldn't be here either. :/ (I left this alone in this CL.)

https://codereview.chromium.org/12221137/diff/7001/build/android/pylib/perf_t...
File build/android/pylib/perf_tests_helper.py (right):

https://codereview.chromium.org/12221137/diff/7001/build/android/pylib/perf_t...
build/android/pylib/perf_tests_helper.py:68: def
SubtractHistogram(histogram_json, start_histogram_json):
And I moved this one away now.

https://codereview.chromium.org/12221137/diff/7001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/histogram_measurement.py (right):

https://codereview.chromium.org/12221137/diff/7001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/histogram_measurement.py:7: def __init__(self,
histogram, is_browser_histogram=False):
On 2013/02/13 18:45:55, nduca wrote:
> at the module level
> 
> BROWSER_HISTOGRAM = 'browser_histogram'
> RENDERER_HISTOGRAM = 'renderer_histogram'
> 
> then have this argument be histogram_type=RENDERER_HISTOGRAM
> 
> makes the call site clear what its saying

Done.

[marja, 2013-02-14 13:07:47]

(Oops, test was missing, I'm adding it back from patch set 1.)

[marja, 2013-02-14 14:46:37]

No, I won't commit this, this is broken.

The WillNavigateToPage will query the histograms from a different renderer
process than MeasurePage. This will work nicely for histograms which are stored
on the browser side, but not at all for histograms which are stored on the
renderer side.

[marja, 2013-02-14 15:05:45]

Alright, fixed. We need DidNavigateToPage instead of WillNavigateToPage. At that
point we can assume that we're in the right renderer and we'll stay in that
renderer for the rest of the test. But... it's... way more fragile than I'd like
it to be. :(

[commit-bot: I haz the power, 2013-02-14 15:06:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/12221137/2003

[commit-bot: I haz the power, 2013-02-14 15:07:01]

Change committed as 182467

[anagy4698, 2013-02-14 15:47:08]

Brian
Anderson<https://chromiumcodereview.appspot.com/user/Brian%20Anderson>:Tisztelettel",,
értérelést, felügyelő úr, nem tudok rolla. Hogy aláírásommal, tettem  meg
ön : felé azon kérésem, hogy felmérést és vissza.Jelzést küldjek az ön, és
becses brigádja számára?"..Ez volt a megméretettés? vagy ki-mocskolás?,
.Ami kb:tart 2-éve.,? rám küdik,az összes, külőnös misztikus enberi,
motivált Hibisz kuszban,,érlelt agyuk álltalt ,,Enberi
agyszüleményüket,Hogy mely hatással, van az emberi, elmére?..Felylett kis,
Embriokra,nehogy véletlen.,Ki legyenek, prédikálva:"a Facebook-becses?"
honlapjain?..  Kérem nagyon "? nem unlyák? ? ?..KÖSZÖ
NETTEL:::::::::anagy4698@gmail.com,,


2013/2/14 <commit-bot@chromium.org>

> CQ is trying da patch. Follow status at
> https://chromium-status.**appspot.com/cq/marja@chromium.**
>
org/12221137/2003<https://chromium-status.appspot.com/cq/marja@chromium.org/12221137/2003>
>
>
>
https://chromiumcodereview.**appspot.com/12221137/<https://chromiumcodereview...
>



-- 
anagy4698@gmail.com.