Add instance-level in-memory cache for PushIfNeeded().

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.
 """
 
 import collections
@@ skipping 235 matching lines @@
     self._device = device
     self._logcat = None
     self.logcat_process = None
     self._logcat_tmpoutfile = None
     self._pushed_files = []
     self._device_utc_offset = None
     self._potential_push_size = 0
     self._actual_push_size = 0
     self._external_storage = ''
     self._util_wrapper = ''
+    self._push_if_needed_cache = {}
 
   def _LogShell(self, cmd):
     """Logs the adb shell command."""
     if self._device:
       device_repr = self._device[-4:]
     else:
       device_repr = '????'
     logging.info('[%s]> %s', device_repr, cmd)
 
   def Adb(self):
@@ skipping 644 matching lines @@
     """Pushes |host_path| to |device_path|.
 
     Works for files and directories. This method skips copying any paths in
     |test_data_paths| that already exist on the device with the same hash.
 
     All pushed files can be removed by calling RemovePushedFiles().
     """
     MAX_INDIVIDUAL_PUSHES = 50
     assert os.path.exists(host_path), 'Local path not found %s' % host_path
 
+    # See if the file on the host changed since the last push (if any) and
+    # return early if it didn't. Note that this shortcut assumes that the tests
+    # on the device don't modify the files.
+    if host_path in self._push_if_needed_cache:

[craigdh, 2013/12/02 17:09:36]

What about when |host_path| is a directory? Updating the mtime of a file doesn't
update the mtime of all parent directories.

Seems like you'd have to cache the mtimes of all files in the directory (and
subdirectories recursively) on line 953 and subsequently check them all here.

[Philippe, 2013/12/02 17:16:43]

Good catch :) I had forgotten that |host_path| could be a directory.

How about not caching directories instead (at least as a preliminary step)?

[craigdh, 2013/12/02 18:20:19]

It should be pretty easy to do with os.walk(). Do you mind trying and seeing if
the performance is reasonable?

[Philippe, 2013/12/03 10:31:31]

My main concern was precisely that I have no immediate way to measure this with
directories. I wrote this change to speedup the execution of the memory
measurements in Telemetry (since I'm working on a CL that causes a significant
slowdown partly due to this specific PushIfNeeded() issue). PushIfNeeded() is
only used repetitively in that context to push the |purge_ashmem| binary. So
this sounded slightly like premature optimization to me and wasn't worth the
complexity IMO given the few edge cases that would have to be handled correctly
(e.g. |host_path|, when it's a directory, has now less children than it had when
the initial push happened or |host_path| was initially a directory but it's now
a file). I would prefer simplicity and correctness until we have good data :)
Now if you feel strongly about this I'm also happy to implement it in a next CL
since I would like first to move forward on memory measurements.

What do you think? I would be happy to file a bug tracking those future
improvements (and possibly assign it to myself although you guys are more
familiar/involved than me with this layer).

+      host_path_mtime = self._push_if_needed_cache[host_path]
+      if host_path_mtime == os.stat(host_path).st_mtime:
+        return
+
     def GetHostSize(path):
       return int(cmd_helper.GetCmdOutput(['du', '-sb', path]).split()[0])
 
     size = GetHostSize(host_path)
     self._pushed_files.append(device_path)
     self._potential_push_size += size
 
     changed_files = self.GetFilesChanged(host_path, device_path)
     logging.info('Found %d files that need to be pushed to %s',
         len(changed_files), device_path)
     if not changed_files:
       return
 
     def Push(host, device):
       # NOTE: We can't use adb_interface.Push() because it hardcodes a timeout
       # of 60 seconds which isn't sufficient for a lot of users of this method.
       push_command = 'push %s %s' % (host, device)
       self._LogShell(push_command)
 
       # Retry push with increasing backoff if the device is busy.
       retry = 0
       while True:
         output = self._adb.SendCommand(push_command, timeout_time=30 * 60)
         if _HasAdbPushSucceeded(output):
+          self._push_if_needed_cache[host] = os.stat(host).st_mtime
           return
         if retry < 3:
           retry += 1
           wait_time = 5 * retry
           logging.error('Push failed, retrying in %d seconds: %s' %
                         (wait_time, output))
           time.sleep(wait_time)
         else:
           raise Exception('Push failed: %s' % output)
 
@@ skipping 759 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()