Add resource_sizes diffs to diagnose_apk_bloat.py.

tools/binary_size/diagnose_apk_bloat.py

Index: tools/binary_size/diagnose_apk_bloat.py
diff --git a/tools/binary_size/diagnose_apk_bloat.py b/tools/binary_size/diagnose_apk_bloat.py
index 847c3059d3327b2dcf083e83bd767df19cb83aa4..58aeba9bed28e2a5a0a27aae3dd1843077fc50c2 100755
--- a/tools/binary_size/diagnose_apk_bloat.py
+++ b/tools/binary_size/diagnose_apk_bloat.py
@@ -9,7 +9,10 @@ Run diagnose_apk_bloat.py -h for detailed usage help.
 """
 
 import argparse
-import logging
+import collections
+from contextlib import contextmanager
+import itertools
+import json
 import multiprocessing
 import os
 import shutil
@@ -17,11 +20,98 @@ import subprocess
 import sys
 
 import helpers
+import map2size
 
 _DEFAULT_OUT_DIR = os.path.join(helpers.SRC_ROOT, 'out', 'diagnose-apk-bloat')
 _DEFAULT_TARGET = 'monochrome_public_apk'
 _DEFAULT_ARCHIVE_DIR = os.path.join(helpers.SRC_ROOT, 'binary-size-bloat')
 
+# Global variable for storing the initial branch before the script was launched
+# so that it doesn't need to be passed everywhere in case we fail and exit.
+_initial_branch = None
+
+
+class BaseDiff(object):
+  """Base class capturing binary size diffs."""
+  def __init__(self, name):
+    self.banner = '\n' + '*' * 30 + name + '*' * 30
+    self.ProduceDiff()
+
+  def PrintResults(self, output_file=None):
+    with _MaybeStdoutToFile(output_file):
+      _Print(self.banner)
+      _Print('Summary:')
+      self.PrintSummary()
+      _Print('\nDetails:')
+      self.PrintDetailedResults()
+
+  def PrintSummary(self):
+    """A short description that summarizes the source of binary size bloat."""
+    raise NotImplementedError()
+
+  def PrintDetailedResults(self):
+    """A detailed description about the cause of binary size bloat."""
+    raise NotImplementedError()
+
+  def ProduceDiff(self):
+    """Prepare a binary size diff with ready to print results."""
+    raise NotImplementedError()
+
+
+_ResourceSizesDiffResult = collections.namedtuple(
+    'ResourceSizesDiffResult', ['section', 'value', 'units'])
+
+
+class ResourceSizesDiff(BaseDiff):
+  _RESOURCE_SIZES_PATH = os.path.join(
+      helpers.SRC_ROOT, 'build', 'android', 'resource_sizes.py')
+
+  def __init__(self, archive_dirs, apk_name, slow_options=False):
+    self._archive_dirs = archive_dirs
+    self._apk_name = apk_name
+    self._slow_options = slow_options
+    super(ResourceSizesDiff, self).__init__('Resource Sizes Diff')
+
+  def PrintDetailedResults(self):
+    for section, value, units in self._diff:
+      _Print('{:>+10,} {} {}', value, units, section)
+
+  def PrintSummary(self):
+    for s in self._diff:
+      if 'normalized' in s.section:
+        _Print('Normalized APK size: {:+,} {}', s.value, s.units)
+
+  def ProduceDiff(self):
+    chartjsons = self._RunResourceSizes()
+    diff = []
+    with_patch = chartjsons[0]['charts']
+    without_patch = chartjsons[1]['charts']
+    for section, section_dict in with_patch.iteritems():
+      for subsection, v in section_dict.iteritems():
+        diff.append(
+            _ResourceSizesDiffResult(
+                '%s %s' % (section, subsection),
+                v['value'] - without_patch[section][subsection]['value'],
+                v['units']))

[agrieve, 2017/04/11 14:10:32]

nit: Let's add logic to have it fail gracefully if the same unit is not present
in both. E.g. we might change what's measured over time and still want the
ability to diff.

[estevenson, 2017/04/11 21:53:16]

Done.

+    self._diff = sorted(diff, key=lambda x: abs(x.value), reverse=True)
+
+  def _RunResourceSizes(self):
+    chartjsons = []
+    for archive_dir in self._archive_dirs:
+      apk_path = os.path.join(archive_dir, self._apk_name)
+      chartjson_file = os.path.join(archive_dir, 'results-chart.json')
+      cmd = [self._RESOURCE_SIZES_PATH, '--output-dir', archive_dir,
+             '--no-output-dir',
+             '--chartjson', apk_path]
+      if self._slow_options:
+        cmd += ['--estimate-patch-size']
+      else:
+        cmd += ['--no-static-initializer-check']
+      _RunCmd(cmd)
+      with open(chartjson_file) as f:
+        chartjsons.append(json.load(f))
+    return chartjsons
+
 
 class _BuildHelper(object):
   """Helper class for generating and building targets."""
@@ -62,26 +152,25 @@ class _BuildHelper(object):
     return cmd
 
   def Build(self):
-    logging.info('Building %s. This may take a while (run with -vv for '
-                 'detailed ninja output).', self.target)
+    _Print('Building: {}.', self.target)
     _RunCmd(self._GenGnCmd())
     _RunCmd(self._GenNinjaCmd(), print_stdout=True)
 
 
-def _GetLinkerMapPath(target_os, target):
+def _GetMainLibPath(target_os, target):
   # TODO(estevenson): Get this from GN instead of hardcoding.
   if target_os == 'linux':
-    return 'chrome.map.gz'
+    return 'chrome'
   elif 'monochrome' in target:
-    return 'lib.unstripped/libmonochrome.so.map.gz'
+    return 'lib.unstripped/libmonochrome.so'
   else:
-    return 'lib.unstripped/libchrome.so.map.gz'
+    return 'lib.unstripped/libchrome.so'
 
 
-def _ApkPathFromTarget(target):
+def _ApkNameFromTarget(target):
   # Only works on apk targets that follow: my_great_apk naming convention.
   apk_name = ''.join(s.title() for s in target.split('_')[:-1]) + '.apk'
-  return os.path.join('apks', apk_name)
+  return apk_name.replace('Webview', 'WebView')
 
 
 def _RunCmd(cmd, print_stdout=False):
@@ -90,27 +179,23 @@ def _RunCmd(cmd, print_stdout=False):
   Args:
     cmd: the command to run.
     print_stdout: if this is True, then the stdout of the process will be
-        printed (to stdout if log level is DEBUG otherwise to /dev/null).
-        If false, stdout will be returned.
+        printed, otherwise stdout will be returned.
 
   Returns:
     Command stdout if |print_stdout| is False otherwise ''.
   """
   cmd_str = ' '.join(c for c in cmd)
-  logging.debug('Running: %s', cmd_str)
-  if not print_stdout:
-    proc_stdout = subprocess.PIPE
-  elif logging.getLogger().isEnabledFor(logging.DEBUG):
+  _Print('Running: {}', cmd_str)
+  if print_stdout:
     proc_stdout = sys.stdout
   else:
-    proc_stdout = open(os.devnull, 'wb')
+    proc_stdout = subprocess.PIPE
 
   proc = subprocess.Popen(cmd, stdout=proc_stdout, stderr=subprocess.PIPE)
   stdout, stderr = proc.communicate()
 
   if proc.returncode != 0:
-    logging.error('Command failed: %s\nstderr:\n%s' % (cmd_str, stderr))
-    sys.exit(1)
+    _Die('command failed: {}\nstderr:\n{}', cmd_str, stderr)
 
   return stdout.strip() if stdout else ''
 
@@ -122,57 +207,92 @@ def _GitCmd(args):
 def _GclientSyncCmd(rev):
   cwd = os.getcwd()
   os.chdir(helpers.SRC_ROOT)
-  logging.info('gclient sync to %s', rev)
   _RunCmd(['gclient', 'sync', '-r', 'src@' + rev], print_stdout=True)
   os.chdir(cwd)
 
 
 def _ArchiveBuildResult(archive_dir, build_helper):
-  """Save resulting APK and mapping file."""
-  def ArchiveFile(file_path):
-    file_path = os.path.join(build_helper.output_directory, file_path)
-    if os.path.exists(file_path):
-      if not os.path.exists(archive_dir):
-        os.makedirs(archive_dir)
-      shutil.copy(file_path, archive_dir)
-    else:
-      logging.error('Expected file: %s not found.' % file_path)
-      sys.exit(1)
-
-  logging.info('Saving build results to: %s', archive_dir)
-  ArchiveFile(_GetLinkerMapPath(build_helper.target_os, build_helper.target))
-  if build_helper.target_os == 'android':
-    ArchiveFile(_ApkPathFromTarget(build_helper.target))
+  """Save build artifacts necessary for diffing."""
+  _Print('Saving build results to: {}', archive_dir)
+  if not os.path.exists(archive_dir):
+    os.makedirs(archive_dir)
+
+  def ArchiveFile(filename):
+    if not os.path.exists(filename):
+      _Die('missing expected file: {}', filename)
+    shutil.copy(filename, archive_dir)
+
+  lib_path = os.path.join(
+      build_helper.output_directory,
+      _GetMainLibPath(build_helper.target_os, build_helper.target))
+  ArchiveFile(lib_path)
+
+  size_path = os.path.join(
+      archive_dir, os.path.splitext(os.path.basename(lib_path))[0] + '.size')
+  map2size.main(['tools/binary_size/map2size.py', '--output-directory',

[agrieve, 2017/04/11 14:10:32]

Probably better to use subprocess for this since it's so RAM hungry and uses
atexit

[estevenson, 2017/04/11 21:53:16]

Done.

+                 build_helper.output_directory, lib_path, size_path, '-v'])
 
+  if build_helper.target_os == 'android':
+    apk_path = os.path.join(build_helper.output_directory, 'apks',
+                            _ApkNameFromTarget(build_helper.target))
+    ArchiveFile(apk_path)
 
-def _SyncAndBuild(rev_with_patch, rev_without_patch, archive_dir, build_helper):
-  rev_with_patch = _GitCmd(['rev-parse', rev_with_patch])
-  rev_without_patch = _GitCmd([
-      'rev-parse', rev_without_patch or rev_with_patch + '^'])
 
+def _SyncAndBuild(revs, archive_dirs, build_helper):
   # Move to a detached state since gclient sync doesn't work with local commits
   # on a branch.
   _GitCmd(['checkout', '--detach'])
+  for rev, archive_dir in itertools.izip(revs, archive_dirs):
+    _GclientSyncCmd(rev)
+    build_helper.Build()
+    _ArchiveBuildResult(archive_dir, build_helper)
 
-  _GclientSyncCmd(rev_with_patch)
-  build_helper.Build()
-  _ArchiveBuildResult(
-      os.path.join(archive_dir, 'with_patch_%s' % rev_with_patch), build_helper)
 
-  _GclientSyncCmd(rev_without_patch)
-  build_helper.Build()
-  _ArchiveBuildResult(
-      os.path.join(archive_dir, 'without_patch_%s' % rev_without_patch),
-      build_helper)
+def _NormalizeRevs(*revs):

[agrieve, 2017/04/11 14:10:32]

nit: This interface seems a bit awkward to me. How about calling it
"_NormalizeRev" and call it twice.

[estevenson, 2017/04/11 21:53:16]

Done. Still left revs as a list (instead of a rev_without and rev_with) since
this will be required for https://crbug.com/710076 anyways.

+  """Use actual revs instead of HEAD, HEAD^, etc."""
+  return [_GitCmd(['rev-parse', r]) for r in revs]
 
 
 def _EnsureDirectoryClean():
-  logging.info('Checking source directory')
+  _Print('Checking source directory')
   stdout = _GitCmd(['status', '--porcelain'])
   # Ignore untracked files.
   if stdout and stdout[:2] != '??':
-    logging.error('Failure: please ensure working directory is clean.')
-    sys.exit(1)
+    _Die('please ensure working directory is clean.')
+
+
+def _SetInitialBranch():
+  global _initial_branch
+  _initial_branch = _GitCmd(['rev-parse', '--abbrev-ref', 'HEAD'])
+
+
+def _RestoreInitialBranch():
+  if _initial_branch:
+    _GitCmd(['checkout', _initial_branch])
+
+
+def _Die(s, *args, **kwargs):
+  _Print('Failure: ' + s, *args, **kwargs)
+  _RestoreInitialBranch()
+  sys.exit(1)
+
+
+def _Print(s, *args, **kwargs):
+  print s.format(*args, **kwargs)
+
+
+@contextmanager
+def _MaybeStdoutToFile(filename):
+  """Print to |filename| instead of stdout."""
+  if not filename:
+    yield
+  else:
+    with open(filename, 'w+') as f:
+      sys.stdout, sys_stdout = f, sys.stdout
+      try:
+        yield
+      finally:
+        sys.stdout = sys_stdout
 
 
 def main():
@@ -188,6 +308,11 @@ def main():
   parser.add_argument('--rev-without-patch',
                       help='Older patch to diff against. If not supplied, '
                       'the previous commit to rev_with_patch will be used.')
+  parser.add_argument('--save-results',

[agrieve, 2017/04/11 14:10:32]

Rather than adding another option, I think it would be better to always save the
diff to a file, and then just cat the file to print it in the logs.

[estevenson, 2017/04/11 21:53:16]

Done.

+                      help='Save diff to this file instead of printing.')
+  parser.add_argument('--include-slow-options',
+                      action='store_true',
+                      help='Run some extra steps that take longer to complete.')

[agrieve, 2017/04/11 14:10:32]

nit: Should say exactly which options this enables here so that devs can reason
about whether it's worth it for them.

[estevenson, 2017/04/11 21:53:16]

Done.

 
   build_group = parser.add_argument_group('ninja', 'Args to use with ninja/gn')
   build_group.add_argument('-j',
@@ -215,13 +340,24 @@ def main():
   build_group.add_argument('--target',
                            default=_DEFAULT_TARGET,
                            help='GN APK target to build.')
-  args = helpers.AddCommonOptionsAndParseArgs(parser, sys.argv, pypy_warn=False)
+  args = parser.parse_args()
 
   _EnsureDirectoryClean()
+  _SetInitialBranch()
+  revs = _NormalizeRevs(args.rev_with_patch,
+                        args.rev_without_patch or args.rev_with_patch + '^')
   build_helper = _BuildHelper(args)
-  _SyncAndBuild(args.rev_with_patch, args.rev_without_patch, args.archive_dir,
-                build_helper)
-
+  archive_dirs = [os.path.join(args.archive_dir, '%d-%s' % (i + 1, rev))

[agrieve, 2017/04/11 14:10:32]

nit: might be better as len(revs) - i, so that the larger number means latter in
time.

[estevenson, 2017/04/11 21:53:16]

Done.

+                  for i, rev in enumerate(revs)]
+  _SyncAndBuild(revs, archive_dirs, build_helper)
+  _RestoreInitialBranch()
+
+  diffs = [
+    ResourceSizesDiff(archive_dirs, _ApkNameFromTarget(args.target),
+                      slow_options=args.include_slow_options),
+  ]
+  for d in diffs:
+    d.PrintResults(output_file=args.save_results)
 
 if __name__ == '__main__':
   sys.exit(main())