|
|
DescriptionAdd Multi-tab System Health Story
A single story to create multiple tabs and switching between the tabs.
BUG=689388
Review-Url: https://codereview.chromium.org/2706483003
Cr-Commit-Position: refs/heads/master@{#454753}
Committed: https://chromium.googlesource.com/chromium/src/+/cd6f4e4cc9866713ba8a2a121a6116310b1a9b9e
Patch Set 1 #Patch Set 2 : Refactoring Benchmark TabSwitching #Patch Set 3 : Refactoring Benchmark TabSwitching #
Total comments: 6
Patch Set 4 : Add Multi-tab System Health Stories #Patch Set 5 : Add Multi-tab System Health Stories #
Total comments: 22
Patch Set 6 : Add Multi-tab System Health Stories #Patch Set 7 : Add Multi-tab System Health Stories #Patch Set 8 : Add Multi-tab System Health Stories #
Total comments: 2
Patch Set 9 : Re-record Multi-tab System Health Story #
Messages
Total messages: 74 (55 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...
There were warnings when CQ was processing your CL: * CQ is not running the chromium_presubmit trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified * CQ is not running the mac_chromium_rel_ng trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the chromium_presubmit trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified * CQ is not running the mac_chromium_rel_ng trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the chromium_presubmit trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified * CQ is not running the mac_chromium_rel_ng trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_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...
There were warnings when CQ was processing your CL: * CQ is not running the chromium_presubmit trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified * CQ is not running the mac_chromium_rel_ng trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the chromium_presubmit trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified * CQ is not running the mac_chromium_rel_ng trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the chromium_presubmit trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified * CQ is not running the mac_chromium_rel_ng trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
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 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...
There were warnings when CQ was processing your CL: * CQ is not running the chromium_presubmit trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified * CQ is not running the mac_chromium_rel_ng trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the chromium_presubmit trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified * CQ is not running the mac_chromium_rel_ng trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
Description was changed from ========== Rewrite Benchmark TabSwitching Rewrite TabSwitching Benchmark, use a single story to execute all the tab switching action. BUG=689388 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:chromium_presubmit;master.tryserver.chromium.mac:mac_chromium_rel_ng ========== to ========== Refactoring Benchmark TabSwitching Refactoring TabSwitching Benchmark, use a single story to execute all the tab switching action. BUG=689388 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:chromium_presubmit;master.tryserver.chromium.mac:mac_chromium_rel_ng ==========
There were warnings when CQ was processing your CL: * CQ is not running the chromium_presubmit trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified * CQ is not running the mac_chromium_rel_ng trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
Description was changed from ========== Refactoring Benchmark TabSwitching Refactoring TabSwitching Benchmark, use a single story to execute all the tab switching action. BUG=689388 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:chromium_presubmit;master.tryserver.chromium.mac:mac_chromium_rel_ng ========== to ========== Refactoring Benchmark TabSwitching Refactoring TabSwitching Benchmark, use a single story to execute all the tab switching action. BUG=689388 ==========
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 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 ========== Refactoring Benchmark TabSwitching Refactoring TabSwitching Benchmark, use a single story to execute all the tab switching action. BUG=689388 ========== to ========== Add Multi-tab System Health Stories Add System Health Stories to test multi tab scenario. Refactoring TabSwitching Benchmark using the multi-tab system health stories. BUG=689388 ==========
Description was changed from ========== Add Multi-tab System Health Stories Add System Health Stories to test multi tab scenario. Refactoring TabSwitching Benchmark using the multi-tab system health stories. BUG=689388 ========== to ========== Add Multi-tab System Health Stories Add System Health Stories to test multi tab scenario. Refactoring TabSwitching Benchmark using the multi-tab system health stories. BUG=689388 ==========
design doc: https://docs.google.com/a/google.com/document/d/1rMrrlW4-5ZGD9dHxUYBOzzkS-vyX... this cl is the first step to refine benchmark tab_switching
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_...)
On 2017/03/01 08:50:06, vovoy wrote: > design doc: > https://docs.google.com/a/google.com/document/d/1rMrrlW4-5ZGD9dHxUYBOzzkS-vyX... > this cl is the first step to refine benchmark tab_switching Can you split this CL into two: one to add tools/perf/page_sets/system_health/multi_tab_stories.py & another to modify existing tab_switching.py to use the multi_tab_stories? I would say that you don't need the three multitab stories. One story should be enough for you. Also please do not rely on the TabSwitchHistogram in the multi_tab_stories. Instead, you can wait for the tab to switch by evaluating the content on the page. e.g: Tab Switch to tab of www.fifa.com wait for the page's url to be www.fifa.com
I'll hold off on further review until you do the split as Ned suggests. https://codereview.chromium.org/2706483003/diff/40001/tools/perf/measurements... File tools/perf/measurements/tab_switching.py (right): https://codereview.chromium.org/2706483003/diff/40001/tools/perf/measurements... tools/perf/measurements/tab_switching.py:23: # TODO: Revisit this test once multitab support is finalized. This is what you're doing now, right? So it might make sense to remove this TODO. https://codereview.chromium.org/2706483003/diff/40001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/multi_tab_stories.py (right): https://codereview.chromium.org/2706483003/diff/40001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/multi_tab_stories.py:31: # pylint: disable=W0640 nit: please write out the name of the rule you're disabling rather than using the code. https://codereview.chromium.org/2706483003/diff/40001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/multi_tab_stories.py:49: # create tabs nit: this comment seems unnecessary because it repeats what the code below does, see go/tott-465.
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...
On 2017/03/01 16:15:32, nednguyen wrote: > On 2017/03/01 08:50:06, vovoy wrote: > > design doc: > > > https://docs.google.com/a/google.com/document/d/1rMrrlW4-5ZGD9dHxUYBOzzkS-vyX... > > this cl is the first step to refine benchmark tab_switching > > Can you split this CL into two: one to add > tools/perf/page_sets/system_health/multi_tab_stories.py & another to modify > existing tab_switching.py to use the multi_tab_stories? I would say that you > don't need the three multitab stories. One story should be enough for you. > > Also please do not rely on the TabSwitchHistogram in the multi_tab_stories. > Instead, you can wait for the tab to switch by evaluating the content on the > page. > e.g: > Tab Switch to tab of http://www.fifa.com > wait for the page's url to be http://www.fifa.com CL split, this CL contains only the multi-tab system health story, will add another CL for tab_switching refactoring. using WaitForFrameToBeDisplayed() instead of TabSwitchHistogram.
https://codereview.chromium.org/2706483003/diff/40001/tools/perf/measurements... File tools/perf/measurements/tab_switching.py (right): https://codereview.chromium.org/2706483003/diff/40001/tools/perf/measurements... tools/perf/measurements/tab_switching.py:23: # TODO: Revisit this test once multitab support is finalized. On 2017/03/01 16:52:13, eakuefner wrote: > This is what you're doing now, right? So it might make sense to remove this > TODO. Done. https://codereview.chromium.org/2706483003/diff/40001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/multi_tab_stories.py (right): https://codereview.chromium.org/2706483003/diff/40001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/multi_tab_stories.py:31: # pylint: disable=W0640 On 2017/03/01 16:52:13, eakuefner wrote: > nit: please write out the name of the rule you're disabling rather than using > the code. Removed this function in the next patch. https://codereview.chromium.org/2706483003/diff/40001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/multi_tab_stories.py:49: # create tabs On 2017/03/01 16:52:13, eakuefner wrote: > nit: this comment seems unnecessary because it repeats what the code below does, > see go/tott-465. Done.
Description was changed from ========== Add Multi-tab System Health Stories Add System Health Stories to test multi tab scenario. Refactoring TabSwitching Benchmark using the multi-tab system health stories. BUG=689388 ========== to ========== Add Multi-tab System Health Stories A single story to create multiple tabs and switching between the tabs. BUG=689388 ==========
The CQ bit was checked by nednguyen@google.com 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...
nednguyen@google.com changed reviewers: + perezju@chromium.org
https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/da... File tools/perf/page_sets/data/system_health_desktop.json (right): https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/da... tools/perf/page_sets/data/system_health_desktop.json:175: "DEFAULT": "system_health_desktop_048.wpr" You need to also upload system_health_desktop_048.wpr.sha1 file to cloud storage. Command: https://www.chromium.org/developers/telemetry/upload_to_cloud_storage#TOC-Upl... Just want to make sure, did you get the PRESUBMIT check that warn about there is no entry for system_health_desktop_048.wpr?
https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/multi_tab_stories.py (right): https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/multi_tab_stories.py:39: NAME = 'multitab:typical24:typical24' can you add: TAGS = [story_tags.TABS_SWITCHING]?
https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/da... File tools/perf/page_sets/data/system_health_desktop.json (right): https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/da... tools/perf/page_sets/data/system_health_desktop.json:175: "DEFAULT": "system_health_desktop_048.wpr" On 2017/03/02 13:24:39, nednguyen wrote: > You need to also upload system_health_desktop_048.wpr.sha1 file to cloud > storage. > > Command: > https://www.chromium.org/developers/telemetry/upload_to_cloud_storage#TOC-Upl... > > Just want to make sure, did you get the PRESUBMIT check that warn about there is > no entry for system_health_desktop_048.wpr? Err, my bad. Upload system_health_desktop_048.wpr to cloud storage & git add system_health_desktop_048.wpr.sha1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
High level questions/concerns: - How long does the story take to run? - How big are the traces produced? https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/multi_tab_stories.py (right): https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/multi_tab_stories.py:18: tabs = action_runner.tab.browser.tabs High level question, do we really want to 1) open all tabs at the same time 2) wait for each page to be ready Or would it be better: A) for each tab: B) open the tab, wait for its page to be ready ?? https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/multi_tab_stories.py:20: for i in range(1, len(self.URL_LIST)): for url in self.URL_LIST[1:]: ... Also add a small comment explaining why the first url is skipped. https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/multi_tab_stories.py:24: for i in range(len(self.URL_LIST)): A possibility here could be: for i, url in enumerate(self.URL_LIST): ... https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/multi_tab_stories.py:26: py_utils.WaitFor(tabs[i].HasReachedQuiescence, 15) tabs[i].action_runner.WaitForNetworkQuiescence(15) https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/multi_tab_stories.py:33: for i in range(len(tabs)): for tab in action_runner.tab.browser.tabs: ... https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/multi_tab_stories.py:39: NAME = 'multitab:typical24:typical24' nit: I think 'multitab:misc:typical24' might be a better name. /bike-shedding https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/multi_tab_stories.py:40: URL_LIST = [ Is this list coming from somewhere else? Could we import it from there rather than duplicate?
> High level questions/concerns: > - How long does the story take to run? I will update the story run time. > - How big are the traces produced? How to check the trace size? https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/da... File tools/perf/page_sets/data/system_health_desktop.json (right): https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/da... tools/perf/page_sets/data/system_health_desktop.json:175: "DEFAULT": "system_health_desktop_048.wpr" On 2017/03/02 13:30:15, nednguyen wrote: > On 2017/03/02 13:24:39, nednguyen wrote: > > You need to also upload system_health_desktop_048.wpr.sha1 file to cloud > > storage. > > > > Command: > > > https://www.chromium.org/developers/telemetry/upload_to_cloud_storage#TOC-Upl... > > > > Just want to make sure, did you get the PRESUBMIT check that warn about there > is > > no entry for system_health_desktop_048.wpr? > > Err, my bad. Upload system_health_desktop_048.wpr to cloud storage & git add > system_health_desktop_048.wpr.sha1 OK, I will upload the wpr and git add wpr.sha1 for review. https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/multi_tab_stories.py (right): https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/multi_tab_stories.py:18: tabs = action_runner.tab.browser.tabs On 2017/03/02 14:33:12, perezju wrote: > High level question, do we really want to > > 1) open all tabs at the same time > 2) wait for each page to be ready > > Or would it be better: > > A) for each tab: > B) open the tab, wait for its page to be ready > > ?? I open all tabs and then wait for each tabs because I think it's fast to load each tab's content simultaneously. while it wait for tab 1 to be ready, others tabs are also loading. I can do some experiment to compare the total time for the 1)2) or A)B) https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/multi_tab_stories.py:20: for i in range(1, len(self.URL_LIST)): On 2017/03/02 14:33:11, perezju wrote: > for url in self.URL_LIST[1:]: > ... > > > Also add a small comment explaining why the first url is skipped. OK, will add comment https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/multi_tab_stories.py:39: NAME = 'multitab:typical24:typical24' On 2017/03/02 13:26:00, nednguyen wrote: > can you add: > TAGS = [story_tags.TABS_SWITCHING]? OK https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/multi_tab_stories.py:40: URL_LIST = [ On 2017/03/02 14:33:11, perezju wrote: > Is this list coming from somewhere else? Could we import it from there rather > than duplicate? the list is coming from page_sets/typical_25.py maybe I could modify typical_25.py to export the list
On 2017/03/02 18:02:29, vovoy wrote: > > High level questions/concerns: > > - How long does the story take to run? > I will update the story run time. > > - How big are the traces produced? > How to check the trace size? You can run the benchmark with: ./tools/perf/run_benchmark --browser=<...> system_health.common_desktop --story-filter=multitab -v The log in https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... should show you the size of trace > > https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/da... > File tools/perf/page_sets/data/system_health_desktop.json (right): > > https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/da... > tools/perf/page_sets/data/system_health_desktop.json:175: "DEFAULT": > "system_health_desktop_048.wpr" > On 2017/03/02 13:30:15, nednguyen wrote: > > On 2017/03/02 13:24:39, nednguyen wrote: > > > You need to also upload system_health_desktop_048.wpr.sha1 file to cloud > > > storage. > > > > > > Command: > > > > > > https://www.chromium.org/developers/telemetry/upload_to_cloud_storage#TOC-Upl... > > > > > > Just want to make sure, did you get the PRESUBMIT check that warn about > there > > is > > > no entry for system_health_desktop_048.wpr? > > > > Err, my bad. Upload system_health_desktop_048.wpr to cloud storage & git add > > system_health_desktop_048.wpr.sha1 > > OK, I will upload the wpr and git add wpr.sha1 for review. > > https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... > File tools/perf/page_sets/system_health/multi_tab_stories.py (right): > > https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... > tools/perf/page_sets/system_health/multi_tab_stories.py:18: tabs = > action_runner.tab.browser.tabs > On 2017/03/02 14:33:12, perezju wrote: > > High level question, do we really want to > > > > 1) open all tabs at the same time > > 2) wait for each page to be ready > > > > Or would it be better: > > > > A) for each tab: > > B) open the tab, wait for its page to be ready > > > > ?? > > I open all tabs and then wait for each tabs because > I think it's fast to load each tab's content simultaneously. > while it wait for tab 1 to be ready, others tabs are also loading. > > I can do some experiment to compare the total time for the 1)2) or A)B) > > https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... > tools/perf/page_sets/system_health/multi_tab_stories.py:20: for i in range(1, > len(self.URL_LIST)): > On 2017/03/02 14:33:11, perezju wrote: > > for url in self.URL_LIST[1:]: > > ... > > > > > > Also add a small comment explaining why the first url is skipped. > > OK, will add comment > > https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... > tools/perf/page_sets/system_health/multi_tab_stories.py:39: NAME = > 'multitab:typical24:typical24' > On 2017/03/02 13:26:00, nednguyen wrote: > > can you add: > > TAGS = [story_tags.TABS_SWITCHING]? > > OK > > https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... > tools/perf/page_sets/system_health/multi_tab_stories.py:40: URL_LIST = [ > On 2017/03/02 14:33:11, perezju wrote: > > Is this list coming from somewhere else? Could we import it from there rather > > than duplicate? > > the list is coming from page_sets/typical_25.py > maybe I could modify typical_25.py to export the list
https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/multi_tab_stories.py (right): https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/multi_tab_stories.py:40: URL_LIST = [ On 2017/03/02 18:02:29, vovoy wrote: > On 2017/03/02 14:33:11, perezju wrote: > > Is this list coming from somewhere else? Could we import it from there rather > > than duplicate? > > the list is coming from page_sets/typical_25.py > maybe I could modify typical_25.py to export the list I think there is no need to share code here. As long as the list of sites here seems reasonable, we're good to go.
Description was changed from ========== Add Multi-tab System Health Stories A single story to create multiple tabs and switching between the tabs. BUG=689388 ========== to ========== Add Multi-tab System Health Story A single story to create multiple tabs and switching between the tabs. BUG=689388 ==========
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: This issue passed the CQ dry run.
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...
> High level questions/concerns: > - How long does the story take to run? multitab:misc:typical24 takes 52 secs > - How big are the traces produced? command: tools/perf/run_benchmark --browser=reference system_health.common_desktop --story-filter=multitab -v shows: INFO:root:Trace sizes in bytes: {'traceEvents': 158059621, 'telemetry': 264370, 'tabIds': 960} https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/multi_tab_stories.py (right): https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/multi_tab_stories.py:18: tabs = action_runner.tab.browser.tabs On 2017/03/02 18:02:29, vovoy wrote: > On 2017/03/02 14:33:12, perezju wrote: > > High level question, do we really want to > > > > 1) open all tabs at the same time > > 2) wait for each page to be ready > > > > Or would it be better: > > > > A) for each tab: > > B) open the tab, wait for its page to be ready > > > > ?? > > I open all tabs and then wait for each tabs because > I think it's fast to load each tab's content simultaneously. > while it wait for tab 1 to be ready, others tabs are also loading. > > I can do some experiment to compare the total time for the 1)2) or A)B) test command: tools/perf/run_benchmark system_health.memory_desktop --browser=reference --device=desktop --story-filter=multitab:typical24:typical24 --also-run-disabled-tests 1)2) takes 52 +- 1 (secs) A)B) takes 119 +- 2 (secs) I will keep parallel loading 1)2) unless there are other reasons for serial loading A)B) https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/multi_tab_stories.py:24: for i in range(len(self.URL_LIST)): On 2017/03/02 14:33:11, perezju wrote: > A possibility here could be: > > for i, url in enumerate(self.URL_LIST): > ... Done. https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/multi_tab_stories.py:26: py_utils.WaitFor(tabs[i].HasReachedQuiescence, 15) On 2017/03/02 14:33:12, perezju wrote: > tabs[i].action_runner.WaitForNetworkQuiescence(15) Done. https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/multi_tab_stories.py:33: for i in range(len(tabs)): On 2017/03/02 14:33:12, perezju wrote: > for tab in action_runner.tab.browser.tabs: > ... Done. https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/multi_tab_stories.py:39: NAME = 'multitab:typical24:typical24' On 2017/03/02 14:33:12, perezju wrote: > nit: I think 'multitab:misc:typical24' might be a better name. /bike-shedding Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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...
lgtm On 2017/03/03 08:30:12, vovoy wrote: > > High level questions/concerns: > > - How long does the story take to run? > multitab:misc:typical24 takes 52 secs > > - How big are the traces produced? > command: > tools/perf/run_benchmark --browser=reference system_health.common_desktop > --story-filter=multitab -v > shows: > INFO:root:Trace sizes in bytes: {'traceEvents': 158059621, 'telemetry': 264370, > 'tabIds': 960} Awesome, this is good. Thanks for checking. Nice story! https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/multi_tab_stories.py (right): https://codereview.chromium.org/2706483003/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/multi_tab_stories.py:18: tabs = action_runner.tab.browser.tabs On 2017/03/03 08:30:11, vovoy wrote: > On 2017/03/02 18:02:29, vovoy wrote: > > On 2017/03/02 14:33:12, perezju wrote: > > > High level question, do we really want to > > > > > > 1) open all tabs at the same time > > > 2) wait for each page to be ready > > > > > > Or would it be better: > > > > > > A) for each tab: > > > B) open the tab, wait for its page to be ready > > > > > > ?? > > > > I open all tabs and then wait for each tabs because > > I think it's fast to load each tab's content simultaneously. > > while it wait for tab 1 to be ready, others tabs are also loading. > > > > I can do some experiment to compare the total time for the 1)2) or A)B) > > test command: > tools/perf/run_benchmark system_health.memory_desktop --browser=reference > --device=desktop --story-filter=multitab:typical24:typical24 > --also-run-disabled-tests > > 1)2) takes 52 +- 1 (secs) > A)B) takes 119 +- 2 (secs) > > I will keep parallel loading 1)2) unless there are other reasons for serial > loading A)B) Sounds good. Thanks for checking!
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
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": 140001, "attempt_start_ts": 1488596622026240, "parent_rev": "0f9bfb00c432d594504502728b8a1405a0ff2cf1", "commit_rev": "cd6f4e4cc9866713ba8a2a121a6116310b1a9b9e"}
The CQ bit was unchecked by nednguyen@google.com
Message was sent while issue was closed.
Description was changed from ========== Add Multi-tab System Health Story A single story to create multiple tabs and switching between the tabs. BUG=689388 ========== to ========== Add Multi-tab System Health Story A single story to create multiple tabs and switching between the tabs. BUG=689388 Review-Url: https://codereview.chromium.org/2706483003 Cr-Commit-Position: refs/heads/master@{#454753} Committed: https://chromium.googlesource.com/chromium/src/+/cd6f4e4cc9866713ba8a2a121a61... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/cd6f4e4cc9866713ba8a2a121a61...
Message was sent while issue was closed.
https://codereview.chromium.org/2706483003/diff/140001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/multi_tab_stories.py (right): https://codereview.chromium.org/2706483003/diff/140001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/multi_tab_stories.py:67: 'http://premierleague.com', I notice that https://www.premierleague.com/, https://www.html5rocks.com/en/ cause a "404 Not Found" error. Feel free to remove ones that have 404 error or rerecord the story (I suggest hack in some wait time when you record it to make sure WPR is able to capture the network).
Message was sent while issue was closed.
https://codereview.chromium.org/2706483003/diff/140001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/multi_tab_stories.py (right): https://codereview.chromium.org/2706483003/diff/140001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/multi_tab_stories.py:67: 'http://premierleague.com', On 2017/03/04 03:13:06, nednguyen wrote: > I notice that https://www.premierleague.com/, https://www.html5rocks.com/en/ > cause a "404 Not Found" error. > > Feel free to remove ones that have 404 error or rerecord the story (I suggest > hack in some wait time when you record it to make sure WPR is able to capture > the network). OK, I will check and fix the recording. |