Move memory-related histogram data collection to metrics/memory.py

Move memory-related histogram data collection to metrics/memory.py

BUG=

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

[qyearsley, 2013-08-08 17:35:52]

Because the HistogramMetric class was only used for collecting memory-related
metrics, and it was only used along-side MemoryMetric in the page cycler and
memory measurements, the content of HistogramMetric was merged into
MemoryMetric. Now the page cycler and memory measurements will output the same
memory-related results.

Additionally: The histogram_util module was re-renamed to histogram, and the
other classes that were using histograms (tab_switching and startup_warm
measurements) were changed to use this module in a way more similar to
MemoryMetric.

https://codereview.chromium.org/22492004/diff/1/tools/perf/metrics/histogram.py
File tools/perf/metrics/histogram.py (right):

https://codereview.chromium.org/22492004/diff/1/tools/perf/metrics/histogram....
tools/perf/metrics/histogram.py:1: # Copyright 2013 The Chromium Authors. All
rights reserved.
This module contains basically what was just in 'histogram_util.py'. It could
also be changed to histogram_util, since it's possible that calling it just
'histogram' and putting it in this metrics/ directory might suggest that it
contains a subclass of Metric.

https://codereview.chromium.org/22492004/diff/1/tools/perf/metrics/histogram....
tools/perf/metrics/histogram.py:20: def GetHistogramData(histogram_type,
histogram_name, tab):
I renamed GetHistogramFromDomAutomation to GetHistogramData because the means by
which the histogram data is obtained is probably not important to other classes
and doesn't need to be put in the function name. But if you think
GetHistogramFromDomAutomation is more descriptive, this name could be used
instead.

https://codereview.chromium.org/22492004/diff/1/tools/perf/metrics/memory.py
File tools/perf/metrics/memory.py (right):

https://codereview.chromium.org/22492004/diff/1/tools/perf/metrics/memory.py#...
tools/perf/metrics/memory.py:20: 'type': histogram.BROWSER_HISTOGRAM}]
I also put the histogram type into each list item here in order to make it
slightly simpler below.

[tonyg, 2013-08-09 01:43:44]

lgtm

This is really awesome stuff. Just one comment that needs to be addressed before
landing.

https://codereview.chromium.org/22492004/diff/1/tools/perf/measurements/page_...
File tools/perf/measurements/page_cycler.py (right):

https://codereview.chromium.org/22492004/diff/1/tools/perf/measurements/page_...
tools/perf/measurements/page_cycler.py:104: self._memory_metric.AddResults(tab,
results)
I think these need to be after we wait for the page to load, right?

[nduca, 2013-08-09 09:39:38]

lgtm with tonyg happy thank you so much

[qyearsley, 2013-08-09 16:43:15]

https://codereview.chromium.org/22492004/diff/1/tools/perf/measurements/page_...
File tools/perf/measurements/page_cycler.py (right):

https://codereview.chromium.org/22492004/diff/1/tools/perf/measurements/page_...
tools/perf/measurements/page_cycler.py:104: self._memory_metric.AddResults(tab,
results)
On 2013/08/09 01:43:44, tonyg wrote:
> I think these need to be after we wait for the page to load, right?

Yeah -- that's how it has been, so if want to keep the behavior the same as
before, then that's how it should be.

However, if we wanted to change it to be exactly the same as the memory
measurement, then this could be kept at the top of MeasurePage. Do we want to
make the memory-related results for page cycler measurement and memory
measurement exactly the same?

(I think that the waiting for load is mostly important for the purposes of the
'page_load_time' result... which incidentally will soon be replaced by the speed
index metric. And the speed index metric might wait even longer... perhaps when
that happens, we still want the memory-related to results to be about the state
of memory when MeaurePage is called?)

[tonyg, 2013-08-09 16:48:42]

On 2013/08/09 16:43:15, qyearsley wrote:
>
https://codereview.chromium.org/22492004/diff/1/tools/perf/measurements/page_...
> File tools/perf/measurements/page_cycler.py (right):
> 
>
https://codereview.chromium.org/22492004/diff/1/tools/perf/measurements/page_...
> tools/perf/measurements/page_cycler.py:104:
self._memory_metric.AddResults(tab,
> results)
> On 2013/08/09 01:43:44, tonyg wrote:
> > I think these need to be after we wait for the page to load, right?
> 
> Yeah -- that's how it has been, so if want to keep the behavior the same as
> before, then that's how it should be.
> 
> However, if we wanted to change it to be exactly the same as the memory
> measurement, then this could be kept at the top of MeasurePage. Do we want to
> make the memory-related results for page cycler measurement and memory
> measurement exactly the same?
> 
> (I think that the waiting for load is mostly important for the purposes of the
> 'page_load_time' result... which incidentally will soon be replaced by the
speed
> index metric. And the speed index metric might wait even longer... perhaps
when
> that happens, we still want the memory-related to results to be about the
state
> of memory when MeaurePage is called?)

The point at which we snapshot memory usage is equally important. Please leave
the page_cycler as it was and ping marja@ about whether or not to change the
memory measurement.

[commit-bot: I haz the power, 2013-08-09 17:20:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qyearsley@chromium.org/22492004/10001

[commit-bot: I haz the power, 2013-08-09 18:18:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qyearsley@chromium.org/22492004/27001

[commit-bot: I haz the power, 2013-08-10 02:58:20]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-08-10 05:32:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qyearsley@chromium.org/22492004/27001

[commit-bot: I haz the power, 2013-08-10 14:58:45]

Change committed as 216856

[f(malita), 2013-08-10 18:17:02]

On 2013/08/10 14:58:45, I haz the power (commit-bot) wrote:
> Change committed as 216856

This appears to have broken the Blink perf canaries. PTAL:

http://build.chromium.org/p/chromium.webkit/builders/Linux%20Perf
http://build.chromium.org/p/chromium.webkit/builders/Mac10.6%20Perf


/usr/bin/python2.6 src/tools/perf/run_measurement -v --output-format=buildbot
--browser=release page_cycler src/tools/perf/page_sets/intl_ja_zh.json
No adb found in $PATH, fallback to checked in binary.
INFO:root:No adb command found. Will not try searching for Android browsers.
INFO:root:Running http://www.amazon.co.jp
Traceback (most recent call last):
  File "src/tools/perf/run_measurement", line 102, in <module>
    sys.exit(main())
  File "src/tools/perf/run_measurement", line 99, in main
    sys.exit(runner.Run(BASE_DIR, page_set_filenames))
  File
"/b/build/slave/Mac10_6_Perf/build/src/tools/telemetry/telemetry/page/page_test_runner.py",
line 41, in Run
    results = page_runner.Run(test, ps, expectations, self._options)
  File
"/b/build/slave/Mac10_6_Perf/build/src/tools/perf/../telemetry/telemetry/page/page_runner.py",
line 275, in Run
    credentials_path, possible_browser, results, state)
  File
"/b/build/slave/Mac10_6_Perf/build/src/tools/perf/../telemetry/telemetry/page/page_runner.py",
line 185, in _PrepareAndRunPage
    results_for_current_run, options)
  File
"/b/build/slave/Mac10_6_Perf/build/src/tools/perf/../telemetry/telemetry/page/page_runner.py",
line 366, in _RunPage
    test.Run(options, page, tab, results)
  File
"/b/build/slave/Mac10_6_Perf/build/src/tools/telemetry/telemetry/page/page_test.py",
line 188, in Run
    self._test_method(page, tab, results)
  File "src/tools/perf/../telemetry/telemetry/page/page_measurement.py", line
59, in _RunTest
    self.MeasurePage(page, tab, results)
  File
"/b/build/slave/Mac10_6_Perf/build/src/tools/perf/measurements/page_cycler.py",
line 111, in MeasurePage
    self._memory_metric.AddResults(tab, results)
  File "/b/build/slave/Mac10_6_Perf/build/src/tools/perf/metrics/memory.py",
line 64, in AddResults
    histogram_data = self._histogram_delta_values[h['name']]
TypeError: string indices must be integers
@@@STEP_FAILURE@@@

[tonyg, 2013-08-10 18:33:53]

Please feel free to revert to fix the build. We can fix properly on Monday.

On Sat, Aug 10, 2013 at 11:17 AM,  <fmalita@chromium.org> wrote:
> On 2013/08/10 14:58:45, I haz the power (commit-bot) wrote:
>>
>> Change committed as 216856
>
>
> This appears to have broken the Blink perf canaries. PTAL:
>
> http://build.chromium.org/p/chromium.webkit/builders/Linux%20Perf
> http://build.chromium.org/p/chromium.webkit/builders/Mac10.6%20Perf
>
>
> /usr/bin/python2.6 src/tools/perf/run_measurement -v
> --output-format=buildbot
> --browser=release page_cycler src/tools/perf/page_sets/intl_ja_zh.json
> No adb found in $PATH, fallback to checked in binary.
> INFO:root:No adb command found. Will not try searching for Android browsers.
> INFO:root:Running http://www.amazon.co.jp
> Traceback (most recent call last):
>   File "src/tools/perf/run_measurement", line 102, in <module>
>     sys.exit(main())
>   File "src/tools/perf/run_measurement", line 99, in main
>     sys.exit(runner.Run(BASE_DIR, page_set_filenames))
>   File
>
"/b/build/slave/Mac10_6_Perf/build/src/tools/telemetry/telemetry/page/page_test_runner.py",
> line 41, in Run
>     results = page_runner.Run(test, ps, expectations, self._options)
>   File
>
"/b/build/slave/Mac10_6_Perf/build/src/tools/perf/../telemetry/telemetry/page/page_runner.py",
> line 275, in Run
>     credentials_path, possible_browser, results, state)
>   File
>
"/b/build/slave/Mac10_6_Perf/build/src/tools/perf/../telemetry/telemetry/page/page_runner.py",
> line 185, in _PrepareAndRunPage
>     results_for_current_run, options)
>   File
>
"/b/build/slave/Mac10_6_Perf/build/src/tools/perf/../telemetry/telemetry/page/page_runner.py",
> line 366, in _RunPage
>     test.Run(options, page, tab, results)
>   File
>
"/b/build/slave/Mac10_6_Perf/build/src/tools/telemetry/telemetry/page/page_test.py",
> line 188, in Run
>     self._test_method(page, tab, results)
>   File "src/tools/perf/../telemetry/telemetry/page/page_measurement.py",
> line
> 59, in _RunTest
>     self.MeasurePage(page, tab, results)
>   File
>
"/b/build/slave/Mac10_6_Perf/build/src/tools/perf/measurements/page_cycler.py",
> line 111, in MeasurePage
>     self._memory_metric.AddResults(tab, results)
>   File "/b/build/slave/Mac10_6_Perf/build/src/tools/perf/metrics/memory.py",
> line 64, in AddResults
>     histogram_data = self._histogram_delta_values[h['name']]
> TypeError: string indices must be integers
> @@@STEP_FAILURE@@@
>
>
> https://chromiumcodereview.appspot.com/22492004/

[qyearsley, 2013-08-11 08:09:07]

I'm having some trouble reverting; I don't know yet what the proper sequence of
steps is to revert a commit -- does it require just running "git revert <git
hash of commit to revert>" and then "git svn dcommit"?

On 2013/08/10 18:33:53, tonyg wrote:
> Please feel free to revert to fix the build. We can fix properly on Monday.
>

[fmalita_google_do_not_use, 2013-08-11 16:12:20]

On 2013/08/11 08:09:07, qyearsley wrote:
> I'm having some trouble reverting; I don't know yet what the proper sequence
of
> steps is to revert a commit -- does it require just running "git revert <git
> hash of commit to revert>" and then "git svn dcommit"?
> 
> On 2013/08/10 18:33:53, tonyg wrote:
> > Please feel free to revert to fix the build. We can fix properly on Monday.
> >

You can also use git cl and upload a revert cl TBR, then git cl dcommit.

But I'll take care of it shortly, when I get into the office. Thanks!

[f(malita), 2013-08-12 11:17:24]

Reverted: https://src.chromium.org/viewvc/chrome?revision=216971&view=revision

[qyearsley, 2013-08-12 20:29:40]

On 2013/08/12 11:17:24, Florin Malita wrote:
> Reverted: https://src.chromium.org/viewvc/chrome?revision=216971&view=revision

There were two bugs that I missed before because I didn't try running anything.

The main one was that histogram data might not be available, but in
metrics/memory.py in Stop() and AddResults() I was assuming that data is always
available for all histograms.

Now the tests run.

[qyearsley, 2013-08-12 20:57:47]

https://codereview.chromium.org/22492004/diff/57001/tools/perf/metrics/memory.py
File tools/perf/metrics/memory.py (right):

https://codereview.chromium.org/22492004/diff/57001/tools/perf/metrics/memory...
tools/perf/metrics/memory.py:44: # Histogram data may not be available
Not sure if it's best not to repeat this comment below, but it seems that
there's no harm in it.

https://codereview.chromium.org/22492004/diff/57001/tools/perf/metrics/memory...
tools/perf/metrics/memory.py:61: self._histogram_delta_values[h['name']] =
histogram.SubtractHistogram(
I forgot to use self._histogram_delta_values as a dictionary here before.

https://codereview.chromium.org/22492004/diff/57001/tools/perf/metrics/memory...
tools/perf/metrics/memory.py:62: histogram_data,
self._histogram_start_values[h['name']])
Here (and in AddResults) I'm assuming that if histogram data was set before for
a particular histogram name, then histogram.GetHistogramData will return new
histogram data; I think this is a reasonable assumption.