Tab Switching Benchmark for ChromeOS

tools/perf/contrib/cros_benchmarks/cros_utils.py

+# Copyright 2017 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import json
+import logging
+import os
+import subprocess
+import time
+
+import py_utils
+
+from telemetry.core import exceptions
+from telemetry.value import histogram_util
+
+def RunRemoteCommand(dut_ip, cmd):
+  return os.system('ssh root@%s %s' % (dut_ip, cmd))
+

[deanliao_goog, 2017/05/23 08:54:25]

Ah, Python style rule said that top level methods and classes should be
separated by two blank lines.

[vovoy, 2017/05/24 09:17:21]

Done.

+def GetTabSwitchHistogram(tab):

[deanliao_goog, 2017/05/23 08:54:25]

Please add description including Args: and Returns: section for public
functions.

[vovoy, 2017/05/24 09:17:21]

Done.

+  histogram_name = 'MPArch.RWH_TabSwitchPaintDuration'
+  histogram_type = histogram_util.BROWSER_HISTOGRAM
+  return histogram_util.GetHistogram(
+      histogram_type, histogram_name, tab)
+
+def GetTabSwitchHistogramRetry(browser):
+  """Retry GetHistogram() as it may fail when a context was discarded."""

[deanliao_goog, 2017/05/23 08:54:26]

Retries...

Same below.

[vovoy, 2017/05/24 09:17:21]

Done.

+  while True:

[deanliao_goog, 2017/05/23 08:54:26]

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?

[vovoy, 2017/05/24 09:17:21]

Done. Using py_utils.WaitFor().

+    try:
+      # Any tab can be used to get the TabSwitchPaintDuration histogram.
+      # At each retry, try to get valid context again.
+      return GetTabSwitchHistogram(browser.tabs[-1])
+    except exceptions.DevtoolsTargetCrashException:
+      logging.info('GetHistogram: devtools context lost')
+    except KeyError:
+      logging.info('GetHistogram: devtools context lost')
+    except Exception as e:
+      logging.warning('GetHistogram: exception')
+      logging.warning(e)
+    time.sleep(0.5)
+
+def WaitTabSwitching(browser, prev_histogram):
+  """Checking the histogram to wait for tab switching completion."""

[deanliao_goog, 2017/05/23 08:54:25]

Waits for tab switching complete.

It is done by checking browser histogram to see if RWH_TabSwitchPaintDuration
count increases.

[vovoy, 2017/05/24 09:17:21]

Done.

+  def _IsDone():
+    cur_histogram = GetTabSwitchHistogramRetry(browser)
+
+    diff_histogram = histogram_util.SubtractHistogram(cur_histogram,
+                            prev_histogram)
+    diff_histogram_count = json.loads(diff_histogram).get('count', 0)
+    return diff_histogram_count > 0
+
+  try:
+    py_utils.WaitFor(_IsDone, 10)
+  except py_utils.TimeoutException:
+    logging.warning('wait for histogram timed out')

[deanliao_goog, 2017/05/23 08:54:25]

Wait

[vovoy, 2017/05/24 09:17:21]

Done.

+
+class KeyboardEmulator(object):

[deanliao_goog, 2017/05/23 08:54:25]

class description.

[vovoy, 2017/05/24 09:17:21]

Done.

+  def __init__(self, dut_ip):
+    self._dut_ip = dut_ip
+    self._process = None
+
+  def _SetupKeyDispatch(self, device_name):
+    """Uploading the script to send key to switch tabs."""
+    cur_dir = os.path.dirname(os.path.abspath(__file__))
+    send_key_filename = cur_dir + '/data/send_key_tab_switch'

[deanliao_goog, 2017/05/23 08:54:26]

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.

[vovoy, 2017/05/24 09:17:21]

Done.

+    log_key_filename = cur_dir + '/data/log_key_tab_switch'
+
+    send_key_content = '#!/bin/bash\n'
+    send_key_content += ('evemu-play --insert-slot0 ' + device_name +

[deanliao_goog, 2017/05/23 08:54:25]

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? 

[vovoy, 2017/05/24 09:17:21]

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...

+                       ' < /home/root/log_key_tab_switch')
+    with open(send_key_filename, 'w') as f:
+      f.write(send_key_content)
+    os.chmod(send_key_filename, 0774)
+
+    os.system('scp -q %s root@%s:/usr/local/tmp/' %
+              (log_key_filename, self._dut_ip))
+    os.system('scp -q %s root@%s:/usr/local/tmp/' %
+              (send_key_filename, self._dut_ip))
+
+  def __enter__(self):
+    kbd_prop_filename = '/usr/local/autotest/cros/input_playback/keyboard.prop'
+
+    ret = RunRemoteCommand(self._dut_ip, 'test -e %s' % kbd_prop_filename)
+    if ret is not 0:

[deanliao_goog, 2017/05/23 08:54:25]

if ret != 0:

[vovoy, 2017/05/24 09:17:21]

Done.
I think 0 (integer literal) is singleton in python that "!= 0" and "is not 0"
shall both work?

[deanliao_goog, 2017/05/24 10:10:52]

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...

+      raise RuntimeError('Keyboard property file does not exist.')
+
+    cmd =  ['ssh', 'root@' + self._dut_ip, 'evemu-device', kbd_prop_filename]

[deanliao_goog, 2017/05/23 08:54:25]

FYI, looks like 
'root@' + self._dut_ip
occurs three times, you may move it to self._root_dut.

[vovoy, 2017/05/24 09:17:21]

Done.

+    self._process = subprocess.Popen(cmd, stdout=subprocess.PIPE)
+
+    # The evemu-device output format:
+    # Emulated Keyboard: /dev/input/event10
+    output = self._process.stdout.readline()
+    if not output.startswith('Emulated Keyboard:'):
+      raise RuntimeError('Keyboard emulation failed.')
+    key_device_name = output.split()[2]
+    self._SetupKeyDispatch(key_device_name)
+    return self
+
+  def SwitchTab(self):
+    """Sending Ctrl-tab key to trigger tab switching."""
+    RunRemoteCommand(self._dut_ip, '/usr/local/tmp/send_key_tab_switch')
+
+  def __exit__(self, exc_type, exc_value, traceback):
+    # The remote process would live when the ssh process was terminated.

[deanliao_goog, 2017/05/23 08:54:26]

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.

[vovoy, 2017/05/24 09:17:21]

Done.

+    # Need to kill it explicitly.
+    self._process.kill()
+    RunRemoteCommand(self._dut_ip, 'pkill evemu-device')
+
+
+def NoScreenOff(dut_ip):
+  """Set screen off and dim timer to 1 hour."""

[deanliao_goog, 2017/05/23 08:54:26]

Sets screen always on for one hour.

[vovoy, 2017/05/24 09:17:21]

Done.

+  RunRemoteCommand(dut_ip, 'set_power_policy --ac_screen_off_delay=3600')
+  RunRemoteCommand(dut_ip, 'set_power_policy --ac_screen_dim_delay=3600')