Android: improve static initializer counting in resource_sizes.py.

build/android/resource_sizes.py

Index: build/android/resource_sizes.py
diff --git a/build/android/resource_sizes.py b/build/android/resource_sizes.py
index 6f531f052d764884f80eab68525b518591fe8637..e0a733d76d98f11899523f2f8bc3fa2288f1c246 100755
--- a/build/android/resource_sizes.py
+++ b/build/android/resource_sizes.py
@@ -10,13 +10,13 @@
 """
 
 import collections
+from contextlib import contextmanager
 import json
 import logging
 import operator
 import optparse
 import os
 import re
-import shutil
 import struct
 import sys
 import tempfile
@@ -33,6 +33,8 @@ from pylib.constants import host_paths
 
 _AAPT_PATH = lazy.WeakConstant(lambda: build_tools.GetPath('aapt'))
 _GRIT_PATH = os.path.join(host_paths.DIR_SOURCE_ROOT, 'tools', 'grit')
+_BUILD_UTILS_PATH = os.path.join(
+    host_paths.DIR_SOURCE_ROOT, 'build', 'android', 'gyp')
 
 # Prepend the grit module from the source tree so it takes precedence over other
 # grit versions that might present in the search path.
@@ -42,6 +44,9 @@ with host_paths.SysPath(_GRIT_PATH, 1):
 with host_paths.SysPath(host_paths.BUILD_COMMON_PATH):
   import perf_tests_results_helper # pylint: disable=import-error
 
+with host_paths.SysPath(_BUILD_UTILS_PATH, 1):
+  from util import build_utils # pylint: disable=import-error
+
 
 # Python had a bug in zipinfo parsing that triggers on ChromeModern.apk
 # https://bugs.python.org/issue14315
@@ -115,26 +120,20 @@ _READELF_SIZES_METRICS = {
 
 
 def _ExtractMainLibSectionSizesFromApk(apk_path, main_lib_path):
-  tmpdir = tempfile.mkdtemp(suffix='_apk_extract')
-  grouped_section_sizes = collections.defaultdict(int)
-  try:
-    with zipfile.ZipFile(apk_path, 'r') as z:
-      extracted_lib_path = z.extract(main_lib_path, tmpdir)
-      section_sizes = _CreateSectionNameSizeMap(extracted_lib_path)
-
-      for group_name, section_names in _READELF_SIZES_METRICS.iteritems():
-        for section_name in section_names:
-          if section_name in section_sizes:
-            grouped_section_sizes[group_name] += section_sizes.pop(section_name)
+  with Unzip(apk_path, pattern=main_lib_path) as extracted_lib_path:
+    grouped_section_sizes = collections.defaultdict(int)
+    section_sizes = _CreateSectionNameSizeMap(extracted_lib_path)
+    for group_name, section_names in _READELF_SIZES_METRICS.iteritems():
+      for section_name in section_names:
+        if section_name in section_sizes:
+          grouped_section_sizes[group_name] += section_sizes.pop(section_name)
 
-      # Group any unknown section headers into the "other" group.
-      for section_header, section_size in section_sizes.iteritems():
-        print "Unknown elf section header:", section_header
-        grouped_section_sizes['other'] += section_size
+    # Group any unknown section headers into the "other" group.
+    for section_header, section_size in section_sizes.iteritems():
+      print "Unknown elf section header:", section_header
+      grouped_section_sizes['other'] += section_size
 
-      return grouped_section_sizes
-  finally:
-    shutil.rmtree(tmpdir)
+    return grouped_section_sizes
 
 
 def _CreateSectionNameSizeMap(so_path):
@@ -148,6 +147,13 @@ def _CreateSectionNameSizeMap(so_path):
   return section_sizes
 
 
+def _ParseLibBuildId(so_path):
+  """Returns the Build ID of the given native library."""
+  stdout = cmd_helper.GetCmdOutput(['readelf', '-n', so_path])
+  match = re.search(r'Build ID: (\w{40})', stdout)

[agrieve, 2017/02/24 20:04:21]

nit: BuildIDs don't need to be 40 characters (although they should be when using
sha1, like we do). It would be better to just take the entire line (since we're
just comparing, there's no need to even strip the prefix, although if you want
to keep that in that's fine)

[estevenson, 2017/02/28 00:58:45]

Left it in. Done.

+  return match.group(1) if match else None
+
+
 def CountStaticInitializers(so_path):
   # Static initializers expected in official builds. Note that this list is
   # built using 'nm' on libchrome.so which results from a GCC official build
@@ -595,7 +601,8 @@ def _PrintStaticInitializersCountFromApk(apk_filename, chartjson=None):
       lib_name = os.path.basename(zip_info.filename).replace('crazy.', '')
       unstripped_path = os.path.join(out_dir, 'lib.unstripped', lib_name)
       if os.path.exists(unstripped_path):
-        si_count += _PrintStaticInitializersCount(unstripped_path)
+        si_count += _PrintStaticInitializersCount(
+            apk_filename, zip_info.filename, unstripped_path)
       else:
         raise Exception('Unstripped .so not found. Looked here: %s',
                         unstripped_path)
@@ -603,21 +610,24 @@ def _PrintStaticInitializersCountFromApk(apk_filename, chartjson=None):
                    'count')
 
 
-def _PrintStaticInitializersCount(so_with_symbols_path):
+def _PrintStaticInitializersCount(apk_path, apk_so_name, so_with_symbols_path):
   """Counts the number of static initializers in the given shared library.
      Additionally, files for which static initializers were found are printed
      on the standard output.
 
      Args:
-       so_with_symbols_path: Path to the unstripped libchrome.so file.
-
+      apk_path: Path to the apk.
+      apk_so_name: Name of the so.
+      so_with_symbols_path: Path to the unstripped libchrome.so file.
      Returns:
        The number of static initializers found.
   """
   # GetStaticInitializers uses get-static-initializers.py to get a list of all
   # static initializers. This does not work on all archs (particularly arm).
   # TODO(rnephew): Get rid of warning when crbug.com/585588 is fixed.
-  si_count = CountStaticInitializers(so_with_symbols_path)
+  with Unzip(apk_path, pattern="*%s" % apk_so_name) as unzipped_so:
+    _VerifyLibBuildIdsMatch(unzipped_so, so_with_symbols_path)
+    si_count = CountStaticInitializers(unzipped_so)
   static_initializers = GetStaticInitializers(so_with_symbols_path)
   static_initializers_count = len(static_initializers) - 1  # Minus summary.
   if si_count != static_initializers_count:
@@ -666,6 +676,21 @@ def _PrintDexAnalysis(apk_filename, chartjson=None):
                    'bytes')
 
 
+@contextmanager
+def Unzip(zip_file, pattern=None, predicate=None):
+  """Utility for temporary use of a set of files in a zip archive."""
+  with build_utils.TempDir() as unzipped_dir:
+    unzipped_files = build_utils.ExtractAll(
+        zip_file, unzipped_dir, True, pattern=pattern, predicate=predicate)
+    yield unzipped_files[0] if len(unzipped_files) == 1 else unzipped_files
+
+
+def _VerifyLibBuildIdsMatch(*so_files):
+  if len(set(_ParseLibBuildId(f) for f in so_files)) > 1:
+    raise Exception('Found differing build ids in output directory and apk. '
+                    'Your output directory is likely stale.')
+
+
 def main(argv):
   usage = """Usage: %prog [options] file1 file2 ...
 
@@ -713,11 +738,6 @@ Pass any number of files to graph their sizes. Any files with the extension
   if not files:
     option_parser.error('Must specify a file')
 
-  if options.so_with_symbols_path:
-    si_count = _PrintStaticInitializersCount(options.so_with_symbols_path)
-    ReportPerfResult(chartjson, 'StaticInitializersCount', 'count', si_count,
-                     'count')
-
   PrintResourceSizes(files, chartjson=chartjson)
 
   for f in files:
@@ -726,7 +746,13 @@ Pass any number of files to graph their sizes. Any files with the extension
       _PrintDexAnalysis(f, chartjson=chartjson)
       if not options.no_output_dir:
         PrintPakAnalysis(f, options.min_pak_resource_size)
-        if not options.so_with_symbols_path:
+        so_path = options.so_with_symbols_path

[agrieve, 2017/02/24 20:04:21]

This actually makes less sense inside the loop. However, this flag doesn't even
look to be used anymore (with one exception), so we should just make it a no-op.

Exception is here:
https://codereview.chromium.org/2718743002/

[estevenson, 2017/02/28 00:58:45]

I removed obsolete args and changed the script to just take 1 apk as an
argument, was there a reason to allow more?

+        if so_path:
+          si_count = _PrintStaticInitializersCount(
+              f, os.path.basename(so_path), so_path)
+          ReportPerfResult(
+              chartjson, 'StaticInitializersCount', 'count', si_count, 'count')
+        else:
           _PrintStaticInitializersCountFromApk(f, chartjson=chartjson)
 
   if chartjson: