Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23963002:
Update V8ObjectStatsMetric to work as a standalone metric (Closed)
|
Created:
7 years, 3 months ago by edmundyan Modified:
7 years, 3 months ago CC:
chromium-reviews, chrome-speed-team+watch_google.com, qyearsley, Michael Achenbach, anantha Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUpdate V8ObjectStatsMetric to work as a standalone metric
TEST=tools/perf/run_measurement page_cycler_bloat --v8-object-stats
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221467
Patch Set 1 #
Total comments: 9
Patch Set 2 : Adding rmcilroy TODO for --no-sandbox option #Messages
Total messages: 12 (0 generated)
Hey Ross, Just found your change and it looks awesome. Thanks for pushing this through. We would like to use this metric to track V8 memory usage in Chrome Endure. You can check out my usage here: https://codereview.chromium.org/23961002/. Found a few bugs in the metric. Also, I want to propose a more API-like method that returns the counters dict rather than storing into _results to make it more usable for endure. Or we add a "results" property? What do you think dave/tony?
https://codereview.chromium.org/23963002/diff/1/tools/perf/metrics/v8_object_... File tools/perf/metrics/v8_object_stats.py (right): https://codereview.chromium.org/23963002/diff/1/tools/perf/metrics/v8_object_... tools/perf/metrics/v8_object_stats.py:167: options.AppendExtraBrowserArg('--no-sandbox') I don't see how this is related. Can you explain why we need --no-sandbox? https://codereview.chromium.org/23963002/diff/1/tools/perf/metrics/v8_object_... tools/perf/metrics/v8_object_stats.py:172: counters = _COUNTER_NAMES Rather than this, can't you just do counters=_COUNTER_NAMES right in the function arguments?
https://codereview.chromium.org/23963002/diff/1/tools/perf/metrics/v8_object_... File tools/perf/metrics/v8_object_stats.py (right): https://codereview.chromium.org/23963002/diff/1/tools/perf/metrics/v8_object_... tools/perf/metrics/v8_object_stats.py:167: options.AppendExtraBrowserArg('--no-sandbox') On 2013/09/04 17:41:11, tonyg wrote: > I don't see how this is related. Can you explain why we need --no-sandbox? I needed to add this line over in endure.py, otherwise no pages could be loaded (I couldn't even navigate to them). I do not know why it is needed. '--no-sandbox' is added in the IO metric, thus this is never a problem for page_cycler. This is the stack trace on endure.py when I don't have sandbox: Stack Trace: ******************************************************************************** [7:7:0904/104928:ERROR:shared_memory_posix.cc(213)] Creating shared memory in /dev/shm/org.chromium.Chromium.shmem.ChromiumStats2-4294967295 failed: Permission denied [7:7:0904/104928:ERROR:shared_memory_posix.cc(216)] Unable to access(W_OK|X_OK) /dev/shm: Permission denied [7:7:0904/104928:FATAL:shared_memory_posix.cc(218)] This is frequently caused by incorrect permissions on /dev/shm. Try 'sudo chmod 1777 /dev/shm' to fix. [8:8:0904/104930:ERROR:shared_memory_posix.cc(213)] Creating shared memory in /dev/shm/org.chromium.Chromium.shmem.ChromiumStats2-4294967295 failed: Permission denied [8:8:0904/104930:ERROR:shared_memory_posix.cc(216)] Unable to access(W_OK|X_OK) /dev/shm: Permission denied [8:8:0904/104930:FATAL:shared_memory_posix.cc(218)] This is frequently caused by incorrect permissions on /dev/shm. Try 'sudo chmod 1777 /dev/shm' to fix. ******************************************************************************** https://codereview.chromium.org/23963002/diff/1/tools/perf/metrics/v8_object_... tools/perf/metrics/v8_object_stats.py:172: counters = _COUNTER_NAMES On 2013/09/04 17:41:11, tonyg wrote: > Rather than this, can't you just do counters=_COUNTER_NAMES right in the > function arguments? That is what I did originally, but pylint didn't like it, saying " dangerous default value as argument". Technically, _COUNTER_NAMES is still a mutable list.
https://codereview.chromium.org/23963002/diff/1/tools/perf/metrics/v8_object_... File tools/perf/metrics/v8_object_stats.py (left): https://codereview.chromium.org/23963002/diff/1/tools/perf/metrics/v8_object_... tools/perf/metrics/v8_object_stats.py:35: 'V8.MemoryLoSpaceBytesUsed)', oops, thanks! https://codereview.chromium.org/23963002/diff/1/tools/perf/metrics/v8_object_... File tools/perf/metrics/v8_object_stats.py (right): https://codereview.chromium.org/23963002/diff/1/tools/perf/metrics/v8_object_... tools/perf/metrics/v8_object_stats.py:167: options.AppendExtraBrowserArg('--no-sandbox') On 2013/09/04 17:53:41, edmundyan wrote: > On 2013/09/04 17:41:11, tonyg wrote: > > I don't see how this is related. Can you explain why we need --no-sandbox? > I needed to add this line over in endure.py, otherwise no pages could be loaded > (I couldn't even navigate to them). I do not know why it is needed. > '--no-sandbox' is added in the IO metric, thus this is never a problem for > page_cycler. This is the stack trace on endure.py when I don't have sandbox: > > Stack Trace: > ******************************************************************************** > [7:7:0904/104928:ERROR:shared_memory_posix.cc(213)] Creating shared memory in > /dev/shm/org.chromium.Chromium.shmem.ChromiumStats2-4294967295 failed: > Permission denied > [7:7:0904/104928:ERROR:shared_memory_posix.cc(216)] Unable to access(W_OK|X_OK) > /dev/shm: Permission denied > [7:7:0904/104928:FATAL:shared_memory_posix.cc(218)] This is frequently caused > by incorrect permissions on /dev/shm. Try 'sudo chmod 1777 /dev/shm' to fix. > [8:8:0904/104930:ERROR:shared_memory_posix.cc(213)] Creating shared memory in > /dev/shm/org.chromium.Chromium.shmem.ChromiumStats2-4294967295 failed: > Permission denied Ahh yes, this is required for --enable-stats-table currently (it must be set elsewhere for the page_cycler). I have a change (https://codereview.chromium.org/22911027/) which enables stats-table to be used on Android, and also means that on Posix systems the --no-sandbox option is not needed, but this hasn't landed yet. I think this is required for now, but maybe add a TODO(rmcilroy) to only do this on windows when the CL lands. https://codereview.chromium.org/23963002/diff/1/tools/perf/metrics/v8_object_... tools/perf/metrics/v8_object_stats.py:172: counters = _COUNTER_NAMES On 2013/09/04 17:53:41, edmundyan wrote: > On 2013/09/04 17:41:11, tonyg wrote: > > Rather than this, can't you just do counters=_COUNTER_NAMES right in the > > function arguments? > > That is what I did originally, but pylint didn't like it, saying " dangerous > default value as argument". Technically, _COUNTER_NAMES is still a mutable > list. Maybe just: counters = counters or _COUNTER_NAMES
https://codereview.chromium.org/23963002/diff/1/tools/perf/metrics/v8_object_... File tools/perf/metrics/v8_object_stats.py (right): https://codereview.chromium.org/23963002/diff/1/tools/perf/metrics/v8_object_... tools/perf/metrics/v8_object_stats.py:167: options.AppendExtraBrowserArg('--no-sandbox') On 2013/09/04 18:09:21, rmcilroy wrote: > On 2013/09/04 17:53:41, edmundyan wrote: > > On 2013/09/04 17:41:11, tonyg wrote: > > > I don't see how this is related. Can you explain why we need --no-sandbox? > > I needed to add this line over in endure.py, otherwise no pages could be > loaded > > (I couldn't even navigate to them). I do not know why it is needed. > > '--no-sandbox' is added in the IO metric, thus this is never a problem for > > page_cycler. This is the stack trace on endure.py when I don't have sandbox: > > > > Stack Trace: > > > ******************************************************************************** > > [7:7:0904/104928:ERROR:shared_memory_posix.cc(213)] Creating shared memory in > > /dev/shm/org.chromium.Chromium.shmem.ChromiumStats2-4294967295 failed: > > Permission denied > > [7:7:0904/104928:ERROR:shared_memory_posix.cc(216)] Unable to > access(W_OK|X_OK) > > /dev/shm: Permission denied > > [7:7:0904/104928:FATAL:shared_memory_posix.cc(218)] This is frequently caused > > by incorrect permissions on /dev/shm. Try 'sudo chmod 1777 /dev/shm' to fix. > > [8:8:0904/104930:ERROR:shared_memory_posix.cc(213)] Creating shared memory in > > /dev/shm/org.chromium.Chromium.shmem.ChromiumStats2-4294967295 failed: > > Permission denied > > Ahh yes, this is required for --enable-stats-table currently (it must be set > elsewhere for the page_cycler). I have a change > (https://codereview.chromium.org/22911027/) which enables stats-table to be used > on Android, and also means that on Posix systems the --no-sandbox option is not > needed, but this hasn't landed yet. I think this is required for now, but maybe > add a TODO(rmcilroy) to only do this on windows when the CL lands. Done. /me wonders if there are other browser options that are platform-dependent in Telemetry. How would we do this? https://codereview.chromium.org/23963002/diff/1/tools/perf/metrics/v8_object_... tools/perf/metrics/v8_object_stats.py:172: counters = _COUNTER_NAMES On 2013/09/04 18:09:21, rmcilroy wrote: > On 2013/09/04 17:53:41, edmundyan wrote: > > On 2013/09/04 17:41:11, tonyg wrote: > > > Rather than this, can't you just do counters=_COUNTER_NAMES right in the > > > function arguments? > > > > That is what I did originally, but pylint didn't like it, saying " dangerous > > default value as argument". Technically, _COUNTER_NAMES is still a mutable > > list. > > Maybe just: > counters = counters or _COUNTER_NAMES Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edmundyan@chromium.org/23963002/7001
Retried try job too often on ios_dbg_simulator for step(s) ui_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edmundyan@chromium.org/23963002/7001
Retried try job too often on ios_dbg_simulator for step(s) ui_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edmundyan@chromium.org/23963002/7001
Message was sent while issue was closed.
Change committed as 221467 |
