Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23112028:
[Telemetry] Add support for capturing V8 object stats to Telemetry. (Closed)
|
Created:
7 years, 4 months ago by rmcilroy Modified:
7 years, 3 months ago CC:
chromium-reviews, chrome-speed-team+watch_google.com, jar (doing other things), jam, joi+watch-content_chromium.org, darin-cc_chromium.org, asvitkine+watch_chromium.org, erikwright+watch_chromium.org, Ilya Sherman, jochen (gone - plz use gerrit) Base URL:
https://chromium.googlesource.com/chromium/src.git@stats_table_android Visibility:
Public. |
Description[Telemetry] Add support for capturing V8 object stats to Telemetry.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220606
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 2
Patch Set 4 : Change to using window.chrome.benchark instead of StatsCollectionController #
Total comments: 7
Patch Set 5 : Address tony's comments. #Patch Set 6 : Remove diffbase #
Total comments: 2
Patch Set 7 : #Patch Set 8 : Rebase #Patch Set 9 : Sync #
Messages
Total messages: 16 (0 generated)
jochen@chromium.org: Please review changes in content/renderer tonyg@chromium.org: Please review changes in tools/perf
https://codereview.chromium.org/23112028/diff/5001/content/renderer/stats_col... File content/renderer/stats_collection_controller.h (right): https://codereview.chromium.org/23112028/diff/5001/content/renderer/stats_col... content/renderer/stats_collection_controller.h:36: void GetStatsTable(const webkit_glue::CppArgumentList& args, I don't think this is necessary. --enable-benchmarking already exposes the stats table to the DOM. See https://codereview.chromium.org/22539003/ (which had to be reverted for unrelated reasons).
https://codereview.chromium.org/23112028/diff/5001/content/renderer/stats_col... File content/renderer/stats_collection_controller.h (right): https://codereview.chromium.org/23112028/diff/5001/content/renderer/stats_col... content/renderer/stats_collection_controller.h:36: void GetStatsTable(const webkit_glue::CppArgumentList& args, On 2013/08/23 16:31:32, tonyg wrote: > I don't think this is necessary. --enable-benchmarking already exposes the stats > table to the DOM. See https://codereview.chromium.org/22539003/ (which had to be > reverted for unrelated reasons). Thanks for the hint - moved to using --enable-benchmarking. I had to add to benchmarking_extension.cc to enable getting stats for a particular process (e.g., the renderer) rather than global stats. https://codereview.chromium.org/23112028/diff/9001/tools/perf/metrics/v8_obje... File tools/perf/metrics/v8_object_stats.py (right): https://codereview.chromium.org/23112028/diff/9001/tools/perf/metrics/v8_obje... tools/perf/metrics/v8_object_stats.py:175: if (!window.chrome || !window.chrome.benchmarking) tonyg: by moving to --enable-benchmarking, sometimes this "if" will be hit thus returning an empty result. This seems to happen for http://www.fifa.com/ in typical_25.json. Any ideas why and how we can fix this?
https://codereview.chromium.org/23112028/diff/9001/tools/perf/measurements/pa... File tools/perf/measurements/page_cycler.py (right): https://codereview.chromium.org/23112028/diff/9001/tools/perf/measurements/pa... tools/perf/measurements/page_cycler.py:74: options.AppendExtraBrowserArg('--enable-stats-collection-bindings') Was there a merge error here? Ideally there should be no new flags in this file and the Metric should add everything it needs in CustomizeBrowserOptions. https://codereview.chromium.org/23112028/diff/9001/tools/perf/metrics/v8_obje... File tools/perf/metrics/v8_object_stats.py (right): https://codereview.chromium.org/23112028/diff/9001/tools/perf/metrics/v8_obje... tools/perf/metrics/v8_object_stats.py:168: """Do Nothing.""" Does this work? I'd expect you need a "pass" line here. https://codereview.chromium.org/23112028/diff/9001/tools/perf/metrics/v8_obje... tools/perf/metrics/v8_object_stats.py:175: if (!window.chrome || !window.chrome.benchmarking) On 2013/08/27 12:06:41, rmcilroy wrote: > tonyg: by moving to --enable-benchmarking, sometimes this "if" will be hit thus > returning an empty result. This seems to happen for http://www.fifa.com/ in > typical_25.json. Any ideas why and how we can fix this? fifa.com appears to be stomping on window.chrome. I just loaded the site, executed "window.chrome" in the console and it returns "1" instead of the window.chrome object. I'd say to just make sure we fail gracefully in this case and output an error message saying that v8 object stats cannot be collected for this page.
https://codereview.chromium.org/23112028/diff/9001/tools/perf/measurements/pa... File tools/perf/measurements/page_cycler.py (right): https://codereview.chromium.org/23112028/diff/9001/tools/perf/measurements/pa... tools/perf/measurements/page_cycler.py:74: options.AppendExtraBrowserArg('--enable-stats-collection-bindings') On 2013/08/27 15:40:02, tonyg wrote: > Was there a merge error here? Ideally there should be no new flags in this file > and the Metric should add everything it needs in CustomizeBrowserOptions. Opps, this seems to have been a merge error. Done. https://codereview.chromium.org/23112028/diff/9001/tools/perf/metrics/v8_obje... File tools/perf/metrics/v8_object_stats.py (right): https://codereview.chromium.org/23112028/diff/9001/tools/perf/metrics/v8_obje... tools/perf/metrics/v8_object_stats.py:168: """Do Nothing.""" On 2013/08/27 15:40:02, tonyg wrote: > Does this work? I'd expect you need a "pass" line here. It does work, but added "pass" anyway for clarity. https://codereview.chromium.org/23112028/diff/9001/tools/perf/metrics/v8_obje... tools/perf/metrics/v8_object_stats.py:175: if (!window.chrome || !window.chrome.benchmarking) On 2013/08/27 15:40:02, tonyg wrote: > On 2013/08/27 12:06:41, rmcilroy wrote: > > tonyg: by moving to --enable-benchmarking, sometimes this "if" will be hit > thus > > returning an empty result. This seems to happen for http://www.fifa.com/ in > > typical_25.json. Any ideas why and how we can fix this? > > http://fifa.com appears to be stomping on window.chrome. I just loaded the site, > executed "window.chrome" in the console and it returns "1" instead of the > window.chrome object. > > I'd say to just make sure we fail gracefully in this case and output an error > message saying that v8 object stats cannot be collected for this page. Done.
lgtm https://codereview.chromium.org/23112028/diff/20001/tools/perf/measurements/p... File tools/perf/measurements/page_cycler.py (right): https://codereview.chromium.org/23112028/diff/20001/tools/perf/measurements/p... tools/perf/measurements/page_cycler.py:80: v8_object_stats.V8ObjectStatsMetric.CustomizeBrowserOptions(options) Another merge error? This bit shows up twice.
Sky: could you please review the chrome/renderer changes. Jochen is OOO. Thanks. https://codereview.chromium.org/23112028/diff/20001/tools/perf/measurements/p... File tools/perf/measurements/page_cycler.py (right): https://codereview.chromium.org/23112028/diff/20001/tools/perf/measurements/p... tools/perf/measurements/page_cycler.py:80: v8_object_stats.V8ObjectStatsMetric.CustomizeBrowserOptions(options) On 2013/08/28 02:12:07, tonyg wrote: > Another merge error? This bit shows up twice. Your right, thanks. Done
Sorry, I'm not a good reviewer for chrome/renderer. On Wed, Aug 28, 2013 at 2:44 AM, <rmcilroy@chromium.org> wrote: > Sky: could you please review the chrome/renderer changes. Jochen is OOO. > > Thanks. > > > > https://codereview.chromium.org/23112028/diff/20001/tools/perf/measurements/p... > File tools/perf/measurements/page_cycler.py (right): > > https://codereview.chromium.org/23112028/diff/20001/tools/perf/measurements/p... > tools/perf/measurements/page_cycler.py:80: > v8_object_stats.V8ObjectStatsMetric.CustomizeBrowserOptions(options) > On 2013/08/28 02:12:07, tonyg wrote: >> >> Another merge error? This bit shows up twice. > > > Your right, thanks. Done > > https://codereview.chromium.org/23112028/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
No problem Scott, sorry for the noise. Jam, would you be able to look at the content/renderer/browser_extension.cc changes please?
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rmcilroy@chromium.org/23112028/23001
Failed to apply patch for tools/perf/measurements/page_cycler.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file tools/perf/measurements/page_cycler.py
Hunk #1 succeeded at 21 (offset 1 line).
Hunk #2 FAILED at 32.
Hunk #3 succeeded at 50 with fuzz 2 (offset 7 lines).
Hunk #4 FAILED at 63.
Hunk #5 succeeded at 128 (offset 44 lines).
2 out of 5 hunks FAILED -- saving rejects to file
tools/perf/measurements/page_cycler.py.rej
Patch: tools/perf/measurements/page_cycler.py
Index: tools/perf/measurements/page_cycler.py
diff --git a/tools/perf/measurements/page_cycler.py
b/tools/perf/measurements/page_cycler.py
index
e373eeab72694b39f1f202a54aedcf6e2842739e..9e1bd3e050b5875168ac597971050e2351766b6e
100644
--- a/tools/perf/measurements/page_cycler.py
+++ b/tools/perf/measurements/page_cycler.py
@@ -20,6 +20,7 @@ import sys
from metrics import io
from metrics import memory
+from metrics import v8_object_stats
from telemetry.core import util
from telemetry.page import page_measurement
@@ -31,7 +32,11 @@ class PageCycler(page_measurement.PageMeasurement):
'page_cycler.js'), 'r') as f:
self._page_cycler_js = f.read()
+ self._record_v8_object_stats = False
+
self._memory_metric = None
+ self._v8_object_stats_metric = None
+
def AddCommandLineOptions(self, parser):
# The page cyclers should default to 10 iterations. In order to change the
@@ -42,9 +47,15 @@ class PageCycler(page_measurement.PageMeasurement):
parser.remove_option('--pageset-repeat')
parser.add_option(pageset_repeat_option)
+ parser.add_option('--v8-object-stats',
+ action='store_true',
+ help='Enable detailed V8 object statistics.')
+
def DidStartBrowser(self, browser):
"""Initialize metrics once right after the browser has been launched."""
self._memory_metric = memory.MemoryMetric(browser)
+ if self._record_v8_object_stats:
+ self._v8_object_stats_metric = v8_object_stats.V8ObjectStatsMetric()
def DidStartHTTPServer(self, tab):
# Avoid paying for a cross-renderer navigation on the first page on legacy
@@ -56,12 +67,18 @@ class PageCycler(page_measurement.PageMeasurement):
def DidNavigateToPage(self, page, tab):
self._memory_metric.Start(page, tab)
+ if self._record_v8_object_stats:
+ self._v8_object_stats_metric.Start(page, tab)
def CustomizeBrowserOptions(self, options):
memory.MemoryMetric.CustomizeBrowserOptions(options)
io.IOMetric.CustomizeBrowserOptions(options)
options.AppendExtraBrowserArg('--js-flags=--expose_gc')
+ if options.v8_object_stats:
+ self._record_v8_object_stats = True
+ v8_object_stats.V8ObjectStatsMetric.CustomizeBrowserOptions(options)
+
# Temporarily disable typical_25 page set on mac.
if sys.platform == 'darwin' and sys.argv[-1].endswith('/typical_25.json'):
print 'typical_25 is currently disabled on mac. Skipping test.'
@@ -77,6 +94,9 @@ class PageCycler(page_measurement.PageMeasurement):
self._memory_metric.Stop(page, tab)
self._memory_metric.AddResults(tab, results)
+ if self._record_v8_object_stats:
+ self._v8_object_stats_metric.Stop(page, tab)
+ self._v8_object_stats_metric.AddResults(tab, results)
def DidRunTest(self, tab, results):
self._memory_metric.AddSummaryResults(results)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rmcilroy@chromium.org/23112028/30001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rmcilroy@chromium.org/23112028/41001
Message was sent while issue was closed.
Change committed as 220606 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
