Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2890333002:
Tab Switching Benchmark for ChromeOS (Closed)
DescriptionTab Switching Benchmark for ChromeOS
Copy the tab switching benchmark to cros_benchmarks. Using key events for
simulate tab switching behavior to handle the case that some tab contexts
may be discarded.
Example usage to open 120 tabs:
$ ./run_benchmark --browser=cros-chrome --remote=DUT_IP
cros_tab_switching.typical_24 --tabset-repeat=5
BUG=724381
Review-Url: https://codereview.chromium.org/2890333002
Cr-Commit-Position: refs/heads/master@{#474975}
Committed: https://chromium.googlesource.com/chromium/src/+/2675d3c72d3ad2257e6efb8775b1c54266ccfb7e
Patch Set 1 #Patch Set 2 : Tab Switching Benchmark for ChromeOS #
Total comments: 15
Patch Set 3 : Wait tab switching completion by checking histogram instead of sleep 2 seconds #Patch Set 4 : Tab Switching Benchmark for ChromeOS #
Total comments: 2
Patch Set 5 : Using emulated keyboard to send key events. #
Total comments: 2
Patch Set 6 : Add more checks for keyboard emulator #Patch Set 7 : Minor fixes. #
Total comments: 37
Patch Set 8 : Tab Switching Benchmark for ChromeOS #Patch Set 9 : Style fixes #
Total comments: 8
Patch Set 10 : Rebase #Patch Set 11 : Style fixes #Patch Set 12 : Style fixes #
Total comments: 10
Patch Set 13 : Minor fixes #Patch Set 14 : Fix: Raise exception in CreateStorySet() will fail presubmit. #Patch Set 15 : Tab Switching Benchmark for ChromeOS #Messages
Total messages: 81 (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...
Description was changed from ========== Tab Switching Benchmark for ChromeOS Using key events for simulate tab switching behavior to handle the case that some tab contexts may be discarded. BUG=724381 ========== to ========== Tab Switching Benchmark for ChromeOS Using key events for simulate tab switching behavior to handle the case that some tab contexts may be discarded. BUG=724381 ==========
vovoy@chromium.org changed reviewers: + bccheng@chromium.org, deanliao@chromium.org
Description was changed from ========== Tab Switching Benchmark for ChromeOS Using key events for simulate tab switching behavior to handle the case that some tab contexts may be discarded. BUG=724381 ========== to ========== Tab Switching Benchmark for ChromeOS Copy the tab switching benchmark to cros_benchmarks.Using key events for simulate tab switching behavior to handle the case that some tab contexts may be discarded. BUG=724381 ==========
Description was changed from ========== Tab Switching Benchmark for ChromeOS Copy the tab switching benchmark to cros_benchmarks.Using key events for simulate tab switching behavior to handle the case that some tab contexts may be discarded. BUG=724381 ========== to ========== Tab Switching Benchmark for ChromeOS Copy the tab switching benchmark to cros_benchmarks.Using key events for simulate tab switching behavior to handle the case that some tab contexts may be discarded. BUG=724381 ==========
ptal
On 2017/05/19 07:17:22, vovoy wrote: > ptal in progress.
Try to make comments as complete sentences - the first letter should be capitalized, and same for words after periods. https://codereview.chromium.org/2890333002/diff/20001/tools/perf/contrib/cros... File tools/perf/contrib/cros_benchmarks/cros_utils.py (right): https://codereview.chromium.org/2890333002/diff/20001/tools/perf/contrib/cros... tools/perf/contrib/cros_benchmarks/cros_utils.py:9: # find the device support events EV=120013 find if the device supports event "EV=120013" https://codereview.chromium.org/2890333002/diff/20001/tools/perf/contrib/cros... tools/perf/contrib/cros_benchmarks/cros_utils.py:25: cur_dir=os.path.dirname(os.path.abspath(__file__)) space around = https://codereview.chromium.org/2890333002/diff/20001/tools/perf/contrib/cros... File tools/perf/contrib/cros_benchmarks/tab_switching_bench.py (right): https://codereview.chromium.org/2890333002/diff/20001/tools/perf/contrib/cros... tools/perf/contrib/cros_benchmarks/tab_switching_bench.py:1: # Copyright 2013 The Chromium Authors. All rights reserved. Change it to 2017? https://codereview.chromium.org/2890333002/diff/20001/tools/perf/contrib/cros... File tools/perf/contrib/cros_benchmarks/tab_switching_stories.py (right): https://codereview.chromium.org/2890333002/diff/20001/tools/perf/contrib/cros... tools/perf/contrib/cros_benchmarks/tab_switching_stories.py:24: # context would be invalidated. avoid store tab object for later use. Capitalize the sentences in comments. For example: NOTE: When tab count is high, ... some tab -> some tabs avoid store -> Avoid storing tab objects
deanliao@google.com changed reviewers: + deanliao@google.com
https://codereview.chromium.org/2890333002/diff/20001/tools/perf/contrib/cros... File tools/perf/contrib/cros_benchmarks/cros_utils.py (right): https://codereview.chromium.org/2890333002/diff/20001/tools/perf/contrib/cros... tools/perf/contrib/cros_benchmarks/cros_utils.py:9: # find the device support events EV=120013 On 2017/05/19 09:09:14, bccheng wrote: > find if the device supports event "EV=120013" Comment should be English sentence, which means it should have capital first word and period at the end. https://codereview.chromium.org/2890333002/diff/20001/tools/perf/contrib/cros... File tools/perf/contrib/cros_benchmarks/data/OWNERS (right): https://codereview.chromium.org/2890333002/diff/20001/tools/perf/contrib/cros... tools/perf/contrib/cros_benchmarks/data/OWNERS:1: bccheng@chromium.org Why you need another OWNER file here? I thought that it would use the parent folder's OWNER file if it doesn't have one. https://codereview.chromium.org/2890333002/diff/20001/tools/perf/contrib/cros... File tools/perf/contrib/cros_benchmarks/tab_switching_bench.py (right): https://codereview.chromium.org/2890333002/diff/20001/tools/perf/contrib/cros... tools/perf/contrib/cros_benchmarks/tab_switching_bench.py:9: from contrib.cros_benchmarks.tab_switching_measure import TabSwitchingMeasurement Import module, not class. https://google.github.io/styleguide/pyguide.html?showone=Imports#Imports https://codereview.chromium.org/2890333002/diff/20001/tools/perf/contrib/cros... tools/perf/contrib/cros_benchmarks/tab_switching_bench.py:19: """This test records the MPArch.RWH_TabSwitchPaintDuration histogram. Please rewrite the one-line description of the benchmark. "It records what... " should be placed in detail or even in measurement code. Also please rewrite the long description to explain what this benchmark wants to measure and how it can be used.
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...
Waiting to tab switching completion by checking histogram instead of time.sleep(2.0). https://codereview.chromium.org/2890333002/diff/20001/tools/perf/contrib/cros... File tools/perf/contrib/cros_benchmarks/cros_utils.py (right): https://codereview.chromium.org/2890333002/diff/20001/tools/perf/contrib/cros... tools/perf/contrib/cros_benchmarks/cros_utils.py:9: # find the device support events EV=120013 On 2017/05/19 09:49:46, deanliao_goog wrote: > On 2017/05/19 09:09:14, bccheng wrote: > > find if the device supports event "EV=120013" > > Comment should be English sentence, which means it should have capital first > word and period at the end. Done. https://codereview.chromium.org/2890333002/diff/20001/tools/perf/contrib/cros... tools/perf/contrib/cros_benchmarks/cros_utils.py:25: cur_dir=os.path.dirname(os.path.abspath(__file__)) On 2017/05/19 09:09:14, bccheng wrote: > space around = Done. https://codereview.chromium.org/2890333002/diff/20001/tools/perf/contrib/cros... File tools/perf/contrib/cros_benchmarks/data/OWNERS (right): https://codereview.chromium.org/2890333002/diff/20001/tools/perf/contrib/cros... tools/perf/contrib/cros_benchmarks/data/OWNERS:1: bccheng@chromium.org On 2017/05/19 09:49:46, deanliao_goog wrote: > Why you need another OWNER file here? I thought that it would use the parent > folder's OWNER file if it doesn't have one. Removed this file. I add this file because of a presubmit warning that require a OWNER file on every folder. But it seems OK without adding this file now. https://codereview.chromium.org/2890333002/diff/20001/tools/perf/contrib/cros... File tools/perf/contrib/cros_benchmarks/tab_switching_bench.py (right): https://codereview.chromium.org/2890333002/diff/20001/tools/perf/contrib/cros... tools/perf/contrib/cros_benchmarks/tab_switching_bench.py:1: # Copyright 2013 The Chromium Authors. All rights reserved. On 2017/05/19 09:09:14, bccheng wrote: > Change it to 2017? Done. https://codereview.chromium.org/2890333002/diff/20001/tools/perf/contrib/cros... tools/perf/contrib/cros_benchmarks/tab_switching_bench.py:9: from contrib.cros_benchmarks.tab_switching_measure import TabSwitchingMeasurement On 2017/05/19 09:49:46, deanliao_goog wrote: > Import module, not class. > https://google.github.io/styleguide/pyguide.html?showone=Imports#Imports Done. https://codereview.chromium.org/2890333002/diff/20001/tools/perf/contrib/cros... tools/perf/contrib/cros_benchmarks/tab_switching_bench.py:19: """This test records the MPArch.RWH_TabSwitchPaintDuration histogram. On 2017/05/19 09:49:46, deanliao_goog wrote: > Please rewrite the one-line description of the benchmark. "It records what... " > should be placed in detail or even in measurement code. > > Also please rewrite the long description to explain what this benchmark wants to > measure and how it can be used. Done. https://codereview.chromium.org/2890333002/diff/20001/tools/perf/contrib/cros... File tools/perf/contrib/cros_benchmarks/tab_switching_stories.py (right): https://codereview.chromium.org/2890333002/diff/20001/tools/perf/contrib/cros... tools/perf/contrib/cros_benchmarks/tab_switching_stories.py:24: # context would be invalidated. avoid store tab object for later use. On 2017/05/19 09:09:14, bccheng wrote: > Capitalize the sentences in comments. For example: > > NOTE: When tab count is high, ... > > some tab -> some tabs > > avoid store -> Avoid storing tab objects Done.
Description was changed from ========== Tab Switching Benchmark for ChromeOS Copy the tab switching benchmark to cros_benchmarks.Using key events for simulate tab switching behavior to handle the case that some tab contexts may be discarded. BUG=724381 ========== to ========== Tab Switching Benchmark for ChromeOS Copy the tab switching benchmark to cros_benchmarks.Using key events for simulate tab switching behavior to handle the case that some tab contexts may be discarded. Example usage to open 120 tabs: $ /run_benchmark --browser=cros-chrome --remote=DUT_IP cros_tab_switching.typical_24 --tabset-repeat=5 BUG=724381 ==========
Description was changed from ========== Tab Switching Benchmark for ChromeOS Copy the tab switching benchmark to cros_benchmarks.Using key events for simulate tab switching behavior to handle the case that some tab contexts may be discarded. Example usage to open 120 tabs: $ /run_benchmark --browser=cros-chrome --remote=DUT_IP cros_tab_switching.typical_24 --tabset-repeat=5 BUG=724381 ========== to ========== Tab Switching Benchmark for ChromeOS Copy the tab switching benchmark to cros_benchmarks.Using key events for simulate tab switching behavior to handle the case that some tab contexts may be discarded. Example usage to open 120 tabs: $ ./run_benchmark --browser=cros-chrome --remote=DUT_IP cros_tab_switching.typical_24 --tabset-repeat=5 BUG=724381 ==========
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.
bccheng@chromium.org changed reviewers: + cywang@chromium.org
Adding cywang@ in case he has comments for the emulated keyboard.
https://codereview.chromium.org/2890333002/diff/60001/tools/perf/contrib/cros... File tools/perf/contrib/cros_benchmarks/cros_utils.py (right): https://codereview.chromium.org/2890333002/diff/60001/tools/perf/contrib/cros... tools/perf/contrib/cros_benchmarks/cros_utils.py:16: def FindKeyboardDevice(dut_ip): Try to use an emulated keyboard instead(e.g. use evemu-device with a property file like https://chromium.googlesource.com/chromiumos/third_party/autotest/+/master/cl...), so we could still run the test w/o physical keyboard. However, you will need to remove the emulated device during cleanup.
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/2890333002/diff/60001/tools/perf/contrib/cros... File tools/perf/contrib/cros_benchmarks/cros_utils.py (right): https://codereview.chromium.org/2890333002/diff/60001/tools/perf/contrib/cros... tools/perf/contrib/cros_benchmarks/cros_utils.py:16: def FindKeyboardDevice(dut_ip): On 2017/05/22 09:04:04, cywang wrote: > Try to use an emulated keyboard instead(e.g. use evemu-device with a property > file like > https://chromium.googlesource.com/chromiumos/third_party/autotest/+/master/cl...), > so we could still run the test w/o physical keyboard. However, you will need to > remove the emulated device during cleanup. Done. Using keyboard emulator to send key events.
Description was changed from ========== Tab Switching Benchmark for ChromeOS Copy the tab switching benchmark to cros_benchmarks.Using key events for simulate tab switching behavior to handle the case that some tab contexts may be discarded. Example usage to open 120 tabs: $ ./run_benchmark --browser=cros-chrome --remote=DUT_IP cros_tab_switching.typical_24 --tabset-repeat=5 BUG=724381 ========== to ========== Tab Switching Benchmark for ChromeOS Copy the tab switching benchmark to cros_benchmarks. Using key events for simulate tab switching behavior to handle the case that some tab contexts may be discarded. Example usage to open 120 tabs: $ ./run_benchmark --browser=cros-chrome --remote=DUT_IP cros_tab_switching.typical_24 --tabset-repeat=5 BUG=724381 ==========
https://codereview.chromium.org/2890333002/diff/80001/tools/perf/contrib/cros... File tools/perf/contrib/cros_benchmarks/cros_utils.py (right): https://codereview.chromium.org/2890333002/diff/80001/tools/perf/contrib/cros... tools/perf/contrib/cros_benchmarks/cros_utils.py:82: '/usr/local/autotest/cros/input_playback/keyboard.prop'] LGTM in keyboard emulation, please make sure if the property file exists in case there is any refactor in autotest repo.
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/2890333002/diff/80001/tools/perf/contrib/cros... File tools/perf/contrib/cros_benchmarks/cros_utils.py (right): https://codereview.chromium.org/2890333002/diff/80001/tools/perf/contrib/cros... tools/perf/contrib/cros_benchmarks/cros_utils.py:82: '/usr/local/autotest/cros/input_playback/keyboard.prop'] On 2017/05/23 02:56:57, cywang wrote: > LGTM in keyboard emulation, please make sure if the property file exists in case > there is any refactor in autotest repo. Done. Added checks to through exception if the property file doesn't exist.
On 2017/05/23 03:24:10, vovoy wrote: > https://codereview.chromium.org/2890333002/diff/80001/tools/perf/contrib/cros... > File tools/perf/contrib/cros_benchmarks/cros_utils.py (right): > > https://codereview.chromium.org/2890333002/diff/80001/tools/perf/contrib/cros... > tools/perf/contrib/cros_benchmarks/cros_utils.py:82: > '/usr/local/autotest/cros/input_playback/keyboard.prop'] > On 2017/05/23 02:56:57, cywang wrote: > > LGTM in keyboard emulation, please make sure if the property file exists in > case > > there is any refactor in autotest repo. > > Done. Added checks to through exception if the property file doesn't exist. Thanks! looks good to me!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
lgtm
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.
https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... File tools/perf/contrib/cros_benchmarks/cros_utils.py (right): https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:18: Ah, Python style rule said that top level methods and classes should be separated by two blank lines. https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:19: def GetTabSwitchHistogram(tab): Please add description including Args: and Returns: section for public functions. https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:26: """Retry GetHistogram() as it may fail when a context was discarded.""" Retries... Same below. https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:27: while True: So it is unlikely that GetTabSwitchHistogram failed indefinitely? How about adding "retry" parameter to set maximum number of retries? Ah, or you rely on py_utils.WaitFor() to terminate the function call if it retries indefinitely? https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:42: """Checking the histogram to wait for tab switching completion.""" Waits for tab switching complete. It is done by checking browser histogram to see if RWH_TabSwitchPaintDuration count increases. https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:54: logging.warning('wait for histogram timed out') Wait https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:56: class KeyboardEmulator(object): class description. https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:64: send_key_filename = cur_dir + '/data/send_key_tab_switch' Though it is okay, but using os.path.join(cur_dir, 'data', 'send_key_tab_switch') is better as os.path.join() will handle some case like trailing / in cur_dir. https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:68: send_key_content += ('evemu-play --insert-slot0 ' + device_name + How about just with open(...) as f: f.write('#!/bin/bash\n') f.write('evemu-play --insert-slot0 %s < /home/root/log_key_tab_switch' % device_name) Also, is it correct to direct /home/root/log_key_tab_switch into evemu-play? https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:83: if ret is not 0: if ret != 0: https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:86: cmd = ['ssh', 'root@' + self._dut_ip, 'evemu-device', kbd_prop_filename] FYI, looks like 'root@' + self._dut_ip occurs three times, you may move it to self._root_dut. https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:103: # The remote process would live when the ssh process was terminated. Since the remote process stays alive when SSH is terminated, how about just end the process in __enter__? output = subprocess.check_output(['ssh', ...]) so that you don't need to hold self._process and kill it here. https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:110: """Set screen off and dim timer to 1 hour.""" Sets screen always on for one hour. https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... File tools/perf/contrib/cros_benchmarks/tab_switching_bench.py (right): https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/tab_switching_bench.py:19: """Measuring the tab switching performance when there are many tabs. Measures tab switching performance with 24 tabs. https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/tab_switching_bench.py:24: time between when a tab was requested to be shwon, and when first paint time between tab being requested and the first paint event. https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/tab_switching_bench.py:27: --tabset-repeat=N: Multiply the tab count by N to open more tabs. How about this: Benchmark specific option: --tabset-repeat=N: Duplicate tab set for N times. https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... File tools/perf/contrib/cros_benchmarks/tab_switching_measure.py (right): https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/tab_switching_measure.py:34: total_diff_histogram = histogram_util.SubtractHistogram(last_histogram, Wrapping indentation: either aligned, e.g. SomeFunction(arg1, arg2) or wrap all args with 4 space indent: SomeFunction( arg1, arg2) https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... File tools/perf/contrib/cros_benchmarks/tab_switching_stories.py (right): https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/tab_switching_stories.py:41: print 'opening tab:', i Is it needed for Telemetry benchmark? Isn't it for debugging purpose? I would say moving it to logging.debug()
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...
Thanks for your thoroughly review. https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... File tools/perf/contrib/cros_benchmarks/cros_utils.py (right): https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:18: On 2017/05/23 08:54:25, deanliao_goog wrote: > Ah, Python style rule said that top level methods and classes should be > separated by two blank lines. Done. https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:19: def GetTabSwitchHistogram(tab): On 2017/05/23 08:54:25, deanliao_goog wrote: > Please add description including Args: and Returns: section for public > functions. Done. https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:26: """Retry GetHistogram() as it may fail when a context was discarded.""" On 2017/05/23 08:54:26, deanliao_goog wrote: > Retries... > > Same below. Done. https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:27: while True: On 2017/05/23 08:54:26, deanliao_goog wrote: > So it is unlikely that GetTabSwitchHistogram failed indefinitely? How about > adding "retry" parameter to set maximum number of retries? > > Ah, or you rely on py_utils.WaitFor() to terminate the function call if it > retries indefinitely? Done. Using py_utils.WaitFor(). https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:42: """Checking the histogram to wait for tab switching completion.""" On 2017/05/23 08:54:25, deanliao_goog wrote: > Waits for tab switching complete. > > It is done by checking browser histogram to see if RWH_TabSwitchPaintDuration > count increases. Done. https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:54: logging.warning('wait for histogram timed out') On 2017/05/23 08:54:25, deanliao_goog wrote: > Wait Done. https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:56: class KeyboardEmulator(object): On 2017/05/23 08:54:25, deanliao_goog wrote: > class description. Done. https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:64: send_key_filename = cur_dir + '/data/send_key_tab_switch' On 2017/05/23 08:54:26, deanliao_goog wrote: > Though it is okay, but using > os.path.join(cur_dir, 'data', 'send_key_tab_switch') > is better as os.path.join() will handle some case like trailing / in cur_dir. Done. https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:68: send_key_content += ('evemu-play --insert-slot0 ' + device_name + On 2017/05/23 08:54:25, deanliao_goog wrote: > How about just > > with open(...) as f: > f.write('#!/bin/bash\n') > f.write('evemu-play --insert-slot0 %s < /home/root/log_key_tab_switch' % > device_name) > > Also, is it correct to direct /home/root/log_key_tab_switch into evemu-play? Done. It shall be OK to direct /home/root/log_key_tab_switch into evemu-play. It's the same way as InputPlayback. https://cs.corp.google.com/chromeos_public/src/third_party/autotest/files/cli... https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:83: if ret is not 0: On 2017/05/23 08:54:25, deanliao_goog wrote: > if ret != 0: Done. I think 0 (integer literal) is singleton in python that "!= 0" and "is not 0" shall both work? https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:86: cmd = ['ssh', 'root@' + self._dut_ip, 'evemu-device', kbd_prop_filename] On 2017/05/23 08:54:25, deanliao_goog wrote: > FYI, looks like > 'root@' + self._dut_ip > occurs three times, you may move it to self._root_dut. Done. https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:103: # The remote process would live when the ssh process was terminated. On 2017/05/23 08:54:26, deanliao_goog wrote: > Since the remote process stays alive when SSH is terminated, how about just end > the process in __enter__? > > output = subprocess.check_output(['ssh', ...]) > > so that you don't need to hold self._process and kill it here. Done. https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:110: """Set screen off and dim timer to 1 hour.""" On 2017/05/23 08:54:26, deanliao_goog wrote: > Sets screen always on for one hour. Done. https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... File tools/perf/contrib/cros_benchmarks/tab_switching_bench.py (right): https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/tab_switching_bench.py:19: """Measuring the tab switching performance when there are many tabs. On 2017/05/23 08:54:26, deanliao_goog wrote: > Measures tab switching performance with 24 tabs. Done. https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/tab_switching_bench.py:24: time between when a tab was requested to be shwon, and when first paint On 2017/05/23 08:54:26, deanliao_goog wrote: > time between tab being requested and the first paint event. Done. https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/tab_switching_bench.py:27: --tabset-repeat=N: Multiply the tab count by N to open more tabs. On 2017/05/23 08:54:26, deanliao_goog wrote: > How about this: > > Benchmark specific option: > --tabset-repeat=N: Duplicate tab set for N times. Done. https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... File tools/perf/contrib/cros_benchmarks/tab_switching_stories.py (right): https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/tab_switching_stories.py:41: print 'opening tab:', i On 2017/05/23 08:54:26, deanliao_goog wrote: > Is it needed for Telemetry benchmark? Isn't it for debugging purpose? I would > say moving it to logging.debug() It's handy to see the progress on the console when testing 100+ tabs. logging.debug() would not print on the console by default. Is it OK to create a logging.Logger and setlevel(logging.DEBUG") to display all messages?
Some tiny picks. You may submit it at will after fixing them. https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... File tools/perf/contrib/cros_benchmarks/cros_utils.py (right): https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:83: if ret is not 0: On 2017/05/24 09:17:21, vovoy wrote: > On 2017/05/23 08:54:25, deanliao_goog wrote: > > if ret != 0: > > Done. > I think 0 (integer literal) is singleton in python that "!= 0" and "is not 0" > shall both work? When handling integers, implicit false may involve more risk than benefit (i.e., accidentally handling None as 0). You may compare a value which is known to be an integer (and is not the result of len()) against the integer 0. https://google.github.io/styleguide/pyguide.html?showone=True/False_evaluatio... https://codereview.chromium.org/2890333002/diff/160001/tools/perf/contrib/cro... File tools/perf/contrib/cros_benchmarks/cros_utils.py (right): https://codereview.chromium.org/2890333002/diff/160001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:37: Returns: One blank line between Args: and Returns: section. https://codereview.chromium.org/2890333002/diff/160001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:98: """Setups a remote emulated keyboard and sends key events to switch tab. Sets up https://codereview.chromium.org/2890333002/diff/160001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:127: ssh_process = subprocess.Popen(cmd, stdout=subprocess.PIPE) Ah, is it because subprocess.check_output(['ssh', ...]) won't exist? https://codereview.chromium.org/2890333002/diff/160001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:161: Redundant blank line.
lgtm
https://codereview.chromium.org/2890333002/diff/160001/tools/perf/contrib/cro... File tools/perf/contrib/cros_benchmarks/cros_utils.py (right): https://codereview.chromium.org/2890333002/diff/160001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:37: Returns: On 2017/05/24 10:10:52, deanliao_goog wrote: > One blank line between Args: and Returns: section. Done. https://codereview.chromium.org/2890333002/diff/160001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:98: """Setups a remote emulated keyboard and sends key events to switch tab. On 2017/05/24 10:10:53, deanliao_goog wrote: > Sets up Done. https://codereview.chromium.org/2890333002/diff/160001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:127: ssh_process = subprocess.Popen(cmd, stdout=subprocess.PIPE) On 2017/05/24 10:10:52, deanliao_goog wrote: > Ah, is it because subprocess.check_output(['ssh', ...]) won't exist? Yes, subprocess.check_output(['ssh', 'evemu-device', ...]) would block indefinitely. It's necessary to kill the ssh process manually. https://codereview.chromium.org/2890333002/diff/160001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:161: On 2017/05/24 10:10:52, deanliao_goog wrote: > Redundant blank line. Done.
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/2890333002/diff/120001/tools/perf/contrib/cro... File tools/perf/contrib/cros_benchmarks/tab_switching_stories.py (right): https://codereview.chromium.org/2890333002/diff/120001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/tab_switching_stories.py:41: print 'opening tab:', i On 2017/05/24 09:17:22, vovoy wrote: > On 2017/05/23 08:54:26, deanliao_goog wrote: > > Is it needed for Telemetry benchmark? Isn't it for debugging purpose? I would > > say moving it to logging.debug() > > It's handy to see the progress on the console when testing 100+ tabs. > logging.debug() would not print on the console by default. > Is it OK to create a logging.Logger and setlevel(logging.DEBUG") to display all > messages? I would keep using print here. The 'opening tab: 0' messages are just for showing progress to user and it's not necessary to log these to file. Let me know if there are some drawbacks using print here.
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 cywang@chromium.org, bccheng@chromium.org, deanliao@google.com Link to the patchset: https://codereview.chromium.org/2890333002/#ps200001 (title: "Style fixes")
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
bccheng@chromium.org changed reviewers: + cylee@chromium.org
cylee@ you are a committer. Could you take a look and give lgtm?
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
lgtm with nits https://codereview.chromium.org/2890333002/diff/220001/tools/perf/contrib/cro... File tools/perf/contrib/cros_benchmarks/cros_utils.py (right): https://codereview.chromium.org/2890333002/diff/220001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:33: than discarding the first valid context. chance https://codereview.chromium.org/2890333002/diff/220001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:46: except exceptions.DevtoolsTargetCrashException: Does except (exception1, exception2) work to merge the two exceptions? https://codereview.chromium.org/2890333002/diff/220001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:69: return py_utils.WaitFor(_TryGetHistogram, 10) Could _TryGetHistogram be replaced by lambda: _GetTabSwitchHistogram(browser) ? https://codereview.chromium.org/2890333002/diff/220001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:138: raise RuntimeError('Keyboard emulation failed.') ssh_process.kill() wouldn't be executed if an error is thrown here? https://codereview.chromium.org/2890333002/diff/220001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:154: f.write('#!/bin/bash\n') could you put the whole file content in a string for better readability ?
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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 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/2890333002/diff/220001/tools/perf/contrib/cro... File tools/perf/contrib/cros_benchmarks/cros_utils.py (right): https://codereview.chromium.org/2890333002/diff/220001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:33: than discarding the first valid context. On 2017/05/25 21:21:35, cylee1 wrote: > chance Done. https://codereview.chromium.org/2890333002/diff/220001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:46: except exceptions.DevtoolsTargetCrashException: On 2017/05/25 21:21:35, cylee1 wrote: > Does except (exception1, exception2) work to merge the two exceptions? Done. https://codereview.chromium.org/2890333002/diff/220001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:69: return py_utils.WaitFor(_TryGetHistogram, 10) On 2017/05/25 21:21:34, cylee1 wrote: > Could _TryGetHistogram be replaced by > lambda: _GetTabSwitchHistogram(browser) > ? Done. https://codereview.chromium.org/2890333002/diff/220001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:138: raise RuntimeError('Keyboard emulation failed.') On 2017/05/25 21:21:35, cylee1 wrote: > ssh_process.kill() wouldn't be executed if an error is thrown here? Fixed. Kills before throwing exception. https://codereview.chromium.org/2890333002/diff/220001/tools/perf/contrib/cro... tools/perf/contrib/cros_benchmarks/cros_utils.py:154: f.write('#!/bin/bash\n') On 2017/05/25 21:21:34, cylee1 wrote: > could you put the whole file content in a string for better readability ? Rewritten. Runs the command directly instead of uploading a script file.
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 bccheng@chromium.org, deanliao@google.com, cywang@chromium.org, cylee@chromium.org Link to the patchset: https://codereview.chromium.org/2890333002/#ps280001 (title: "Tab Switching Benchmark for ChromeOS")
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": 280001, "attempt_start_ts": 1495790439705040,
"parent_rev": "b0a2e7079c5723d2a3677f6ce4078c65d42b08cb", "commit_rev":
"2675d3c72d3ad2257e6efb8775b1c54266ccfb7e"}
Message was sent while issue was closed.
Description was changed from ========== Tab Switching Benchmark for ChromeOS Copy the tab switching benchmark to cros_benchmarks. Using key events for simulate tab switching behavior to handle the case that some tab contexts may be discarded. Example usage to open 120 tabs: $ ./run_benchmark --browser=cros-chrome --remote=DUT_IP cros_tab_switching.typical_24 --tabset-repeat=5 BUG=724381 ========== to ========== Tab Switching Benchmark for ChromeOS Copy the tab switching benchmark to cros_benchmarks. Using key events for simulate tab switching behavior to handle the case that some tab contexts may be discarded. Example usage to open 120 tabs: $ ./run_benchmark --browser=cros-chrome --remote=DUT_IP cros_tab_switching.typical_24 --tabset-repeat=5 BUG=724381 Review-Url: https://codereview.chromium.org/2890333002 Cr-Commit-Position: refs/heads/master@{#474975} Committed: https://chromium.googlesource.com/chromium/src/+/2675d3c72d3ad2257e6efb8775b1... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as https://chromium.googlesource.com/chromium/src/+/2675d3c72d3ad2257e6efb8775b1... |
