Add resource_sizes diffs to diagnose_apk_bloat.py.

tools/binary_size/diagnose_apk_bloat.py

 #!/usr/bin/env python
 # Copyright 2017 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Tool for finding the cause of APK bloat.
 
 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
 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."""
 
   def __init__(self, args):
     self.enable_chrome_android_internal = args.enable_chrome_android_internal
     self.max_jobs = args.max_jobs
     self.max_load_average = args.max_load_average
     self.output_directory = args.output_directory
     self.target = args.target
@@ skipping 20 matching lines @@
     return ['gn', 'gen', self.output_directory, '--args=%s' % gn_args]
 
   def _GenNinjaCmd(self):
     cmd = ['ninja', '-C', self.output_directory]
     cmd += ['-j', self.max_jobs] if self.max_jobs else []
     cmd += ['-l', self.max_load_average] if self.max_load_average else []
     cmd += [self.target]
     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):
   """Convenience function for running commands.
 
   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)
 
 
 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)
+  """Save build artifacts necessary for diffing."""
+  _Print('Saving build results to: {}', archive_dir)
+  if not os.path.exists(archive_dir):
+    os.makedirs(archive_dir)
 
-  logging.info('Saving build results to: %s', archive_dir)
-  ArchiveFile(_GetLinkerMapPath(build_helper.target_os, build_helper.target))
+  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':
-    ArchiveFile(_ApkPathFromTarget(build_helper.target))
+    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():
   parser = argparse.ArgumentParser(
       description='Find the cause of APK size bloat.',
       formatter_class=argparse.ArgumentDefaultsHelpFormatter)
   parser.add_argument('--archive-dir',
                       default=_DEFAULT_ARCHIVE_DIR,
                       help='Where results are stored.')
   parser.add_argument('--rev-with-patch',
                       default='HEAD',
                       help='Commit with patch.')
   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',
                            dest='max_jobs',
                            help='Run N jobs in parallel.')
   build_group.add_argument('-l',
                            dest='max_load_average',
                            help='Do not start new jobs if the load average is '
                            'greater than N.')
   build_group.add_argument('--no-goma',
                            action='store_false',
                            dest='use_goma',
                            default=True,
                            help='Use goma when building with ninja.')
   build_group.add_argument('--target-os',
                            default='android',
                            choices=['android', 'linux'],
                            help='target_os gn arg.')
   build_group.add_argument('--output-directory',
                            default=_DEFAULT_OUT_DIR,
                            help='ninja output directory.')
   build_group.add_argument('--enable_chrome_android_internal',
                            action='store_true',
                            help='Allow downstream targets to be built.')
   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())