Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2161363004:
[system-health] Tear down the browser after each story in system_health.memory_* (Closed)
|
Created:
4 years, 5 months ago by petrcermak Modified:
4 years, 5 months ago CC:
chromium-reviews, telemetry-reviews_chromium.org, perezju Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[system-health] Tear down the browser after each story in system_health.memory_*
Rationale: Each story should be completely independent as it will allow
us to compare the results from individual stories. As a result, it will
be possible to compare/subtract the results from, e.g.
LOAD:social:facebook and BROWSE:social:facebook.
More importantly, there are literally hundreds of perf dashboard alerts
each time we add a story to the SH story set. This should fix that.
*** NOTE TO PERF SHERIFF ***
This patch will definitely cause a lot of improvements and/or
regressions in system_health.memory_desktop and
system_health.memory_mobile benchmarks. This is expected.
BUG=589726
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
Committed: https://crrev.com/a30a0719cdc625d240ba75054fc0de1b9f826625
Cr-Commit-Position: refs/heads/master@{#407380}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 29 (14 generated)
Description was changed from ========== [system-health] Tear down the browser after each story in system_health.memory_* Rationale: Each story should be completely independent as it will allow us to compare the results from individual stories. As a result, it will be possible to compare/subtract the results from, e.g. LOAD:social:facebook and BROWSE:social:facebook. More importantly, there are literally hundreds of perf dashboard alerts each time we add a story to the SH story set. This should fix that. BUG=589726 ========== to ========== [system-health] Tear down the browser after each story in system_health.memory_* Rationale: Each story should be completely independent as it will allow us to compare the results from individual stories. As a result, it will be possible to compare/subtract the results from, e.g. LOAD:social:facebook and BROWSE:social:facebook. More importantly, there are literally hundreds of perf dashboard alerts each time we add a story to the SH story set. This should fix that. BUG=589726 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 ==========
petrcermak@chromium.org changed reviewers: + nednguyen@google.com, primiano@chromium.org
Primiano, Ned: PTAL. Charlie, Ulan: FYI. You might want to do the same in your SH benchmarks (in case you're not doing it already). Thanks, Petr
Description was changed from ========== [system-health] Tear down the browser after each story in system_health.memory_* Rationale: Each story should be completely independent as it will allow us to compare the results from individual stories. As a result, it will be possible to compare/subtract the results from, e.g. LOAD:social:facebook and BROWSE:social:facebook. More importantly, there are literally hundreds of perf dashboard alerts each time we add a story to the SH story set. This should fix that. BUG=589726 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] Tear down the browser after each story in system_health.memory_* Rationale: Each story should be completely independent as it will allow us to compare the results from individual stories. As a result, it will be possible to compare/subtract the results from, e.g. LOAD:social:facebook and BROWSE:social:facebook. More importantly, there are literally hundreds of perf dashboard alerts each time we add a story to the SH story set. This should fix that. *** NOTE TO PERF SHERIFF *** This patch will definitely cause a lot of improvements and/or regressions in system_health.memory_desktop and system_health.memory_mobile benchmarks. This is expected. BUG=589726 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 ==========
On 2016/07/21 10:37:28, petrcermak wrote: > Primiano, Ned: PTAL. > > Charlie, Ulan: FYI. You might want to do the same in your SH benchmarks (in case > you're not doing it already). > > Thanks, > Petr Thanks, v8_browsing does the same :)
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...
LGTM CC-ing Juan. I might remember wrong, but I thought that if we do this, as things are today, the bisect bot will not behave consistently? Can you please check that this flag is respected also there? Maybe I am just confusing with something else, but worth checking.
On 2016/07/21 10:54:36, Primiano Tucci wrote: > LGTM > > CC-ing Juan. I might remember wrong, but I thought that if we do this, as things > are today, the bisect bot will not behave consistently? On contrary, I think this will make the bisect bot behave more consistently as a failure on a single page will no longer affect perf number of other pages? > Can you please check > that this flag is respected also there? > Maybe I am just confusing with something else, but worth checking. Oh wow, this will increase the cycle time by a lot. Do you have before & after number?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_retina_perf_cq on master.tryserver.chromium.perf (JOB_TIMED_OUT, no build URL)
On 2016/07/21 11:23:13, nednguyen wrote: > Oh wow, this will increase the cycle time by a lot. Do you have before & after > number? I've just finished doing a local experiment and got the following results: *** Desktop (Linux) *** WITHOUT teardown ------> WITH teardown real 18m 54.613s real 20m 56.244s (+ 10.7%) user 8m 44.448s user 9m 0.692s sys 1m 40.782s sys 1m 36.224s (num_failed= 3 count) (num_failed= 1 count) *** Mobile (Nexus 5) *** WITHOUT teardown ------> WITH teardown real 31m 41.942s real 38m 48.004s (+ 22.4%) user 6m 22.155s user 6m 40.625s sys 1m 5.046s sys 3m 20.503s (num_failed= 2 count) (num_failed= 2 count) The differences are, as expected, non-negligible. Nevertheless, I'm convinced that we really need this. Also remember that this will NOT affect the smoke tests. WDYT? Petr
On 2016/07/21 19:27:15, petrcermak wrote: > On 2016/07/21 11:23:13, nednguyen wrote: > > Oh wow, this will increase the cycle time by a lot. Do you have before & after > > number? > > I've just finished doing a local experiment and got the following results: > > > *** Desktop (Linux) *** > WITHOUT teardown ------> WITH teardown > real 18m 54.613s real 20m 56.244s (+ 10.7%) > user 8m 44.448s user 9m 0.692s > sys 1m 40.782s sys 1m 36.224s > (num_failed= 3 count) (num_failed= 1 count) > > *** Mobile (Nexus 5) *** > WITHOUT teardown ------> WITH teardown > real 31m 41.942s real 38m 48.004s (+ 22.4%) > user 6m 22.155s user 6m 40.625s > sys 1m 5.046s sys 3m 20.503s > (num_failed= 2 count) (num_failed= 2 count) > > > The differences are, as expected, non-negligible. Nevertheless, I'm > convinced that we really need this. Also remember that this will NOT > affect the smoke tests. WDYT? > > Petr lgtm I was worried about 50%+ increase. For this type of increase, it's totally worth trading off cycle time for stability of the stories :-)
https://codereview.chromium.org/2161363004/diff/1/tools/perf/benchmarks/syste... File tools/perf/benchmarks/system_health.py (right): https://codereview.chromium.org/2161363004/diff/1/tools/perf/benchmarks/syste... tools/perf/benchmarks/system_health.py:19: class _SystemHealthBenchmark(perf_benchmark.PerfBenchmark): I forgot to comment, why not override ShouldTearDownStateAfterEachStoryRun here so that it applies to all system health benchmark by default?
The CQ bit was checked by petrcermak@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2161363004/diff/1/tools/perf/benchmarks/syste... File tools/perf/benchmarks/system_health.py (right): https://codereview.chromium.org/2161363004/diff/1/tools/perf/benchmarks/syste... tools/perf/benchmarks/system_health.py:19: class _SystemHealthBenchmark(perf_benchmark.PerfBenchmark): On 2016/07/21 20:08:50, nednguyen wrote: > I forgot to comment, why not override ShouldTearDownStateAfterEachStoryRun here > so that it applies to all system health benchmark by default? I decided not to do that because _MemorySystemHealthBenchmark currently doesn't inherit from _SystemHealthBenchmark (and I didn't want to change it in this patch because other methods would be inherited as well). Also note that all subclasses of _SystemHealthBenchmark are globally disabled.
The CQ bit was unchecked by petrcermak@chromium.org
On 2016/07/22 at 19:37:12, petrcermak wrote: > https://codereview.chromium.org/2161363004/diff/1/tools/perf/benchmarks/syste... > File tools/perf/benchmarks/system_health.py (right): > > https://codereview.chromium.org/2161363004/diff/1/tools/perf/benchmarks/syste... > tools/perf/benchmarks/system_health.py:19: class _SystemHealthBenchmark(perf_benchmark.PerfBenchmark): > On 2016/07/21 20:08:50, nednguyen wrote: > > I forgot to comment, why not override ShouldTearDownStateAfterEachStoryRun here > > so that it applies to all system health benchmark by default? > > I decided not to do that because _MemorySystemHealthBenchmark currently doesn't inherit from _SystemHealthBenchmark (and I didn't want to change it in this patch because other methods would be inherited as well). Also note that all subclasses of _SystemHealthBenchmark are globally disabled. Thanks for the heads-up Petr! Will definitely implement this for power also.
The CQ bit was checked by petrcermak@chromium.org
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...)
The CQ bit was checked by petrcermak@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [system-health] Tear down the browser after each story in system_health.memory_* Rationale: Each story should be completely independent as it will allow us to compare the results from individual stories. As a result, it will be possible to compare/subtract the results from, e.g. LOAD:social:facebook and BROWSE:social:facebook. More importantly, there are literally hundreds of perf dashboard alerts each time we add a story to the SH story set. This should fix that. *** NOTE TO PERF SHERIFF *** This patch will definitely cause a lot of improvements and/or regressions in system_health.memory_desktop and system_health.memory_mobile benchmarks. This is expected. BUG=589726 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] Tear down the browser after each story in system_health.memory_* Rationale: Each story should be completely independent as it will allow us to compare the results from individual stories. As a result, it will be possible to compare/subtract the results from, e.g. LOAD:social:facebook and BROWSE:social:facebook. More importantly, there are literally hundreds of perf dashboard alerts each time we add a story to the SH story set. This should fix that. *** NOTE TO PERF SHERIFF *** This patch will definitely cause a lot of improvements and/or regressions in system_health.memory_desktop and system_health.memory_mobile benchmarks. This is expected. BUG=589726 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 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== [system-health] Tear down the browser after each story in system_health.memory_* Rationale: Each story should be completely independent as it will allow us to compare the results from individual stories. As a result, it will be possible to compare/subtract the results from, e.g. LOAD:social:facebook and BROWSE:social:facebook. More importantly, there are literally hundreds of perf dashboard alerts each time we add a story to the SH story set. This should fix that. *** NOTE TO PERF SHERIFF *** This patch will definitely cause a lot of improvements and/or regressions in system_health.memory_desktop and system_health.memory_mobile benchmarks. This is expected. BUG=589726 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] Tear down the browser after each story in system_health.memory_* Rationale: Each story should be completely independent as it will allow us to compare the results from individual stories. As a result, it will be possible to compare/subtract the results from, e.g. LOAD:social:facebook and BROWSE:social:facebook. More importantly, there are literally hundreds of perf dashboard alerts each time we add a story to the SH story set. This should fix that. *** NOTE TO PERF SHERIFF *** This patch will definitely cause a lot of improvements and/or regressions in system_health.memory_desktop and system_health.memory_mobile benchmarks. This is expected. BUG=589726 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 Committed: https://crrev.com/a30a0719cdc625d240ba75054fc0de1b9f826625 Cr-Commit-Position: refs/heads/master@{#407380} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/a30a0719cdc625d240ba75054fc0de1b9f826625 Cr-Commit-Position: refs/heads/master@{#407380} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
