|
|
DescriptionSupport flag --tabset-repeat on benchmark tab_switching
flag --tabset-repeat multiply the tab count of benchmark tab_switching
to measure metrics on different tab count. ex: add flag --tabset-repeat=2
to double the tab count.
BUG=689388
Review-Url: https://codereview.chromium.org/2819423002
Cr-Commit-Position: refs/heads/master@{#466852}
Committed: https://chromium.googlesource.com/chromium/src/+/ef1307664210fb58e9b9fb85586704ca1d5e59bc
Patch Set 1 #
Total comments: 5
Patch Set 2 : fixing style #
Messages
Total messages: 35 (19 generated)
The CQ bit was checked by vovoy@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...
Description was changed from ========== Support flag --tabset-repeat on benchmark tab_switching flag --tabset-repeat multiply the tab count of benchmark tab_switching to measure metrics on different tab count BUG=689388 ========== to ========== Support flag --tabset-repeat on benchmark tab_switching flag --tabset-repeat multiply the tab count of benchmark tab_switching to measure metrics on different tab count BUG=689388 ==========
vovoy@chromium.org changed reviewers: + nednguyen@google.com, perezju@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Support flag --tabset-repeat on benchmark tab_switching flag --tabset-repeat multiply the tab count of benchmark tab_switching to measure metrics on different tab count BUG=689388 ========== to ========== Support flag --tabset-repeat on benchmark tab_switching flag --tabset-repeat multiply the tab count of benchmark tab_switching to measure metrics on different tab count. ex: add flag --tabset-repeat=2 to double the tab count. BUG=689388 ==========
it's the second part of Benchmark Tab Switching Refinement design doc: https://docs.google.com/document/d/1rMrrlW4-5ZGD9dHxUYBOzzkS-vyX0_mtiJ-CMT2eg...
On 2017/04/18 05:46:28, vovoy wrote: > it's the second part of Benchmark Tab Switching Refinement > > design doc: > https://docs.google.com/document/d/1rMrrlW4-5ZGD9dHxUYBOzzkS-vyX0_mtiJ-CMT2eg... How long does it take to run this with tabset_repeat=<the number you want?> If the cycle time is reasonable, I think it would be ok to just hard code that number in system_health/multi_tab_stories.py or in the tab_switching.TabSwitching benchmark
On 2017/04/18 11:17:30, nednguyen wrote: > On 2017/04/18 05:46:28, vovoy wrote: > > it's the second part of Benchmark Tab Switching Refinement > > > > design doc: > > > https://docs.google.com/document/d/1rMrrlW4-5ZGD9dHxUYBOzzkS-vyX0_mtiJ-CMT2eg... > > How long does it take to run this with tabset_repeat=<the number you want?> If > the cycle time is reasonable, I think it would be ok to just hard code that > number in system_health/multi_tab_stories.py or in the > tab_switching.TabSwitching benchmark Also stepping back a bit, what is the rationale for having this option?
On 2017/04/18 12:58:09, perezju wrote: > On 2017/04/18 11:17:30, nednguyen wrote: > > On 2017/04/18 05:46:28, vovoy wrote: > > > it's the second part of Benchmark Tab Switching Refinement > > > > > > design doc: > > > > > > https://docs.google.com/document/d/1rMrrlW4-5ZGD9dHxUYBOzzkS-vyX0_mtiJ-CMT2eg... > > > > How long does it take to run this with tabset_repeat=<the number you want?> If > > the cycle time is reasonable, I think it would be ok to just hard code that > > number in system_health/multi_tab_stories.py or in the > > tab_switching.TabSwitching benchmark > > Also stepping back a bit, what is the rationale for having this option? tested on some chromebook with intel core m3 and 4G RAM tabset_repeat=1 ( 24 tabs), duration: 88 (sec), TabSwitchPaintDuration: 134 (ms) tabset_repeat=2 ( 48 tabs), duration: 147 (sec), TabSwitchPaintDuration: 146 (ms) tabset_repeat=3 ( 72 tabs), duration: 214 (sec), TabSwitchPaintDuration: 149 (ms) tabset_repeat=4 ( 96 tabs), duration: 290 (sec), TabSwitchPaintDuration: 176 (ms) tabset_repeat=5 (120 tabs), duration: 395 (sec), TabSwitchPaintDuration: 293 (ms) the rationale for this option is to easily compare chrome/chromeos performance on different tab counts. From the above data, the tab switch paint duration increased a lot from repeat 4 to 5. (when will the metric increase may depend on system configuration and total ram size) I can use these information to investigate how to improve chromeos tab discarding logic. for more info, also check the design doc https://docs.google.com/document/d/1rMrrlW4-5ZGD9dHxUYBOzzkS-vyX0_mtiJ-CMT2eg...
On 2017/04/19 09:49:41, vovoy wrote: > On 2017/04/18 12:58:09, perezju wrote: > > On 2017/04/18 11:17:30, nednguyen wrote: > > > On 2017/04/18 05:46:28, vovoy wrote: > > > > it's the second part of Benchmark Tab Switching Refinement > > > > > > > > design doc: > > > > > > > > > > https://docs.google.com/document/d/1rMrrlW4-5ZGD9dHxUYBOzzkS-vyX0_mtiJ-CMT2eg... > > > > > > How long does it take to run this with tabset_repeat=<the number you want?> > If > > > the cycle time is reasonable, I think it would be ok to just hard code that > > > number in system_health/multi_tab_stories.py or in the > > > tab_switching.TabSwitching benchmark > > > > Also stepping back a bit, what is the rationale for having this option? > > tested on some chromebook with intel core m3 and 4G RAM > tabset_repeat=1 ( 24 tabs), duration: 88 (sec), TabSwitchPaintDuration: 134 > (ms) > tabset_repeat=2 ( 48 tabs), duration: 147 (sec), TabSwitchPaintDuration: 146 > (ms) > tabset_repeat=3 ( 72 tabs), duration: 214 (sec), TabSwitchPaintDuration: 149 > (ms) > tabset_repeat=4 ( 96 tabs), duration: 290 (sec), TabSwitchPaintDuration: 176 > (ms) > tabset_repeat=5 (120 tabs), duration: 395 (sec), TabSwitchPaintDuration: 293 > (ms) > > the rationale for this option is to easily compare chrome/chromeos performance > on different tab counts. > From the above data, the tab switch paint duration increased a lot from repeat 4 > to 5. > (when will the metric increase may depend on system configuration and total ram > size) > I can use these information to investigate how to improve chromeos tab > discarding logic. > > for more info, also check the design doc > https://docs.google.com/document/d/1rMrrlW4-5ZGD9dHxUYBOzzkS-vyX0_mtiJ-CMT2eg... I think this provides nice flexibility to stress testing tab switching without too much code complication. What do you think Juan? lgtm but please wait for Juan.
lgtm w/nits and a comment https://codereview.chromium.org/2819423002/diff/1/tools/perf/benchmarks/tab_s... File tools/perf/benchmarks/tab_switching.py (right): https://codereview.chromium.org/2819423002/diff/1/tools/perf/benchmarks/tab_s... tools/perf/benchmarks/tab_switching.py:42: return story_set Hmm, this doesn't make me supper happy. But I guess we don't have a cleaner way to get the options to the stories. Ned, maybe something to think about if we would like to provide such an interface? Anyway, I'm OK with landing this change for now. https://codereview.chromium.org/2819423002/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/multi_tab_stories.py (right): https://codereview.chromium.org/2819423002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/multi_tab_stories.py:16: ABSTRACT_STORY = True nit: add a blank like in between https://codereview.chromium.org/2819423002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/multi_tab_stories.py:17: def __init__(self, story_set, take_memory_measurement, tabset_repeat = 1): nit: spacing, should be tabset_repeat=1
lgtm w/nits and a comment https://codereview.chromium.org/2819423002/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/multi_tab_stories.py (right): https://codereview.chromium.org/2819423002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/multi_tab_stories.py:16: ABSTRACT_STORY = True nit: add a blank like in between https://codereview.chromium.org/2819423002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/multi_tab_stories.py:17: def __init__(self, story_set, take_memory_measurement, tabset_repeat = 1): nit: spacing, should be tabset_repeat=1
https://codereview.chromium.org/2819423002/diff/1/tools/perf/benchmarks/tab_s... File tools/perf/benchmarks/tab_switching.py (right): https://codereview.chromium.org/2819423002/diff/1/tools/perf/benchmarks/tab_s... tools/perf/benchmarks/tab_switching.py:42: return story_set On 2017/04/21 11:59:48, perezju wrote: > Hmm, this doesn't make me supper happy. But I guess we don't have a cleaner way > to get the options to the stories. > > Ned, maybe something to think about if we would like to provide such an > interface? > > Anyway, I'm OK with landing this change for now. I think in general, we should not allow people parameterizing system health story, so this case is more like an exception.
On 2017/04/21 12:14:03, nednguyen wrote: > https://codereview.chromium.org/2819423002/diff/1/tools/perf/benchmarks/tab_s... > File tools/perf/benchmarks/tab_switching.py (right): > > https://codereview.chromium.org/2819423002/diff/1/tools/perf/benchmarks/tab_s... > tools/perf/benchmarks/tab_switching.py:42: return story_set > On 2017/04/21 11:59:48, perezju wrote: > > Hmm, this doesn't make me supper happy. But I guess we don't have a cleaner > way > > to get the options to the stories. > > > > Ned, maybe something to think about if we would like to provide such an > > interface? > > > > Anyway, I'm OK with landing this change for now. > > I think in general, we should not allow people parameterizing system health > story, so this case is more like an exception. Ah actually, not lgtm yet because of the below: One thing I found out when tinkering with the tab_switching benchmark locally is that you can force it to stress testing the number of tabs by: 1) Overriding ShouldTearDownStateAfterEachStoryRun & ShouldTearDownStateAfterEachStorySetRun of the benchmark to both return True. 2) Run the benchmark with "--pageset-repeat=x" If you try the two things above & find that it allows you to tweak the number of tabs creation to stress memory usage, I think that would be a more preferable way.
On 2017/04/21 14:23:04, nednguyen wrote: > On 2017/04/21 12:14:03, nednguyen wrote: > > > https://codereview.chromium.org/2819423002/diff/1/tools/perf/benchmarks/tab_s... > > File tools/perf/benchmarks/tab_switching.py (right): > > > > > https://codereview.chromium.org/2819423002/diff/1/tools/perf/benchmarks/tab_s... > > tools/perf/benchmarks/tab_switching.py:42: return story_set > > On 2017/04/21 11:59:48, perezju wrote: > > > Hmm, this doesn't make me supper happy. But I guess we don't have a cleaner > > way > > > to get the options to the stories. > > > > > > Ned, maybe something to think about if we would like to provide such an > > > interface? > > > > > > Anyway, I'm OK with landing this change for now. > > > > I think in general, we should not allow people parameterizing system health > > story, so this case is more like an exception. > > Ah actually, not lgtm yet because of the below: > > One thing I found out when tinkering with the tab_switching benchmark locally is > that you can force it to stress testing the number of tabs by: > 1) Overriding ShouldTearDownStateAfterEachStoryRun & > ShouldTearDownStateAfterEachStorySetRun of the benchmark to both return True. > 2) Run the benchmark with "--pageset-repeat=x" > > If you try the two things above & find that it allows you to tweak the number of > tabs creation to stress memory usage, I think that would be a more preferable > way. I tried to include the following in class TabSwitchingTypical25: @classmethod def ShouldTearDownStateAfterEachStoryRun(cls): return False @classmethod def ShouldTearDownStateAfterEachStorySetRun(cls): return False and runs: src/tools/perf/run_benchmark tab_switching.typical_25 --browser=reference --device=desktop --also-run-disabled-tests --pageset-repeat=2 but the result is unexpected: 1) create tab 1, 2, ..., 24 2) tab switching to 1, 2, ..., 24 3) tab 24, 23, 22, ..., 2 are closed <--------- unexpected 4) create tab 25, 26, ..., 48 5) tab switching to 25, 26, ..., 48 does the issue in step 3) happened in your setup?
On 2017/04/21 16:04:51, vovoy wrote: > On 2017/04/21 14:23:04, nednguyen wrote: > > On 2017/04/21 12:14:03, nednguyen wrote: > > > > > > https://codereview.chromium.org/2819423002/diff/1/tools/perf/benchmarks/tab_s... > > > File tools/perf/benchmarks/tab_switching.py (right): > > > > > > > > > https://codereview.chromium.org/2819423002/diff/1/tools/perf/benchmarks/tab_s... > > > tools/perf/benchmarks/tab_switching.py:42: return story_set > > > On 2017/04/21 11:59:48, perezju wrote: > > > > Hmm, this doesn't make me supper happy. But I guess we don't have a > cleaner > > > way > > > > to get the options to the stories. > > > > > > > > Ned, maybe something to think about if we would like to provide such an > > > > interface? > > > > > > > > Anyway, I'm OK with landing this change for now. > > > > > > I think in general, we should not allow people parameterizing system health > > > story, so this case is more like an exception. > > > > Ah actually, not lgtm yet because of the below: > > > > One thing I found out when tinkering with the tab_switching benchmark locally > is > > that you can force it to stress testing the number of tabs by: > > 1) Overriding ShouldTearDownStateAfterEachStoryRun & > > ShouldTearDownStateAfterEachStorySetRun of the benchmark to both return True. > > 2) Run the benchmark with "--pageset-repeat=x" > > > > If you try the two things above & find that it allows you to tweak the number > of > > tabs creation to stress memory usage, I think that would be a more preferable > > way. > > I tried to include the following in class TabSwitchingTypical25: > @classmethod > def ShouldTearDownStateAfterEachStoryRun(cls): > return False > @classmethod > def ShouldTearDownStateAfterEachStorySetRun(cls): > return False > > and runs: > src/tools/perf/run_benchmark tab_switching.typical_25 --browser=reference > --device=desktop --also-run-disabled-tests --pageset-repeat=2 > > but the result is unexpected: > 1) create tab 1, 2, ..., 24 > 2) tab switching to 1, 2, ..., 24 > 3) tab 24, 23, 22, ..., 2 are closed <--------- unexpected > 4) create tab 25, 26, ..., 48 > 5) tab switching to 25, 26, ..., 48 > > does the issue in step 3) happened in your setup? Yes it does. You maybe able to fix that by simply modifying https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...
On 2017/04/22 05:53:13, nednguyen wrote: > On 2017/04/21 16:04:51, vovoy wrote: > > On 2017/04/21 14:23:04, nednguyen wrote: > > > On 2017/04/21 12:14:03, nednguyen wrote: > > > > > > > > > > https://codereview.chromium.org/2819423002/diff/1/tools/perf/benchmarks/tab_s... > > > > File tools/perf/benchmarks/tab_switching.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2819423002/diff/1/tools/perf/benchmarks/tab_s... > > > > tools/perf/benchmarks/tab_switching.py:42: return story_set > > > > On 2017/04/21 11:59:48, perezju wrote: > > > > > Hmm, this doesn't make me supper happy. But I guess we don't have a > > cleaner > > > > way > > > > > to get the options to the stories. > > > > > > > > > > Ned, maybe something to think about if we would like to provide such an > > > > > interface? > > > > > > > > > > Anyway, I'm OK with landing this change for now. > > > > > > > > I think in general, we should not allow people parameterizing system > health > > > > story, so this case is more like an exception. > > > > > > Ah actually, not lgtm yet because of the below: > > > > > > One thing I found out when tinkering with the tab_switching benchmark > locally > > is > > > that you can force it to stress testing the number of tabs by: > > > 1) Overriding ShouldTearDownStateAfterEachStoryRun & > > > ShouldTearDownStateAfterEachStorySetRun of the benchmark to both return > True. > > > 2) Run the benchmark with "--pageset-repeat=x" > > > > > > If you try the two things above & find that it allows you to tweak the > number > > of > > > tabs creation to stress memory usage, I think that would be a more > preferable > > > way. > > > > I tried to include the following in class TabSwitchingTypical25: > > @classmethod > > def ShouldTearDownStateAfterEachStoryRun(cls): > > return False > > @classmethod > > def ShouldTearDownStateAfterEachStorySetRun(cls): > > return False > > > > and runs: > > src/tools/perf/run_benchmark tab_switching.typical_25 --browser=reference > > --device=desktop --also-run-disabled-tests --pageset-repeat=2 > > > > but the result is unexpected: > > 1) create tab 1, 2, ..., 24 > > 2) tab switching to 1, 2, ..., 24 > > 3) tab 24, 23, 22, ..., 2 are closed <--------- unexpected > > 4) create tab 25, 26, ..., 48 > > 5) tab switching to 25, 26, ..., 48 > > > > does the issue in step 3) happened in your setup? > > Yes it does. You maybe able to fix that by simply modifying > https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... I think it's OK to abandon --tabset-repeat. I created another codereview, ptal: Fix tab_switching to support flag --pageset-repeat properly https://codereview.chromium.org/2829423002/ --pageset-repeat=2 is a little more stressful than --tabset-repeat=2. --tabset-repeat=2: 1) create 1-48 tabs 2) tab switching to 1-48 tabs --pageset-repeat=2: a) create 1-24 tabs b) tab switching to 1-24 tabs c) create 25-48 tabs d) tab switching to 1-48 tabs
lgtm After discussing on the other CL, we agree that using "is_multi_tab_test = True" is the worse approach.
The CQ bit was checked by vovoy@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: 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 vovoy@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/2819423002/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/multi_tab_stories.py (right): https://codereview.chromium.org/2819423002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/multi_tab_stories.py:16: ABSTRACT_STORY = True On 2017/04/21 11:59:48, perezju wrote: > nit: add a blank like in between Done. https://codereview.chromium.org/2819423002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/multi_tab_stories.py:17: def __init__(self, story_set, take_memory_measurement, tabset_repeat = 1): On 2017/04/21 11:59:48, perezju wrote: > nit: spacing, should be tabset_repeat=1 Done.
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 vovoy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org, nednguyen@google.com Link to the patchset: https://codereview.chromium.org/2819423002/#ps20001 (title: "fixing style")
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": 20001, "attempt_start_ts": 1493082547230140, "parent_rev": "92072b3d9f87ffe78a4e977dd85db597452caa92", "commit_rev": "ef1307664210fb58e9b9fb85586704ca1d5e59bc"}
Message was sent while issue was closed.
Description was changed from ========== Support flag --tabset-repeat on benchmark tab_switching flag --tabset-repeat multiply the tab count of benchmark tab_switching to measure metrics on different tab count. ex: add flag --tabset-repeat=2 to double the tab count. BUG=689388 ========== to ========== Support flag --tabset-repeat on benchmark tab_switching flag --tabset-repeat multiply the tab count of benchmark tab_switching to measure metrics on different tab count. ex: add flag --tabset-repeat=2 to double the tab count. BUG=689388 Review-Url: https://codereview.chromium.org/2819423002 Cr-Commit-Position: refs/heads/master@{#466852} Committed: https://chromium.googlesource.com/chromium/src/+/ef1307664210fb58e9b9fb855867... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/ef1307664210fb58e9b9fb855867... |