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..71de5886a39dc19593cb37141e0d7bfb0b4ab070 100755
--- a/tools/binary_size/diagnose_apk_bloat.py
+++ b/tools/binary_size/diagnose_apk_bloat.py
@@ -9,18 +9,121 @@ Run diagnose_apk_bloat.py -h for detailed usage help.
 """
 
 import argparse
-import logging
+import collections
+import itertools
+import json
 import multiprocessing
 import os
 import shutil
 import subprocess
 import sys
 
-import helpers
-
-_DEFAULT_OUT_DIR = os.path.join(helpers.SRC_ROOT, 'out', 'diagnose-apk-bloat')
+_SRC_ROOT = os.path.abspath(
+    os.path.join(os.path.dirname(__file__), os.pardir, os.pardir))
+_DEFAULT_OUT_DIR = os.path.join(_SRC_ROOT, 'out', 'diagnose-apk-bloat')
 _DEFAULT_TARGET = 'monochrome_public_apk'
-_DEFAULT_ARCHIVE_DIR = os.path.join(helpers.SRC_ROOT, 'binary-size-bloat')
+_DEFAULT_ARCHIVE_DIR = os.path.join(_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.name = name
+    self.banner = '\n' + '*' * 30 + name + '*' * 30
+    self.RunDiff()
+
+  def PrintResults(self, output_file):
+    with open(output_file, 'w+') as logfile:
+      _PrintAndWriteToFile(logfile, self.banner)
+      _PrintAndWriteToFile(logfile, 'Summary:')
+      _PrintAndWriteToFile(logfile, self.Summary())
+      _PrintAndWriteToFile(logfile, '\nDetails:')
+      for l in self.DetailedResults():
+        _PrintAndWriteToFile(logfile, l)
+
+  def Summary(self):
+    """A short description that summarizes the source of binary size bloat."""
+    raise NotImplementedError()
+
+  def DetailedResults(self):
+    """An iterable description of the cause of binary size bloat."""
+    raise NotImplementedError()
+
+  def ProduceDiff(self):
+    """Prepare a binary size diff with ready to print results."""
+    raise NotImplementedError()
+
+  def RunDiff(self):
+    _Print('Creating {}', self.name)
+    self.ProduceDiff()
+
+
+_ResourceSizesDiffResult = collections.namedtuple(
+    'ResourceSizesDiffResult', ['section', 'value', 'units'])
+
+
+class ResourceSizesDiff(BaseDiff):
+  _RESOURCE_SIZES_PATH = os.path.join(
+      _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
+    self._diff = None  # Set by |ProduceDiff()|
+    super(ResourceSizesDiff, self).__init__('Resource Sizes Diff')
+
+  def DetailedResults(self):
+    for section, value, units in self._diff:
+      yield '{:>+10,} {} {}'.format(value, units, section)
+
+  def Summary(self):
+    for s in self._diff:
+      if 'normalized' in s.section:
+        return 'Normalized APK size: {:+,} {}'.format(s.value, s.units)
+    return ''
+
+  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():
+        # Ignore entries when resource_sizes.py chartjson format has changed.
+        if (section not in without_patch or
+            subsection not in without_patch[section] or
+            v['units'] != without_patch[section][subsection]['units']):
+          _Print('Found differing dict structures for resource_sizes.py, '
+                 'skipping {} {}', section, subsection)
+        else:
+          diff.append(
+              _ResourceSizesDiffResult(
+                  '%s %s' % (section, subsection),
+                  v['value'] - without_patch[section][subsection]['value'],
+                  v['units']))
+    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):
@@ -62,26 +165,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,89 +192,112 @@ 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 ''
 
 
 def _GitCmd(args):
-  return _RunCmd(['git', '-C', helpers.SRC_ROOT] + args)
+  return _RunCmd(['git', '-C', _SRC_ROOT] + args)
 
 
 def _GclientSyncCmd(rev):
   cwd = os.getcwd()
-  os.chdir(helpers.SRC_ROOT)
-  logging.info('gclient sync to %s', rev)
+  os.chdir(_SRC_ROOT)
   _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')
+  _RunCmd(['tools/binary_size/supersize', 'archive', '--output-directory',

[agrieve, 2017/04/12 00:20:50]

I think you need to os.path.join this with _SRC_ROOT

Also - I know the parser doesn't care, but it's much more visually appealing to
put size_path before the --hyphen-flags

[estevenson, 2017/04/12 01:39:23]

Done.

+           build_helper.output_directory, '--elf-file', lib_path, size_path])
 
+  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 _NormalizeRev(rev):
+  """Use actual revs instead of HEAD, HEAD^, etc."""
+  return _GitCmd(['rev-parse', rev])
 
 
 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)
+
+
+def _PrintAndWriteToFile(logfile, s):
+  """Print |s| to |logfile| and stdout."""
+  _Print(s)
+  logfile.write('%s\n' % s)
 
 
 def main():
@@ -188,6 +313,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('--include-slow-options',
+                      action='store_true',
+                      help='Run some extra steps that take longer to complete. '
+                      'This includes apk-patch-size estimation and '
+                      'static-initializer counting')
 
   build_group = parser.add_argument_group('ninja', 'Args to use with ninja/gn')
   build_group.add_argument('-j',
@@ -215,13 +345,30 @@ 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 = [args.rev_with_patch,
+          args.rev_without_patch or args.rev_with_patch + '^']
+  revs = [_NormalizeRev(r) for r in revs]
   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' % (len(revs) - i, rev))
+                  for i, rev in enumerate(revs)]
+  _SyncAndBuild(revs, archive_dirs, build_helper)
+  _RestoreInitialBranch()
+
+  output_file = os.path.join(args.archive_dir, 'diff_result.txt')
+  if os.path.exists(output_file):

[agrieve, 2017/04/12 00:20:50]

nit: probably unnecessary to explicitly delete this (instead of just letting it
be overwritten). Unless the thinking is that it might be harmful to have an old
one lying around? In which case you should add a comment.

Actually, would it be better to just error out if the file already exists? I
know when I've been using this script, I "mv" the output directory after running
it so that I start fresh each time.

[estevenson, 2017/04/12 01:39:23]

Oops, what I meant to do here was delete it if it exists and use output_file in
append mode to make it easy to use the same file for each type of diff.

I changed it do this and changed the name to be
diff_result_revwithpatch_revwithoutpatch so that you can keep multiple diffs and
actually know which one is which. Another thing we could do (as a follow up)
would be to record the GN args used and avoid doing unnecessary builds.

Let me know if you still think the error out is better and I'll change it.

+    os.remove(output_file)
+  diffs = []
+  if build_helper.target_os == 'android':
+    diffs +=  [
+        ResourceSizesDiff(archive_dirs, _ApkNameFromTarget(args.target),
+                          slow_options=args.include_slow_options)
+    ]
+  for d in diffs:
+    d.PrintResults(output_file)
 
 if __name__ == '__main__':
   sys.exit(main())