[Telemetry] Speed up page_cycler on android.

build/android/pylib/android_commands.py

 # Copyright (c) 2012 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.
 
 """Provides an interface to communicate with the device via the adb command.
 
 Assumes adb binary is currently on system path.
 """
 # pylint: disable-all
 
@@ skipping 37 matching lines @@
 # Set the adb shell prompt to be a unique marker that will [hopefully] not
 # appear at the start of any line of a command's output.
 SHELL_PROMPT = '~+~PQ\x17RS~+~'
 
 # Java properties file
 LOCAL_PROPERTIES_PATH = constants.DEVICE_LOCAL_PROPERTIES_PATH
 
 # Property in /data/local.prop that controls Java assertions.
 JAVA_ASSERT_PROPERTY = 'dalvik.vm.enableassertions'
 
-MEMORY_INFO_RE = re.compile('^(?P<key>\w+):\s+(?P<usage_kb>\d+) kB$')
-NVIDIA_MEMORY_INFO_RE = re.compile('^\s*(?P<user>\S+)\s*(?P<name>\S+)\s*'
-                                   '(?P<pid>\d+)\s*(?P<usage_bytes>\d+)$')
-
 # Keycode "enum" suitable for passing to AndroidCommands.SendKey().
 KEYCODE_HOME = 3
 KEYCODE_BACK = 4
 KEYCODE_DPAD_UP = 19
 KEYCODE_DPAD_DOWN = 20
 KEYCODE_DPAD_RIGHT = 22
 KEYCODE_ENTER = 66
 KEYCODE_MENU = 82
 
 MD5SUM_DEVICE_FOLDER = constants.TEST_EXECUTABLE_DIR + '/md5sum/'
@@ skipping 1519 matching lines @@
     """Returns the memory usage for given pid.
 
     Args:
       pid: The pid number of the specific process running on device.
 
     Returns:
       A tuple containg:
       [0]: Dict of {metric:usage_kb}, for the process which has specified pid.
       The metric keys which may be included are: Size, Rss, Pss, Shared_Clean,
       Shared_Dirty, Private_Clean, Private_Dirty, Referenced, Swap,
-      KernelPageSize, MMUPageSize, Nvidia (tablet only), VmHWM.
+      KernelPageSize, MMUPageSize, VmHWM.
       [1]: Detailed /proc/[PID]/smaps information.
     """
     usage_dict = collections.defaultdict(int)
     smaps = collections.defaultdict(dict)
     current_smap = ''
     for line in self.GetProtectedFileContents('/proc/%s/smaps' % pid):
       items = line.split()
       # See man 5 proc for more details. The format is:
       # address perms offset dev inode pathname
       if len(items) > 5:
         current_smap = ' '.join(items[5:])
       elif len(items) > 3:
         current_smap = ' '.join(items[3:])
-      match = re.match(MEMORY_INFO_RE, line)
-      if match:
-        key = match.group('key')
-        usage_kb = int(match.group('usage_kb'))
+      if line.endswith('kB'):
+        key, value = line.split(':')
+        key = key.strip()
+        usage_kb = int(value.strip().split()[0])
         usage_dict[key] += usage_kb
         if key not in smaps[current_smap]:
           smaps[current_smap][key] = 0
         smaps[current_smap][key] += usage_kb
     if not usage_dict or not any(usage_dict.values()):
       # Presumably the process died between ps and calling this method.
       logging.warning('Could not find memory usage for pid ' + str(pid))
 
-    for line in self.GetProtectedFileContents('/d/nvmap/generic-0/clients'):

[tonyg, 2014/05/13 14:37:50]

Are you sure we don't care about this any more for any nexus on the bots? If it
is at all useful, we could also perhaps check for the file once, cache whether
it exists and then shortcut reading the file if it didn't exist.

[bulach, 2014/05/13 14:45:57]

well... :)
way up in telemetry, this is not being used at all:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...

I'm not aware of any bots looking into this.
ok to nuke it? git log would show anyways, should we ever want to bring this
back...

-      match = re.match(NVIDIA_MEMORY_INFO_RE, line)
-      if match and match.group('pid') == pid:
-        usage_bytes = int(match.group('usage_bytes'))
-        usage_dict['Nvidia'] = int(round(usage_bytes / 1000.0))  # kB
-        break
-
     peak_value_kb = 0
     for line in self.GetProtectedFileContents('/proc/%s/status' % pid):
       if not line.startswith('VmHWM:'):  # Format: 'VmHWM: +[0-9]+ kB'
         continue
       peak_value_kb = int(line.split(':')[1].strip().split(' ')[0])

[picksi, 2014/05/14 16:32:18]

It looks like the code to read the peak_value_kb is almost the same as the new
(much nicer!) code you added above to read the key & usage_kb. Is there a way to
avoid this (almost) duplication? [... long discussion about functions returning
keys & ints that aren't always used]

[bulach, 2014/05/14 16:52:56]

hehe :) there was another follow up on this:
https://codereview.chromium.org/280703010/diff/20001/build/android/pylib/andr...

so this duplication is no longer happening: the bit above is now using "showmap"
command, which faster but has a slightly different format..

having said that, thanks for highlighting this! there's no need to use a
"defaultdict" anymore, patch on its way:
https://codereview.chromium.org/285693008

+      break
     usage_dict['VmHWM'] = peak_value_kb
     if not peak_value_kb:
       logging.warning('Could not find memory peak value for pid ' + str(pid))

[picksi, 2014/05/14 16:32:18]

This relies on a zero peak value meaning it couldn't find a peak value. I assume
that a zero-peak is not possible, but this feels like using one variable for two
separate jobs. Especially as 'if not peak_value_kb' is treating peak_value_kb as
a boolean, when we all know its really an int!

Now the 'break' exists we could write the peak value directly into the
usage_dict (unless writing 0 in is necessary?), and then use it's absence from
the dictionary to trigger the logging warning... or set a 'key found' flag to
true if efficiency is an issue...

I've gone too far down this rabbit hole for a single line of code now ...

[bulach, 2014/05/14 16:52:56]

yeah, this is a good use for python's "for..else" construction!


(having said that, afaict the styleguide allows using boolean coercion for ints
and None on "if" clauses, so it wasn't truly wrong given that it can't have a 0
peak... but yes, it'll be clearer now anyways :)

 
     return (usage_dict, smaps)
 
   def GetMemoryUsageForPackage(self, package):
     """Returns the memory usage for all processes whose name contains |pacakge|.
 
     Args:
       package: A string holding process name to lookup pid list for.
 
     Returns:
@@ skipping 317 matching lines @@
   """
   def __init__(self, output):
     self._output = output
 
   def write(self, data):
     data = data.replace('\r\r\n', '\n')
     self._output.write(data)
 
   def flush(self):
     self._output.flush()