Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2203303002:
[system-health] Re-enable non-verbose logging in System Health memory benchmarks (Closed)
|
Created:
4 years, 4 months ago by petrcermak Modified:
4 years, 4 months ago Reviewers:
nednguyen CC:
chromium-reviews, telemetry-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[system-health] Re-enable non-verbose logging in System Health memory benchmarks
Rationale: The Chrome issue on Windows that caused this to crash has
been fixed (https://codereview.chromium.org/2198603002).
*** NOTE TO PERF SHERIFF ***
This patch is likely to cause regressions in
system_health.memory_desktop and
system_health.memory_mobile benchmarks. This is expected.
BUG=625172, 623058
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq
Patch Set 1 #
Total comments: 7
Messages
Total messages: 25 (19 generated)
Description was changed from ========== [system-health] Re-enable non-verbose logging in System Health memory benchmarks Rationale: The Chrome issue on Windows that caused this to crash has been fixed (https://codereview.chromium.org/2198603002). BUG=625172,623058 ========== to ========== [system-health] Re-enable non-verbose logging in System Health memory benchmarks Rationale: The Chrome issue on Windows that caused this to crash has been fixed (https://codereview.chromium.org/2198603002). BUG=625172,623058 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ==========
Description was changed from ========== [system-health] Re-enable non-verbose logging in System Health memory benchmarks Rationale: The Chrome issue on Windows that caused this to crash has been fixed (https://codereview.chromium.org/2198603002). BUG=625172,623058 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ========== to ========== [system-health] Re-enable non-verbose logging in System Health memory benchmarks Rationale: The Chrome issue on Windows that caused this to crash has been fixed (https://codereview.chromium.org/2198603002). *** NOTE TO PERF SHERIFF *** This patch is likely to cause regressions in system_health.memory_desktop and system_health.memory_mobile benchmarks. This is expected. BUG=625172,623058 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ==========
The CQ bit was checked by petrcermak@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
petrcermak@chromium.org changed reviewers: + nednguyen@google.com
PTAL. Thanks, Petr
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by petrcermak@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by petrcermak@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: android_s5_perf_cq on master.tryserver.chromium.perf (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by petrcermak@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2203303002/diff/1/tools/perf/benchmarks/syste... File tools/perf/benchmarks/system_health.py (right): https://codereview.chromium.org/2203303002/diff/1/tools/perf/benchmarks/syste... tools/perf/benchmarks/system_health.py:69: options.logging_verbosity = options.NON_VERBOSE_LOGGING Do we still crash failure that are not caught by cq tests? My hope is that we don't need to do this to reduce extra-overhead on the browser. https://codereview.chromium.org/2203303002/diff/1/tools/perf/benchmarks/syste... File tools/perf/benchmarks/system_health_smoke_test.py (left): https://codereview.chromium.org/2203303002/diff/1/tools/perf/benchmarks/syste... tools/perf/benchmarks/system_health_smoke_test.py:110: options.logging_verbosity = 'non-verbose' Why remove the log here? These are convenient for diagnosing the failure if any of the test fails.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: winx64_10_perf_cq on master.tryserver.chromium.perf (JOB_TIMED_OUT, no build URL)
https://codereview.chromium.org/2203303002/diff/1/tools/perf/benchmarks/syste... File tools/perf/benchmarks/system_health.py (right): https://codereview.chromium.org/2203303002/diff/1/tools/perf/benchmarks/syste... tools/perf/benchmarks/system_health.py:69: options.logging_verbosity = options.NON_VERBOSE_LOGGING On 2016/08/03 12:31:13, nednguyen wrote: > Do we still crash failure that are not caught by cq tests? My hope is that we > don't need to do this to reduce extra-overhead on the browser. Actually, apart from the disabled smoke tests, we only have 2 failures on reference builds (desktop-reference + n5x-reference). Everythin else is enabled. Are you suggesting that we only enable logging in the smoke tests? https://codereview.chromium.org/2203303002/diff/1/tools/perf/benchmarks/syste... File tools/perf/benchmarks/system_health_smoke_test.py (left): https://codereview.chromium.org/2203303002/diff/1/tools/perf/benchmarks/syste... tools/perf/benchmarks/system_health_smoke_test.py:110: options.logging_verbosity = 'non-verbose' On 2016/08/03 12:31:13, nednguyen wrote: > Why remove the log here? These are convenient for diagnosing the failure if any > of the test fails. No need to do it here when SetExtraBrowserOptions above would do it.
https://codereview.chromium.org/2203303002/diff/1/tools/perf/benchmarks/syste... File tools/perf/benchmarks/system_health.py (right): https://codereview.chromium.org/2203303002/diff/1/tools/perf/benchmarks/syste... tools/perf/benchmarks/system_health.py:69: options.logging_verbosity = options.NON_VERBOSE_LOGGING On 2016/08/03 13:05:35, petrcermak wrote: > On 2016/08/03 12:31:13, nednguyen wrote: > > Do we still crash failure that are not caught by cq tests? My hope is that we > > don't need to do this to reduce extra-overhead on the browser. > > Actually, apart from the disabled smoke tests, we only have 2 failures on > reference builds (desktop-reference + n5x-reference). Everythin else is enabled. > Are you suggesting that we only enable logging in the smoke tests? Yes. I hope the smoke tests would defend for all crashes, hence hopefully removes the need to enable logging in production mode. https://codereview.chromium.org/2203303002/diff/1/tools/perf/benchmarks/syste... File tools/perf/benchmarks/system_health_smoke_test.py (left): https://codereview.chromium.org/2203303002/diff/1/tools/perf/benchmarks/syste... tools/perf/benchmarks/system_health_smoke_test.py:110: options.logging_verbosity = 'non-verbose' On 2016/08/03 13:05:35, petrcermak wrote: > On 2016/08/03 12:31:13, nednguyen wrote: > > Why remove the log here? These are convenient for diagnosing the failure if > any > > of the test fails. > > No need to do it here when SetExtraBrowserOptions above would do it. I see.
https://codereview.chromium.org/2203303002/diff/1/tools/perf/benchmarks/syste... File tools/perf/benchmarks/system_health.py (right): https://codereview.chromium.org/2203303002/diff/1/tools/perf/benchmarks/syste... tools/perf/benchmarks/system_health.py:69: options.logging_verbosity = options.NON_VERBOSE_LOGGING On 2016/08/03 13:13:26, nednguyen wrote: > On 2016/08/03 13:05:35, petrcermak wrote: > > On 2016/08/03 12:31:13, nednguyen wrote: > > > Do we still crash failure that are not caught by cq tests? My hope is that > we > > > don't need to do this to reduce extra-overhead on the browser. > > > > Actually, apart from the disabled smoke tests, we only have 2 failures on > > reference builds (desktop-reference + n5x-reference). Everythin else is > enabled. > > Are you suggesting that we only enable logging in the smoke tests? > > Yes. I hope the smoke tests would defend for all crashes, hence hopefully > removes the need to enable logging in production mode. Makes sense. In that case, let's abandon this patch.
Message was sent while issue was closed.
On 2016/08/03 13:16:40, petrcermak wrote: > https://codereview.chromium.org/2203303002/diff/1/tools/perf/benchmarks/syste... > File tools/perf/benchmarks/system_health.py (right): > > https://codereview.chromium.org/2203303002/diff/1/tools/perf/benchmarks/syste... > tools/perf/benchmarks/system_health.py:69: options.logging_verbosity = > options.NON_VERBOSE_LOGGING > On 2016/08/03 13:13:26, nednguyen wrote: > > On 2016/08/03 13:05:35, petrcermak wrote: > > > On 2016/08/03 12:31:13, nednguyen wrote: > > > > Do we still crash failure that are not caught by cq tests? My hope is that > > we > > > > don't need to do this to reduce extra-overhead on the browser. > > > > > > Actually, apart from the disabled smoke tests, we only have 2 failures on > > > reference builds (desktop-reference + n5x-reference). Everythin else is > > enabled. > > > Are you suggesting that we only enable logging in the smoke tests? > > > > Yes. I hope the smoke tests would defend for all crashes, hence hopefully > > removes the need to enable logging in production mode. > > Makes sense. In that case, let's abandon this patch. sgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
