[Android] Tune DeviceUtils commands that are prone to large outputs.

build/android/pylib/device/device_utils.py

 # Copyright 2014 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 a variety of device interactions based on adb.
 
 Eventually, this will be based on adb_wrapper.
 """
 # pylint: disable=unused-argument
 
@@ skipping 575 matching lines @@
       blocking: A boolean indicating whether we should wait until all processes
                 with the given |process_name| are dead.
       timeout: timeout in seconds
       retries: number of retries
 
     Raises:
       CommandFailedError if no process was killed.
       CommandTimeoutError on timeout.
       DeviceUnreachableError on missing device.
     """
-    pids = self._GetPidsImpl(process_name)
+    pids = self.GetPids(process_name)
     if not pids:
       raise device_errors.CommandFailedError(
           'No process "%s"' % process_name, str(self))
 
     cmd = ['kill', '-%d' % signum] + pids.values()
     self.RunShellCommand(cmd, as_root=as_root, check_return=True)
 
     if blocking:
       wait_period = 0.1
-      while self._GetPidsImpl(process_name):
+      while self.GetPids(process_name):
         time.sleep(wait_period)
 
     return len(pids)
 
   @decorators.WithTimeoutAndRetriesFromInstance()
   def StartActivity(self, intent_obj, blocking=False, trace_file_name=None,
                     force_stop=False, timeout=None, retries=None):
     """Start package's activity on the device.
 
     Args:
@@ skipping 396 matching lines @@
     for line in ls_out:
       m = self._LS_RE.match(line)
       if m and m.group('name') == posixpath.basename(device_path):
         size = int(m.group('size'))
         break
     else:
       logging.warning('Could not determine size of %s.', device_path)
 
     if size is None or size <= self._MAX_ADB_OUTPUT_LENGTH:
       return _JoinLines(self.RunShellCommand(
-          ['cat', device_path], as_root=as_root, check_return=True))
+          ['cat', device_path], as_root=as_root, check_return=True,
+          large_output=(not bool(size))))

[perezju, 2015/04/13 09:58:32]

I don't think this makes much sense, and introduces a cyclic dependency that
makes me worry.

We are using "cat" as an optimization to avoid having to pull the file from the
device. But with large_output=True we'll instead cat the file in the device to
_another_ file which is then read using ... ReadFile!

I would prefer:
- If we can't determine the size of the file, play it safe and just do a pull.
- If the file of the size is 0, don't event try to read anything from the
device, just return '' (or '\n'?).
- Only use cat if we know that the size is small enough so that large_output
would not be needed (otherwise the "optimization" is pointless).

[rnephew (Wrong account), 2015/04/13 17:21:04]

In at least the /proc/ fs on the device, and probably elsewhere, some files
return 0 size even though they are not 0 size. One example is /proc/wakelocks:
adb shell ls -l /proc/wakelocks ; adb shell cat /proc/wakelocks | wc
-r--r--r-- root     root            0 2015-04-13 13:19 wakelocks
     64     576    3729

[jbudorick, 2015/04/13 17:44:29]

This doesn't make much sense because what it's reading doesn't really make much
sense.

e.g. telemetry reads /proc/timer_list here:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...

/proc/timer_list reports zero size, but actually will dump quite a bit of data
when catting:

jbudorick@jbudorick:~$ adb shell ls -l /proc/timer_list && adb pull
/proc/timer_list timer_list.txt && ls -l timer_list.txt
-r--r--r-- root     root            0 2015-04-13 08:18 timer_list
70 KB/s (5670 bytes in 0.078s)
-rw-r----- 1 jbudorick eng 5670 Apr 13 06:18 timer_list.txt
I had thought that pull didn't work with files that report zero size, but it
certainly seems to do so. I'll check this again on K and then switch so long as
it works.
This is wrong, see above.

     elif as_root and self.NeedsSU():
       with device_temp_file.DeviceTempFile(self.adb) as device_temp:
         self.RunShellCommand(['cp', device_path, device_temp.name],
                              as_root=True, check_return=True)
         return self._ReadFileWithPull(device_temp.name)
     else:
       return self._ReadFileWithPull(device_path)
 
   def _WriteFileWithPush(self, device_path, contents):
     with tempfile.NamedTemporaryFile() as host_temp:
@@ skipping 312 matching lines @@
       retries: number of retries
 
     Returns:
       A dict mapping process name to PID for each process that contained the
       provided |process_name|.
 
     Raises:
       CommandTimeoutError on timeout.
       DeviceUnreachableError on missing device.
     """
-    return self._GetPidsImpl(process_name)
-
-  def _GetPidsImpl(self, process_name):
     procs_pids = {}
-    for line in self.RunShellCommand('ps', check_return=True):
+    for line in self.RunShellCommand('ps', check_return=True,
+                                     large_output=True):

[perezju, 2015/04/13 09:58:32]

This is precisely one of the things I really think we shouldn't do.

I think that, instead of large_output=True, we should change the command to 'ps
| grep -F {{process_name}}' instead.

[jbudorick, 2015/04/13 17:44:29]

Done.

       try:
         ps_data = line.split()
         if process_name in ps_data[-1]:
           procs_pids[ps_data[-1]] = ps_data[1]
       except IndexError:
         pass
     return procs_pids
 
   @decorators.WithTimeoutAndRetriesFromInstance()
   def TakeScreenshot(self, host_path=None, timeout=None, retries=None):
@@ skipping 52 matching lines @@
       logging.exception('Error getting memory usage from status')
 
     return result
 
   def _GetMemoryUsageForPidFromSmaps(self, pid):
     SMAPS_COLUMNS = (
         'Size', 'Rss', 'Pss', 'Shared_Clean', 'Shared_Dirty', 'Private_Clean',
         'Private_Dirty')
 
     showmap_out = self.RunShellCommand(
-        ['showmap', str(pid)], as_root=True, check_return=True)
+        ['showmap', str(pid)], as_root=True, check_return=True,
+        large_output=True)

[perezju, 2015/04/13 09:58:32]

How about: 'showmap {{pid}} | grep TOTAL'

[jbudorick, 2015/04/13 17:44:29]

Done.

     if not showmap_out:
       raise device_errors.CommandFailedError('No output from showmap')
 
     split_totals = showmap_out[-1].split()
     if (not split_totals
         or len(split_totals) != 9
         or split_totals[-1] != 'TOTAL'):
       raise device_errors.CommandFailedError(
           'Invalid output from showmap: %s' % '\n'.join(showmap_out))
 
@@ skipping 87 matching lines @@
     """Returns client cache."""
     if client_name not in self._client_caches:
       self._client_caches[client_name] = {}
     return self._client_caches[client_name]
 
   def _ClearCache(self):
     """Clears all caches."""
     for client in self._client_caches:
       self._client_caches[client].clear()
     self._cache.clear()