Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2744273003:
[Telemetry] Add benchmark that doesn't launch chrome and collects platform data. (Closed)
|
Created:
3 years, 9 months ago by rnephew (Reviews Here) Modified:
3 years, 8 months ago CC:
chromium-reviews, shrike, telemetry-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Telemetry] Add benchmark that doesn't launch chrome and collects platform data.
It will be useful to collect background power data on the platforms we run on
and will help reduce noise in power benchmarks.
BUG=700022
Review-Url: https://codereview.chromium.org/2744273003
Cr-Commit-Position: refs/heads/master@{#463697}
Committed: https://chromium.googlesource.com/chromium/src/+/20017d2b1fcf1d125715da9b32ea695600d45b07
Patch Set 1 : [Telemetry] Add benchmark that doesn't launch chrome and collects platform data. #
Total comments: 4
Patch Set 2 : page -> story #
Total comments: 2
Patch Set 3 : Disable cpu tracing and rebase and run generate_perf_json #
Total comments: 21
Patch Set 4 : [Telemetry] Add benchmark that doesn't launch chrome and collects platform data. #
Total comments: 11
Patch Set 5 : [Telemetry] Add benchmark that doesn't launch chrome and collects platform data. #Patch Set 6 : rebase and regen json #Patch Set 7 : fix archive_data_path from None to empty string #
Messages
Total messages: 53 (21 generated)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:1) has been deleted
rnephew@chromium.org changed reviewers: + charliea@chromium.org, nednguyen@google.com
Have you been able to run this locally? There is the check in https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... that should be removed first for this to work. https://codereview.chromium.org/2744273003/diff/60001/tools/perf/benchmarks/i... File tools/perf/benchmarks/idle_platform.py (right): https://codereview.chromium.org/2744273003/diff/60001/tools/perf/benchmarks/i... tools/perf/benchmarks/idle_platform.py:12: class IdlePlatformBenchmark(perf_benchmark.PerfBenchmark): Let put this benchmark in benchmarks/power.py? +Charlie: I think we should move all the power benchmark to power.py file. wdyt? https://codereview.chromium.org/2744273003/diff/60001/tools/perf/page_sets/id... File tools/perf/page_sets/idle_platform.py (right): https://codereview.chromium.org/2744273003/diff/60001/tools/perf/page_sets/id... tools/perf/page_sets/idle_platform.py:30: class IdlePlatformPage(page.Page): Let inherit this from story, so that you don't need to force in the "url" concept
On 2017/03/13 16:33:35, nednguyen wrote: > Have you been able to run this locally? There is the check in > https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... > that should be removed first for this to work. Yes, this runs locally for me.
https://codereview.chromium.org/2744273003/diff/60001/tools/perf/benchmarks/i... File tools/perf/benchmarks/idle_platform.py (right): https://codereview.chromium.org/2744273003/diff/60001/tools/perf/benchmarks/i... tools/perf/benchmarks/idle_platform.py:12: class IdlePlatformBenchmark(perf_benchmark.PerfBenchmark): On 2017/03/13 16:33:35, nednguyen wrote: > Let put this benchmark in benchmarks/power.py? > > +Charlie: I think we should move all the power benchmark to power.py file. wdyt? Waiting for Charlies response on this. https://codereview.chromium.org/2744273003/diff/60001/tools/perf/page_sets/id... File tools/perf/page_sets/idle_platform.py (right): https://codereview.chromium.org/2744273003/diff/60001/tools/perf/page_sets/id... tools/perf/page_sets/idle_platform.py:30: class IdlePlatformPage(page.Page): On 2017/03/13 16:33:35, nednguyen wrote: > Let inherit this from story, so that you don't need to force in the "url" > concept Done, but now requires https://codereview.chromium.org/2749533003/ to land. https://codereview.chromium.org/2744273003/diff/80001/tools/perf/page_sets/id... File tools/perf/page_sets/idle_platform.py (right): https://codereview.chromium.org/2744273003/diff/80001/tools/perf/page_sets/id... tools/perf/page_sets/idle_platform.py:11: def __init__(self, test, finder_options, story_set, idle_time=30): I keep flopping between thinking this should be here or in IdlePlatformPage(). Any strong opinions on this?
https://codereview.chromium.org/2744273003/diff/80001/tools/perf/page_sets/id... File tools/perf/page_sets/idle_platform.py (right): https://codereview.chromium.org/2744273003/diff/80001/tools/perf/page_sets/id... tools/perf/page_sets/idle_platform.py:11: def __init__(self, test, finder_options, story_set, idle_time=30): On 2017/03/13 21:29:18, rnephew (Reviews Here) wrote: > I keep flopping between thinking this should be here or in IdlePlatformPage(). > Any strong opinions on this? shared_state is construct by story_runner, so its constructor's signature must be exactly the same as https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...
On 2017/03/13 21:35:51, nednguyen wrote: > https://codereview.chromium.org/2744273003/diff/80001/tools/perf/page_sets/id... > File tools/perf/page_sets/idle_platform.py (right): > > https://codereview.chromium.org/2744273003/diff/80001/tools/perf/page_sets/id... > tools/perf/page_sets/idle_platform.py:11: def __init__(self, test, > finder_options, story_set, idle_time=30): > On 2017/03/13 21:29:18, rnephew (Reviews Here) wrote: > > I keep flopping between thinking this should be here or in IdlePlatformPage(). > > Any strong opinions on this? > > shared_state is construct by story_runner, so its constructor's signature must > be exactly the same as > https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... Have you figured out why https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... doesn't throw exception in your test? IIRC, IsChromeTracingSupported should only return True if there is Chrome enabled.
> Have you figured out why > https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... > doesn't throw exception in your test? IIRC, IsChromeTracingSupported should only > return True if there is Chrome enabled. There are two paths that lead to this working. tbm.py.IsCTSupported -> tracing_controller_backend.py.IsCTSupported -> CTAgent.IsSupported -> CTAgent.IsStartupTracingSupported() _STARTUP_TRACING_OS_NAMES = _DESKTOP_OS_NAMES + ['android', 'chromeos'] @classmethod def IsStartupTracingSupported(cls, platform_backend): if platform_backend.GetOSName() in _STARTUP_TRACING_OS_NAMES: return True else: return False @classmethod def IsSupported(cls, platform_backend): if cls.IsStartupTracingSupported(platform_backend): return True else: return chrome_tracing_devtools_manager.IsSupported(platform_backend) tbm.py.IsCTSupported -> tracing_controller_backend.py.IsCTSupported -> CTAgent.IsSupported -> CTAgent.IsStartupTracingSupported()->chrome_tracing_devtools_manager.IsSupported -> devtools_client_backend.IsCTSupported -> tracing_backend.IsTracingSupported @decorators.Cache def IsTracingSupported(self): req = {'method': 'Tracing.hasCompleted'} res = self._inspector_websocket.SyncRequest(req, timeout=10) return not res.get('response') IsChromeTracingSupported inside devtools_client_backend.py also runs: _CreateAndConnectBrowserInspectorWebsocketIfNeeded I assume that is why that path works. I was only running this on android (since thats where I have a BattOr connected locally). I ran it on my desktop and it is having clocksync issues: Exception: MapFunctionError: No clock sync markers exist pairing clock domain "BATTOR" with target clock domain "UNKNOWN_CHROME_LEGACY". at ClockSyncManager.getTimeTransformerRaw_ (/tracing/model/clock_sync_manager.html:173:15) at ClockSyncManager.getTimeTransformerError (/tracing/model/clock_sync_manager.html:166:19) at new clockSyncLatencyMetric (/tracing/metrics/system_health/clock_sync_latency_metric.html:27:46) at runMetrics (/usr/local/google/home/rnephew/chromium/clank2/src/third_party/catapult/tracing/tracing/metrics/metric_map_function.html:37:14) at metricMapFunction (/usr/local/google/home/rnephew/chromium/clank2/src/third_party/catapult/tracing/tracing/metrics/metric_map_function.html:66:22) at Object.mapSingleTrace (/tracing/mre/map_single_trace.html:40:7) at eval (/usr/local/google/home/rnephew/chromium/clank2/src/third_party/catapult/tracing/tracing/mre/map_single_trace_cmdline.html:60:18) at Object.runAndConvertErrorsToFailures (/tracing/mre/map_single_trace.html:25:10) at mapSingleTraceWithResult (/usr/local/google/home/rnephew/chromium/clank2/src/third_party/catapult/tracing/tracing/mre/map_single_trace_cmdline.html:51:12) at Object.mapSingleTraceMain (/usr/local/google/home/rnephew/chromium/clank2/src/third_party/catapult/tracing/tracing/mre/map_single_trace_cmdline.html:75:18) If I turn off cpu tracing there is no clock sync issue, so I think there may be a problem with how cpu tracings clock domain is collapsed in the absence of a chrome trace. Charliea@ you did some work on the CPU tracing agent, does that seem like a possibility?
On 2017/03/13 22:16:06, rnephew (Reviews Here) wrote: > > Have you figured out why > > > https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... > > doesn't throw exception in your test? IIRC, IsChromeTracingSupported should > only > > return True if there is Chrome enabled. > > There are two paths that lead to this working. > > tbm.py.IsCTSupported -> tracing_controller_backend.py.IsCTSupported -> > CTAgent.IsSupported -> CTAgent.IsStartupTracingSupported() > > _STARTUP_TRACING_OS_NAMES = _DESKTOP_OS_NAMES + ['android', 'chromeos'] > > @classmethod > def IsStartupTracingSupported(cls, platform_backend): > if platform_backend.GetOSName() in _STARTUP_TRACING_OS_NAMES: > return True > else: > return False > > @classmethod > def IsSupported(cls, platform_backend): > if cls.IsStartupTracingSupported(platform_backend): > return True > else: > return chrome_tracing_devtools_manager.IsSupported(platform_backend) > > tbm.py.IsCTSupported -> tracing_controller_backend.py.IsCTSupported -> > CTAgent.IsSupported -> > CTAgent.IsStartupTracingSupported()->chrome_tracing_devtools_manager.IsSupported > -> devtools_client_backend.IsCTSupported -> tracing_backend.IsTracingSupported > > @decorators.Cache > def IsTracingSupported(self): > req = {'method': 'Tracing.hasCompleted'} > res = self._inspector_websocket.SyncRequest(req, timeout=10) > return not res.get('response') > > IsChromeTracingSupported inside devtools_client_backend.py also runs: > _CreateAndConnectBrowserInspectorWebsocketIfNeeded > > I assume that is why that path works. > > > I was only running this on android (since thats where I have a BattOr connected > locally). I ran it on my desktop and it is having clocksync issues: > > Exception: MapFunctionError: No clock sync markers exist pairing clock domain > "BATTOR" with target clock domain "UNKNOWN_CHROME_LEGACY". > at ClockSyncManager.getTimeTransformerRaw_ > (/tracing/model/clock_sync_manager.html:173:15) > at ClockSyncManager.getTimeTransformerError > (/tracing/model/clock_sync_manager.html:166:19) > at new clockSyncLatencyMetric > (/tracing/metrics/system_health/clock_sync_latency_metric.html:27:46) > at runMetrics > (/usr/local/google/home/rnephew/chromium/clank2/src/third_party/catapult/tracing/tracing/metrics/metric_map_function.html:37:14) > at metricMapFunction > (/usr/local/google/home/rnephew/chromium/clank2/src/third_party/catapult/tracing/tracing/metrics/metric_map_function.html:66:22) > at Object.mapSingleTrace (/tracing/mre/map_single_trace.html:40:7) > at eval > (/usr/local/google/home/rnephew/chromium/clank2/src/third_party/catapult/tracing/tracing/mre/map_single_trace_cmdline.html:60:18) > at Object.runAndConvertErrorsToFailures > (/tracing/mre/map_single_trace.html:25:10) > at mapSingleTraceWithResult > (/usr/local/google/home/rnephew/chromium/clank2/src/third_party/catapult/tracing/tracing/mre/map_single_trace_cmdline.html:51:12) > at Object.mapSingleTraceMain > (/usr/local/google/home/rnephew/chromium/clank2/src/third_party/catapult/tracing/tracing/mre/map_single_trace_cmdline.html:75:18) > > > If I turn off cpu tracing there is no clock sync issue, so I think there may be > a problem with how cpu tracings clock domain is collapsed in the absence of a > chrome trace. Charliea@ you did some work on the CPU tracing agent, does that > seem like a possibility? I'm not sure why this is. Can you email me a trace exhibiting the problem? I think I'm going to make an easy way to view the clock sync graph of a trace as a dot graph: it's clear at this point that we're going to continue to run into problems like this.
(Note: I also filed https://github.com/catapult-project/catapult/issues/3388 as a suggested workaround for problems like this in the future.)
s/workaround/debugging tool
On 2017/03/13 22:16:06, rnephew (Reviews Here) wrote: > > Have you figured out why > > > https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... > > doesn't throw exception in your test? IIRC, IsChromeTracingSupported should > only > > return True if there is Chrome enabled. > > There are two paths that lead to this working. > > tbm.py.IsCTSupported -> tracing_controller_backend.py.IsCTSupported -> > CTAgent.IsSupported -> CTAgent.IsStartupTracingSupported() > > _STARTUP_TRACING_OS_NAMES = _DESKTOP_OS_NAMES + ['android', 'chromeos'] > > @classmethod > def IsStartupTracingSupported(cls, platform_backend): > if platform_backend.GetOSName() in _STARTUP_TRACING_OS_NAMES: > return True > else: > return False Ah right. This is because of startup tracing, so this is almost always True. > > @classmethod > def IsSupported(cls, platform_backend): > if cls.IsStartupTracingSupported(platform_backend): > return True > else: > return chrome_tracing_devtools_manager.IsSupported(platform_backend) > > tbm.py.IsCTSupported -> tracing_controller_backend.py.IsCTSupported -> > CTAgent.IsSupported -> > CTAgent.IsStartupTracingSupported()->chrome_tracing_devtools_manager.IsSupported > -> devtools_client_backend.IsCTSupported -> tracing_backend.IsTracingSupported > > @decorators.Cache > def IsTracingSupported(self): > req = {'method': 'Tracing.hasCompleted'} > res = self._inspector_websocket.SyncRequest(req, timeout=10) > return not res.get('response') > > IsChromeTracingSupported inside devtools_client_backend.py also runs: > _CreateAndConnectBrowserInspectorWebsocketIfNeeded > > I assume that is why that path works. > > > I was only running this on android (since thats where I have a BattOr connected > locally). I ran it on my desktop and it is having clocksync issues: > > Exception: MapFunctionError: No clock sync markers exist pairing clock domain > "BATTOR" with target clock domain "UNKNOWN_CHROME_LEGACY". > at ClockSyncManager.getTimeTransformerRaw_ > (/tracing/model/clock_sync_manager.html:173:15) > at ClockSyncManager.getTimeTransformerError > (/tracing/model/clock_sync_manager.html:166:19) > at new clockSyncLatencyMetric > (/tracing/metrics/system_health/clock_sync_latency_metric.html:27:46) > at runMetrics > (/usr/local/google/home/rnephew/chromium/clank2/src/third_party/catapult/tracing/tracing/metrics/metric_map_function.html:37:14) > at metricMapFunction > (/usr/local/google/home/rnephew/chromium/clank2/src/third_party/catapult/tracing/tracing/metrics/metric_map_function.html:66:22) > at Object.mapSingleTrace (/tracing/mre/map_single_trace.html:40:7) > at eval > (/usr/local/google/home/rnephew/chromium/clank2/src/third_party/catapult/tracing/tracing/mre/map_single_trace_cmdline.html:60:18) > at Object.runAndConvertErrorsToFailures > (/tracing/mre/map_single_trace.html:25:10) > at mapSingleTraceWithResult > (/usr/local/google/home/rnephew/chromium/clank2/src/third_party/catapult/tracing/tracing/mre/map_single_trace_cmdline.html:51:12) > at Object.mapSingleTraceMain > (/usr/local/google/home/rnephew/chromium/clank2/src/third_party/catapult/tracing/tracing/mre/map_single_trace_cmdline.html:75:18) > > > If I turn off cpu tracing there is no clock sync issue, so I think there may be > a problem with how cpu tracings clock domain is collapsed in the absence of a > chrome trace. Charliea@ you did some work on the CPU tracing agent, does that > seem like a possibility?
I went ahead and just disabled cpu tracing agent for this test until we figure out what is going wrong with the clock domain collapsing. PTAL.
charliea@chromium.org changed required reviewers: + nednguyen@google.com
lgtm, but I defer to Ned for ultimate approval https://codereview.chromium.org/2744273003/diff/100001/tools/perf/benchmarks/... File tools/perf/benchmarks/idle_platform.py (right): https://codereview.chromium.org/2744273003/diff/100001/tools/perf/benchmarks/... tools/perf/benchmarks/idle_platform.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2744273003/diff/100001/tools/perf/benchmarks/... tools/perf/benchmarks/idle_platform.py:15: This benchmark just starts up tracing agents and lets the platform sit idle. Please include information about _why_ we want this benchmark. This is always important, but especially important for this benchmark because I could see pretty rational people thinking this is a dumb benchmark. (it's not) https://codereview.chromium.org/2744273003/diff/100001/tools/perf/benchmarks/... tools/perf/benchmarks/idle_platform.py:22: options.config.enable_cpu_trace = False Instead of options.config.enable_cpu_trace = False, which is the default, could you please file a crbug against me to investigate the clock sync problem and just add a TODO to enable the CPU trace? https://codereview.chromium.org/2744273003/diff/100001/tools/perf/benchmarks/... tools/perf/benchmarks/idle_platform.py:23: options.config.enable_chrome_trace = False nit: no need to respecify the default https://codereview.chromium.org/2744273003/diff/100001/tools/perf/page_sets/i... File tools/perf/page_sets/idle_platform.py (right): https://codereview.chromium.org/2744273003/diff/100001/tools/perf/page_sets/i... tools/perf/page_sets/idle_platform.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2744273003/diff/100001/tools/perf/page_sets/i... tools/perf/page_sets/idle_platform.py:11: def __init__(self, test, finder_options, story_set, idle_time=30): nit: generally, when we're talking about a length of time, I prefer to use "duration" over "time" because it's more specific https://codereview.chromium.org/2744273003/diff/100001/tools/perf/page_sets/i... tools/perf/page_sets/idle_platform.py:11: def __init__(self, test, finder_options, story_set, idle_time=30): nit: IMO prefixing "duration" with "idle_" is redundant. If we're talking about an IdleSharedState, and it has a duration attribute, I think it's implied that the duration is the amount of time that the thing sits idle for
https://codereview.chromium.org/2744273003/diff/100001/tools/perf/benchmarks/... File tools/perf/benchmarks/idle_platform.py (right): https://codereview.chromium.org/2744273003/diff/100001/tools/perf/benchmarks/... tools/perf/benchmarks/idle_platform.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. this should be move to power.py file https://codereview.chromium.org/2744273003/diff/100001/tools/perf/benchmarks/... tools/perf/benchmarks/idle_platform.py:20: options.config.chrome_trace_config.category_filter.AddFilterString('rail') we don't enable Chrome trace, so there is no use in adding chrome categories https://codereview.chromium.org/2744273003/diff/100001/tools/perf/page_sets/i... File tools/perf/page_sets/idle_platform.py (right): https://codereview.chromium.org/2744273003/diff/100001/tools/perf/page_sets/i... tools/perf/page_sets/idle_platform.py:11: def __init__(self, test, finder_options, story_set, idle_time=30): this idle_time variable should either be a const or a variable pulled from the story. Having it in the constructor of _IdleSharedState is misleading, because this is always instantiate with only 4 params: test, finder_options, story_set https://codereview.chromium.org/2744273003/diff/100001/tools/perf/page_sets/i... tools/perf/page_sets/idle_platform.py:36: class IdlePlatformPage(story.Story): IdleStory https://codereview.chromium.org/2744273003/diff/100001/tools/perf/page_sets/i... tools/perf/page_sets/idle_platform.py:49: cloud_storage_bucket=story.PARTNER_BUCKET) you shouldn't need cloud_storage_bucket defined.
https://codereview.chromium.org/2744273003/diff/100001/tools/perf/benchmarks/... File tools/perf/benchmarks/idle_platform.py (right): https://codereview.chromium.org/2744273003/diff/100001/tools/perf/benchmarks/... tools/perf/benchmarks/idle_platform.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2017/03/20 16:54:35, nednguyen wrote: > this should be move to power.py file Done. https://codereview.chromium.org/2744273003/diff/100001/tools/perf/benchmarks/... tools/perf/benchmarks/idle_platform.py:15: This benchmark just starts up tracing agents and lets the platform sit idle. On 2017/03/20 14:20:25, charliea wrote: > Please include information about _why_ we want this benchmark. This is always > important, but especially important for this benchmark because I could see > pretty rational people thinking this is a dumb benchmark. (it's not) Done. https://codereview.chromium.org/2744273003/diff/100001/tools/perf/benchmarks/... tools/perf/benchmarks/idle_platform.py:20: options.config.chrome_trace_config.category_filter.AddFilterString('rail') On 2017/03/20 16:54:35, nednguyen wrote: > we don't enable Chrome trace, so there is no use in adding chrome categories Done. https://codereview.chromium.org/2744273003/diff/100001/tools/perf/benchmarks/... tools/perf/benchmarks/idle_platform.py:22: options.config.enable_cpu_trace = False On 2017/03/20 14:20:25, charliea wrote: > Instead of options.config.enable_cpu_trace = False, which is the default, could > you please file a crbug against me to investigate the clock sync problem and > just add a TODO to enable the CPU trace? https://github.com/catapult-project/catapult/issues/3463 https://codereview.chromium.org/2744273003/diff/100001/tools/perf/benchmarks/... tools/perf/benchmarks/idle_platform.py:23: options.config.enable_chrome_trace = False On 2017/03/20 14:20:25, charliea wrote: > nit: no need to respecify the default It actually is required. It was failing to clock sync chrome when I get rid of htat line. Printing out the stack in the setter I determined it was getting overwritten here: https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... https://codereview.chromium.org/2744273003/diff/100001/tools/perf/page_sets/i... File tools/perf/page_sets/idle_platform.py (right): https://codereview.chromium.org/2744273003/diff/100001/tools/perf/page_sets/i... tools/perf/page_sets/idle_platform.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2017/03/20 14:20:25, charliea wrote: > 2017 Done. https://codereview.chromium.org/2744273003/diff/100001/tools/perf/page_sets/i... tools/perf/page_sets/idle_platform.py:11: def __init__(self, test, finder_options, story_set, idle_time=30): On 2017/03/20 16:54:35, nednguyen wrote: > this idle_time variable should either be a const or a variable pulled from the > story. Having it in the constructor of _IdleSharedState is misleading, because > this is always instantiate with only 4 params: test, finder_options, story_set Done. https://codereview.chromium.org/2744273003/diff/100001/tools/perf/page_sets/i... tools/perf/page_sets/idle_platform.py:36: class IdlePlatformPage(story.Story): On 2017/03/20 16:54:35, nednguyen wrote: > IdleStory Done. https://codereview.chromium.org/2744273003/diff/100001/tools/perf/page_sets/i... tools/perf/page_sets/idle_platform.py:49: cloud_storage_bucket=story.PARTNER_BUCKET) On 2017/03/20 16:54:35, nednguyen wrote: > you shouldn't need cloud_storage_bucket defined. Done.
https://codereview.chromium.org/2744273003/diff/120001/tools/perf/benchmarks/... File tools/perf/benchmarks/power.py (right): https://codereview.chromium.org/2744273003/diff/120001/tools/perf/benchmarks/... tools/perf/benchmarks/power.py:256: Should I add this to the benchmark, or just let it run everywhere without battors even? @classmethod def ShouldDisable(cls, possible_browser): return not possible_browser.platform.HasBattOrConnected() If we get system level tracing, it could still be useful without battors. We only have that with android atrace for now though.
Mostly lg2me https://codereview.chromium.org/2744273003/diff/120001/tools/perf/benchmarks/... File tools/perf/benchmarks/power.py (right): https://codereview.chromium.org/2744273003/diff/120001/tools/perf/benchmarks/... tools/perf/benchmarks/power.py:256: On 2017/03/30 21:55:34, rnephew (Reviews Here) wrote: > Should I add this to the benchmark, or just let it run everywhere without > battors even? > > @classmethod > def ShouldDisable(cls, possible_browser): > return not possible_browser.platform.HasBattOrConnected() > > If we get system level tracing, it could still be useful without battors. We > only have that with android atrace for now though. Not really. We don't have any metric for atrace only, so nothing but the benchmark timing will be output. https://codereview.chromium.org/2744273003/diff/120001/tools/perf/page_sets/i... File tools/perf/page_sets/idle_platform.py (right): https://codereview.chromium.org/2744273003/diff/120001/tools/perf/page_sets/i... tools/perf/page_sets/idle_platform.py:21: # benchmarks. We don't need to worry about supporting this for legacy_page_test benchmark. I think you can just add assert that tracing is running.
Description was changed from ========== [Telemetry] Add benchmark that doesn't launch chrome and collects platform data. It will be useful to collect background power data on the platforms we run on and will help reduce noise in power benchmarks. BUG=700022 ========== to ========== [Telemetry] Add benchmark that doesn't launch chrome and collects platform data. It will be useful to collect background power data on the platforms we run on and will help reduce noise in power benchmarks. BUG=700022 ==========
On 2017/03/30 22:01:40, nednguyen wrote: > Mostly lg2me > > https://codereview.chromium.org/2744273003/diff/120001/tools/perf/benchmarks/... > File tools/perf/benchmarks/power.py (right): > > https://codereview.chromium.org/2744273003/diff/120001/tools/perf/benchmarks/... > tools/perf/benchmarks/power.py:256: > On 2017/03/30 21:55:34, rnephew (Reviews Here) wrote: > > Should I add this to the benchmark, or just let it run everywhere without > > battors even? > > > > @classmethod > > def ShouldDisable(cls, possible_browser): > > return not possible_browser.platform.HasBattOrConnected() > > > > If we get system level tracing, it could still be useful without battors. We > > only have that with android atrace for now though. > > Not really. We don't have any metric for atrace only, so nothing but the > benchmark timing will be output. > > https://codereview.chromium.org/2744273003/diff/120001/tools/perf/page_sets/i... > File tools/perf/page_sets/idle_platform.py (right): > > https://codereview.chromium.org/2744273003/diff/120001/tools/perf/page_sets/i... > tools/perf/page_sets/idle_platform.py:21: # benchmarks. > We don't need to worry about supporting this for legacy_page_test benchmark. I > think you can just add assert that tracing is running. Cc'ed Shrike & Erik: this is a special power benchmark that do nothing & use battor to measure the power for 30s. We use this to check how stable our bot's power usage is & use this to get a baseline power of the system.
On 2017/03/30 22:04:06, nednguyen wrote: > On 2017/03/30 22:01:40, nednguyen wrote: > > Mostly lg2me > > > > > https://codereview.chromium.org/2744273003/diff/120001/tools/perf/benchmarks/... > > File tools/perf/benchmarks/power.py (right): > > > > > https://codereview.chromium.org/2744273003/diff/120001/tools/perf/benchmarks/... > > tools/perf/benchmarks/power.py:256: > > On 2017/03/30 21:55:34, rnephew (Reviews Here) wrote: > > > Should I add this to the benchmark, or just let it run everywhere without > > > battors even? > > > > > > @classmethod > > > def ShouldDisable(cls, possible_browser): > > > return not possible_browser.platform.HasBattOrConnected() > > > > > > If we get system level tracing, it could still be useful without battors. We > > > only have that with android atrace for now though. > > > > Not really. We don't have any metric for atrace only, so nothing but the > > benchmark timing will be output. > > > > > https://codereview.chromium.org/2744273003/diff/120001/tools/perf/page_sets/i... > > File tools/perf/page_sets/idle_platform.py (right): > > > > > https://codereview.chromium.org/2744273003/diff/120001/tools/perf/page_sets/i... > > tools/perf/page_sets/idle_platform.py:21: # benchmarks. > > We don't need to worry about supporting this for legacy_page_test benchmark. I > > think you can just add assert that tracing is running. > > Cc'ed Shrike & Erik: this is a special power benchmark that do nothing & use > battor to measure the power for 30s. We use this to check how stable our bot's > power usage is & use this to get a baseline power of the system. Thanks - that sounds really helpful. :)
https://codereview.chromium.org/2744273003/diff/120001/tools/perf/page_sets/i... File tools/perf/page_sets/idle_platform.py (right): https://codereview.chromium.org/2744273003/diff/120001/tools/perf/page_sets/i... tools/perf/page_sets/idle_platform.py:31: class IdleStory(story.Story): make this private: _IdleStory https://codereview.chromium.org/2744273003/diff/120001/tools/perf/page_sets/i... tools/perf/page_sets/idle_platform.py:32: def __init__(self, name, duration=30): No need to set a default param here if this is only internal. https://codereview.chromium.org/2744273003/diff/120001/tools/perf/page_sets/i... tools/perf/page_sets/idle_platform.py:45: self.AddStory(IdleStory('Idle Platform')) Can you create 3 stories: self.AddStory(IdleStory('IdleStory_10s'), 10) self.AddStory(IdleStory('IdleStory_60s'), 60) self.AddStory(IdleStory('IdleStory_120s'), 120)
https://codereview.chromium.org/2744273003/diff/120001/tools/perf/benchmarks/... File tools/perf/benchmarks/power.py (right): https://codereview.chromium.org/2744273003/diff/120001/tools/perf/benchmarks/... tools/perf/benchmarks/power.py:256: On 2017/03/30 22:01:40, nednguyen wrote: > On 2017/03/30 21:55:34, rnephew (Reviews Here) wrote: > > Should I add this to the benchmark, or just let it run everywhere without > > battors even? > > > > @classmethod > > def ShouldDisable(cls, possible_browser): > > return not possible_browser.platform.HasBattOrConnected() > > > > If we get system level tracing, it could still be useful without battors. We > > only have that with android atrace for now though. > > Not really. We don't have any metric for atrace only, so nothing but the > benchmark timing will be output. Ok, since there is no atrace only metrics I went ahead and added the ShouldDisable so it only runs with a BattOr attached. https://codereview.chromium.org/2744273003/diff/120001/tools/perf/page_sets/i... File tools/perf/page_sets/idle_platform.py (right): https://codereview.chromium.org/2744273003/diff/120001/tools/perf/page_sets/i... tools/perf/page_sets/idle_platform.py:21: # benchmarks. On 2017/03/30 22:01:40, nednguyen wrote: > We don't need to worry about supporting this for legacy_page_test benchmark. I > think you can just add assert that tracing is running. Done. https://codereview.chromium.org/2744273003/diff/120001/tools/perf/page_sets/i... tools/perf/page_sets/idle_platform.py:31: class IdleStory(story.Story): On 2017/03/30 22:07:56, nednguyen wrote: > make this private: _IdleStory Done. https://codereview.chromium.org/2744273003/diff/120001/tools/perf/page_sets/i... tools/perf/page_sets/idle_platform.py:32: def __init__(self, name, duration=30): On 2017/03/30 22:07:55, nednguyen wrote: > No need to set a default param here if this is only internal. Done. https://codereview.chromium.org/2744273003/diff/120001/tools/perf/page_sets/i... tools/perf/page_sets/idle_platform.py:45: self.AddStory(IdleStory('Idle Platform')) On 2017/03/30 22:07:56, nednguyen wrote: > Can you create 3 stories: > self.AddStory(IdleStory('IdleStory_10s'), 10) > self.AddStory(IdleStory('IdleStory_60s'), 60) > self.AddStory(IdleStory('IdleStory_120s'), 120) Done.
Ping.
lgtm
The CQ bit was checked by rnephew@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from charliea@chromium.org Link to the patchset: https://codereview.chromium.org/2744273003/#ps140001 (title: "[Telemetry] Add benchmark that doesn't launch chrome and collects platform data.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by rnephew@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, charliea@chromium.org Link to the patchset: https://codereview.chromium.org/2744273003/#ps160001 (title: "rebase and regen json")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rnephew@chromium.org to run a CQ dry run
This is failing with: AssertionError: story_set's archive_data_file path must have type string It looks like it defaults to '', so I'm getting rid of the part where I set it to None. Ned, I am going to start a dry run after uploading this fix, can you confirm that that is the correct thing to do before I actually land it?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/11 16:58:37, rnephew (Reviews Here) wrote: > This is failing with: > AssertionError: story_set's archive_data_file path must have type string > > It looks like it defaults to '', so I'm getting rid of the part where I set it > to None. Ned, I am going to start a dry run after uploading this fix, can you > confirm that that is the correct thing to do before I actually land it? Yup, sgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rnephew@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, charliea@chromium.org Link to the patchset: https://codereview.chromium.org/2744273003/#ps180001 (title: "fix archive_data_path from None to empty string")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1491934865671310,
"parent_rev": "8ee0be28f0383dc0c4e5a6062bf7dd87df956cbd", "commit_rev":
"20017d2b1fcf1d125715da9b32ea695600d45b07"}
Message was sent while issue was closed.
Description was changed from ========== [Telemetry] Add benchmark that doesn't launch chrome and collects platform data. It will be useful to collect background power data on the platforms we run on and will help reduce noise in power benchmarks. BUG=700022 ========== to ========== [Telemetry] Add benchmark that doesn't launch chrome and collects platform data. It will be useful to collect background power data on the platforms we run on and will help reduce noise in power benchmarks. BUG=700022 Review-Url: https://codereview.chromium.org/2744273003 Cr-Commit-Position: refs/heads/master@{#463697} Committed: https://chromium.googlesource.com/chromium/src/+/20017d2b1fcf1d125715da9b32ea... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001) as https://chromium.googlesource.com/chromium/src/+/20017d2b1fcf1d125715da9b32ea...
Message was sent while issue was closed.
martiniss@chromium.org changed reviewers: + martiniss@chromium.org
Message was sent while issue was closed.
This is broken on the waterfall; see https://chromium-swarm.appspot.com/task?id=357923fcc8329710&refresh=10&show_r... for an example failure. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
