Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22492004:
Move memory-related histogram data collection to metrics/memory.py (Closed)
|
Created:
7 years, 4 months ago by qyearsley Modified:
7 years, 4 months ago CC:
chromium-reviews, chrome-speed-team+watch_google.com, tdanderson Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionMove memory-related histogram data collection to metrics/memory.py
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216856
Patch Set 1 #
Total comments: 5
Patch Set 2 : Move adding of memory results to after page time results #Patch Set 3 : Fixed small mistake (MEMORY_HISTOGRAM should be RENDERER_HISTOGRAM) #Patch Set 4 : Revert #Patch Set 5 : Fix bug caused by not checking whether there is histogram data available #Patch Set 6 : Minor change (to make what is happening more explicit) #
Total comments: 3
Messages
Total messages: 17 (0 generated)
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.
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?
lgtm with tonyg happy thank you so much
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?)
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.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qyearsley@chromium.org/22492004/10001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qyearsley@chromium.org/22492004/27001
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...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qyearsley@chromium.org/22492004/27001
Message was sent while issue was closed.
Change committed as 216856
Message was sent while issue was closed.
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@@@
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/
Message was sent while issue was closed.
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. >
Message was sent while issue was closed.
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!
Message was sent while issue was closed.
Reverted: https://src.chromium.org/viewvc/chrome?revision=216971&view=revision
Message was sent while issue was closed.
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.
Message was sent while issue was closed.
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. |
