Cache device apk checksums in device_utils.py

build/android/pylib/device/device_utils.py

Index: build/android/pylib/device/device_utils.py
diff --git a/build/android/pylib/device/device_utils.py b/build/android/pylib/device/device_utils.py
index f201ef36b8bf04fa20881109d11c6225f8322bfb..b9fed1c0b29ab66a6d10855111cbc2b69202272b 100644
--- a/build/android/pylib/device/device_utils.py
+++ b/build/android/pylib/device/device_utils.py
@@ -176,6 +176,8 @@ class DeviceUtils(object):
     self._default_retries = default_retries
     self._cache = {}
     self._client_caches = {}
+    self._package_to_device_apk_checksums_cache = {}

[jbudorick, 2015/07/16 16:31:33]

This should use self._cache, which we already have some lifetime control for,
rather than using its own dict -- something like
self._cache['package_checksums'] = {} and self._cache['package_paths'] = {}

[agrieve, 2015/07/16 19:01:20]

I think these caches are better split out since their freshness are tied to
package installs rather than the device itself.

+    self._package_to_device_apk_paths_cache = {}
     assert hasattr(self, decorators.DEFAULT_TIMEOUT_ATTR)
     assert hasattr(self, decorators.DEFAULT_RETRIES_ATTR)
 
@@ -343,7 +345,8 @@ class DeviceUtils(object):
     return value
 
   @decorators.WithTimeoutAndRetriesFromInstance()
-  def GetApplicationPaths(self, package, timeout=None, retries=None):
+  def GetApplicationPaths(self, package, timeout=None, retries=None,
+                          skip_cache=False):

[jbudorick, 2015/07/16 16:31:33]

Do we envision external clients using this, or does this option exist solely for
the pm_ready check? If the latter, perhaps we should expose this option in a
private method that gets called by the public one.

If this sticks around, it needs to be:
 - before timeout and retries
 - documented

[agrieve, 2015/07/16 19:01:21]

Made it an internal helper. 

     """Get the paths of the installed apks on the device for the given package.
 
     Args:
@@ -352,6 +355,9 @@ class DeviceUtils(object):
     Returns:
       List of paths to the apks on the device for the given package.
     """
+    cached_result = self._package_to_device_apk_paths_cache.get(package)
+    if cached_result is not None and not skip_cache:
+      return list(cached_result)
     # 'pm path' is liable to incorrectly exit with a nonzero number starting
     # in Lollipop.
     # TODO(jbudorick): Check if this is fixed as new Android versions are
@@ -366,6 +372,7 @@ class DeviceUtils(object):
         raise device_errors.CommandFailedError(
             'pm path returned: %r' % '\n'.join(output), str(self))
       apks.append(line[len('package:'):])
+    self._package_to_device_apk_paths_cache[package] = list(apks)
     return apks
 
   @decorators.WithTimeoutAndRetriesFromInstance()
@@ -418,7 +425,7 @@ class DeviceUtils(object):
 
     def pm_ready():
       try:
-        return self.GetApplicationPaths('android')
+        return self.GetApplicationPaths('android', skip_cache=True)
       except device_errors.CommandFailedError:
         return False
 
@@ -494,15 +501,24 @@ class DeviceUtils(object):
         logging.warning(
             'Installing single APK (%s) when split APKs (%s) are currently '
             'installed.', apk_path, ' '.join(device_paths))
-      (files_to_push, _) = self._GetChangedAndStaleFiles(
-          apk_path, device_paths[0])
-      should_install = bool(files_to_push)
+      apks_to_install, host_checksums = (
+          self._ComputeStaleApks(package_name, [apk_path]))
+      should_install = bool(apks_to_install)
       if should_install and not reinstall:
-        self.adb.Uninstall(package_name)
+        self._Uninstall(package_name)
     else:
       should_install = True
+      host_checksums = None
     if should_install:
-      self.adb.Install(apk_path, reinstall=reinstall)
+      # We won't know the resulting device apk names.
+      self._package_to_device_apk_paths_cache.pop(package_name, 0)
+      try:
+        self.adb.Install(apk_path, reinstall=reinstall)
+        self._package_to_device_apk_checksums_cache[package_name] = (
+            host_checksums)
+      except:
+        self._package_to_device_apk_paths_cache.pop(package_name, 0)
+        raise
 
   @decorators.WithTimeoutAndRetriesDefaults(
       INSTALL_DEFAULT_TIMEOUT,
@@ -535,20 +551,39 @@ class DeviceUtils(object):
 
     if device_apk_paths:
       partial_install_package = package_name
-      device_checksums = md5sum.CalculateDeviceMd5Sums(device_apk_paths, self)
-      host_checksums = md5sum.CalculateHostMd5Sums(all_apks)
-      apks_to_install = [k for (k, v) in host_checksums.iteritems()
-                         if v not in device_checksums.values()]
+      apks_to_install, host_checksums = (
+          self._ComputeStaleApks(package_name, all_apks))
       if apks_to_install and not reinstall:
-        self.adb.Uninstall(package_name)
+        self._Uninstall(package_name)
         partial_install_package = None
         apks_to_install = all_apks
     else:
       partial_install_package = None
       apks_to_install = all_apks
+      host_checksums = None
     if apks_to_install:
-      self.adb.InstallMultiple(
-          apks_to_install, partial=partial_install_package, reinstall=reinstall)
+      # We won't know the resulting device apk names.
+      self._package_to_device_apk_paths_cache.pop(package_name, 0)
+      try:
+        self.adb.InstallMultiple(
+            apks_to_install, partial=partial_install_package,
+            reinstall=reinstall)
+        self._package_to_device_apk_checksums_cache[package_name] = (
+            host_checksums)
+      except:
+        self._package_to_device_apk_paths_cache.pop(package_name, 0)
+        raise
+
+  def _Uninstall(self, package_name):

[jbudorick, 2015/07/16 16:31:32]

This should _not_ be named "Uninstall". As-is, it's liable to confuse a user who
looks in here and thinks "why is this private?" instead of "oh, uninstall is
provided by AdbWrapper"

[agrieve, 2015/07/16 19:01:21]

Deleted in favour of hooking self.adb.Uninstall

+    try:
+      self.adb.Uninstall(package_name)
+    except:
+      # Clear cache since we can't be sure of the state.
+      self._package_to_device_apk_paths_cache.pop(package_name, 0)
+      self._package_to_device_apk_checksums_cache.pop(package_name, 0)
+      raise
+    self._package_to_device_apk_paths_cache[package_name] = []
+    self._package_to_device_apk_checksums_cache[package_name] = set()
 
   def _CheckSdkLevel(self, required_sdk_level):
     """Raises an exception if the device does not have the required SDK level.
@@ -1017,6 +1052,22 @@ class DeviceUtils(object):
       to_delete = device_checksums.keys()
       return (to_push, to_delete)
 
+  def _ComputeDeviceChecksumsForApks(self, package_name):
+    ret = self._package_to_device_apk_checksums_cache.get(package_name)
+    if ret is None:
+      device_paths = self.GetApplicationPaths(package_name)
+      file_to_checksums = md5sum.CalculateDeviceMd5Sums(device_paths, self)
+      ret = set(file_to_checksums.values())
+      self._package_to_device_apk_checksums_cache[package_name] = ret
+    return ret
+
+  def _ComputeStaleApks(self, package_name, host_apk_paths):
+    host_checksums = md5sum.CalculateHostMd5Sums(host_apk_paths)
+    device_checksums = self._ComputeDeviceChecksumsForApks(package_name)
+    stale_apks = [k for (k, v) in host_checksums.iteritems()
+                  if v not in device_checksums]
+    return stale_apks, set(host_checksums.values())
+
   def _PushFilesImpl(self, host_device_tuples, files):
     size = sum(host_utils.GetRecursiveDiskUsage(h) for h, _ in files)
     file_count = len(files)
@@ -1712,6 +1763,8 @@ class DeviceUtils(object):
     for client in self._client_caches:
       self._client_caches[client].clear()
     self._cache.clear()
+    self._package_to_device_apk_paths_cache.clear()
+    self._package_to_device_apk_checksums_cache.clear()
 
   @classmethod
   def parallel(cls, devices=None, async=False):