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

build/android/pylib/android_commands.py

Index: build/android/pylib/android_commands.py
diff --git a/build/android/pylib/android_commands.py b/build/android/pylib/android_commands.py
index 8d8949cac74c8db343ff68b5c9a33da11d462bf8..fe9f7533d4db36cf5f90c67c6a04c2b79c127365 100644
--- a/build/android/pylib/android_commands.py
+++ b/build/android/pylib/android_commands.py
@@ -253,6 +253,7 @@ class AndroidCommands(object):
     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."""
@@ -917,6 +918,14 @@ class AndroidCommands(object):
     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])
 
@@ -941,6 +950,7 @@ class AndroidCommands(object):
       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