diagnose_apk_bloat.py: fix error messages and simplify rev order.

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 collections
 from contextlib import contextmanager
 import distutils.spawn
 import json
 import multiprocessing
 import os
 import re
 import shutil
 import subprocess
 import sys
 import tempfile
 import zipfile
 
 _COMMIT_COUNT_WARN_THRESHOLD = 15
 _ALLOWED_CONSECUTIVE_FAILURES = 2
 _DIFF_DETAILS_LINES_THRESHOLD = 100
 _BUILDER_URL = \
     'https://build.chromium.org/p/chromium.perf/builders/Android%20Builder'
-_CLOUD_OUT_DIR = os.path.join('out', 'Release')
 _SRC_ROOT = os.path.abspath(
     os.path.join(os.path.dirname(__file__), os.pardir, os.pardir))
 _DEFAULT_ARCHIVE_DIR = os.path.join(_SRC_ROOT, 'binary-size-bloat')
 _DEFAULT_OUT_DIR = os.path.join(_SRC_ROOT, 'out', 'diagnose-apk-bloat')
-_DEFAULT_TARGET = 'monochrome_public_apk'
-
+_DEFAULT_ANDROID_TARGET = 'monochrome_public_apk'
 
 _global_restore_checkout_func = None
 
 
 def _SetRestoreFunc(subrepo):
   branch = _GitCmd(['rev-parse', '--abbrev-ref', 'HEAD'], subrepo)
   global _global_restore_checkout_func
   _global_restore_checkout_func = lambda: _GitCmd(['checkout', branch], subrepo)
 
 
-_DiffResult = collections.namedtuple(
-    'DiffResult', ['name', 'value', 'units'])
+_DiffResult = collections.namedtuple('DiffResult', ['name', 'value', 'units'])
 
 
 class BaseDiff(object):
   """Base class capturing binary size diffs."""
   def __init__(self, name):
     self.name = name
     self.banner = '\n' + '*' * 30 + name + '*' * 30
 
   def AppendResults(self, logfile):
     """Print and write diff results to an open |logfile|."""
     _PrintAndWriteToFile(logfile, self.banner)
     _PrintAndWriteToFile(logfile, 'Summary:')
     _PrintAndWriteToFile(logfile, self.Summary())
     _PrintAndWriteToFile(logfile, '\nDetails:')
     _PrintAndWriteToFile(logfile, self.DetailedResults())
 
   @property
   def summary_stat(self):
     return None
 
   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, archive_dirs):
+  def ProduceDiff(self, before_dir, after_dir):
     """Prepare a binary size diff with ready to print results."""
     raise NotImplementedError()
 
-  def RunDiff(self, logfile, archive_dirs):
-    self.ProduceDiff(archive_dirs)
+  def RunDiff(self, logfile, before_dir, after_dir):
+    self.ProduceDiff(before_dir, after_dir)
     self.AppendResults(logfile)
 
 
 class NativeDiff(BaseDiff):
   _RE_SUMMARY = re.compile(
       r'.*(Section Sizes .*? object files added, \d+ removed).*',
       flags=re.DOTALL)
   _RE_SUMMARY_STAT = re.compile(
       r'Section Sizes \(Total=(?P<value>\d+) (?P<units>\w+)\)')
   _SUMMARY_STAT_NAME = 'Native Library Delta'
@@ skipping 11 matching lines @@
       return _DiffResult(
           NativeDiff._SUMMARY_STAT_NAME, m.group('value'), m.group('units'))
     return None
 
   def DetailedResults(self):
     return self._diff.splitlines()
 
   def Summary(self):
     return NativeDiff._RE_SUMMARY.match(self._diff).group(1)
 
-  def ProduceDiff(self, archive_dirs):
-    size_files = [os.path.join(a, self._size_name)
-                  for a in reversed(archive_dirs)]
-    cmd = [self._supersize_path, 'diff'] + size_files
+  def ProduceDiff(self, before_dir, after_dir):
+    before_size = os.path.join(before_dir, self._size_name)
+    after_size = os.path.join(after_dir, self._size_name)
+    cmd = [self._supersize_path, 'diff', before_size, after_size]
     self._diff = _RunCmd(cmd)[0].replace('{', '{{').replace('}', '}}')
 
 
 class ResourceSizesDiff(BaseDiff):
   _RESOURCE_SIZES_PATH = os.path.join(
       _SRC_ROOT, 'build', 'android', 'resource_sizes.py')
 
   def __init__(self, apk_name, slow_options=False):
     self._apk_name = apk_name
     self._slow_options = slow_options
     self._diff = None  # Set by |ProduceDiff()|
     super(ResourceSizesDiff, self).__init__('Resource Sizes Diff')
 
   @property
   def summary_stat(self):
     for s in self._diff:
       if 'normalized' in s.name:
         return s
     return None
 
   def DetailedResults(self):
     return ['{:>+10,} {} {}'.format(value, units, name)
             for name, value, units in self._diff]
 
   def Summary(self):
     return 'Normalized APK size: {:+,} {}'.format(
         self.summary_stat.value, self.summary_stat.units)
 
-  def ProduceDiff(self, archive_dirs):
-    chartjsons = self._RunResourceSizes(archive_dirs)
+  def ProduceDiff(self, before_dir, after_dir):
+    before = self._RunResourceSizes(before_dir)
+    after = self._RunResourceSizes(after_dir)
     diff = []
-    with_patch = chartjsons[0]['charts']
-    without_patch = chartjsons[1]['charts']
-    for section, section_dict in with_patch.iteritems():
+    for section, section_dict in after.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']):
+        if (section not in before or
+            subsection not in before[section] or
+            v['units'] != before[section][subsection]['units']):
           _Print('Found differing dict structures for resource_sizes.py, '
                  'skipping {} {}', section, subsection)
         else:
           diff.append(
               _DiffResult(
                   '%s %s' % (section, subsection),
-                  v['value'] - without_patch[section][subsection]['value'],
+                  v['value'] - before[section][subsection]['value'],
                   v['units']))
     self._diff = sorted(diff, key=lambda x: abs(x.value), reverse=True)
 
-  def _RunResourceSizes(self, archive_dirs):
-    chartjsons = []
-    for archive_dir in 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, apk_path,'--output-dir', archive_dir,
-             '--no-output-dir', '--chartjson']
-      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
+  def _RunResourceSizes(self, archive_dir):
+    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, apk_path,'--output-dir', archive_dir,
+           '--no-output-dir', '--chartjson']
+    if self._slow_options:
+      cmd += ['--estimate-patch-size']
+    else:
+      cmd += ['--no-static-initializer-check']
+    _RunCmd(cmd)
+    with open(chartjson_file) as f:
+      chartjson = json.load(f)
+    return chartjson['charts']
 
 
 class _BuildHelper(object):
   """Helper class for generating and building targets."""
   def __init__(self, args):
     self.cloud = args.cloud
     self.enable_chrome_android_internal = args.enable_chrome_android_internal
     self.extra_gn_args_str = ''
     self.max_jobs = args.max_jobs
     self.max_load_average = args.max_load_average
@@ skipping 25 matching lines @@
     elif 'monochrome' in self.target:
       return 'lib.unstripped/libmonochrome.so'
     else:
       return 'lib.unstripped/libchrome.so'
 
   @property
   def abs_main_lib_path(self):
     return os.path.join(self.output_directory, self.main_lib_path)
 
   @property
+  def download_bucket(self):
+    return 'gs://chrome-perf/%s Builder/' % self.target_os.title()
+
+  @property
+  def download_output_dir(self):
+    return 'out/Release' if self.IsAndroid() else 'full-build-linux'
+
+  @property
   def map_file_path(self):
     return self.main_lib_path + '.map.gz'
 
   @property
   def size_name(self):
     return os.path.splitext(os.path.basename(self.main_lib_path))[0] + '.size'
 
   def _SetDefaults(self):
     has_goma_dir = os.path.exists(os.path.join(os.path.expanduser('~'), 'goma'))
     self.use_goma = self.use_goma or has_goma_dir
     self.max_load_average = (self.max_load_average or
                              str(multiprocessing.cpu_count()))
     if not self.max_jobs:
       self.max_jobs = '10000' if self.use_goma else '500'
 
     if os.path.exists(os.path.join(os.path.dirname(_SRC_ROOT), 'src-internal')):
       self.extra_gn_args_str = ' is_chrome_branded=true'
     else:
       self.extra_gn_args_str = (' exclude_unwind_tables=true '
           'ffmpeg_branding="Chrome" proprietary_codecs=true')
+    self.target = self.target if self.IsAndroid() else 'chrome'
 
   def _GenGnCmd(self):
     gn_args = 'is_official_build=true symbol_level=1'
     gn_args += ' use_goma=%s' % str(self.use_goma).lower()
     gn_args += ' target_os="%s"' % self.target_os
     gn_args += (' enable_chrome_android_internal=%s' %
                 str(self.enable_chrome_android_internal).lower())
     gn_args += self.extra_gn_args_str
     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 Run(self):
     """Run GN gen/ninja build and return the process returncode."""
     _Print('Building: {}.', self.target)
     retcode = _RunCmd(
         self._GenGnCmd(), print_stdout=True, exit_on_failure=False)[1]
     if retcode:
       return retcode
     return _RunCmd(
         self._GenNinjaCmd(), print_stdout=True, exit_on_failure=False)[1]
 
+  def DownloadUrl(self, rev):
+    return self.download_bucket + 'full-build-linux_%s.zip' % rev
+
   def IsAndroid(self):
     return self.target_os == 'android'
 
   def IsLinux(self):
     return self.target_os == 'linux'
 
   def IsCloud(self):
     return self.cloud
 
 
@@ skipping 41 matching lines @@
     self.build = build
     self.build_archives = [_BuildArchive(rev, archive_dir, build, subrepo)
                            for rev in revs]
     self.diffs = diffs
     self.subrepo = subrepo
     self._summary_stats = []
 
   def IterArchives(self):
     return iter(self.build_archives)
 
-  def MaybeDiff(self, first_id, second_id):
+  def MaybeDiff(self, before_id, after_id):
     """Perform diffs given two build archives."""
-    archives = [
-        self.build_archives[first_id], self.build_archives[second_id]]
-    diff_path = self._DiffFilePath(archives)
-    if not self._CanDiff(archives):
+    before = self.build_archives[before_id]
+    after = self.build_archives[after_id]
+    diff_path = self._DiffFilePath(before, after)
+    if not self._CanDiff(before, after):
       _Print('Skipping diff for {} due to missing build archives.', diff_path)
       return
 
-    metadata_path = self._DiffMetadataPath(archives)
+    metadata_path = self._DiffMetadataPath(before, after)
     metadata = _GenerateMetadata(
-        archives, self.build, metadata_path, self.subrepo)
+        [before, after], self.build, metadata_path, self.subrepo)
     if _MetadataExists(metadata):
       _Print('Skipping diff for {} and {}. Matching diff already exists: {}',
-             archives[0].rev, archives[1].rev, diff_path)
+             before.rev, after.rev, diff_path)
     else:
       if os.path.exists(diff_path):
         os.remove(diff_path)
-      archive_dirs = [archives[0].dir, archives[1].dir]
       with open(diff_path, 'a') as diff_file:
         for d in self.diffs:
-          d.RunDiff(diff_file, archive_dirs)
+          d.RunDiff(diff_file, before.dir, after.dir)
         _Print('\nSee detailed diff results here: {}.', diff_path)
       _WriteMetadata(metadata)
-      self._AddDiffSummaryStat(archives)
+      self._AddDiffSummaryStat(before, after)
 
   def Summarize(self):
     if self._summary_stats:
       path = os.path.join(self.archive_dir, 'last_diff_summary.txt')
       with open(path, 'w') as f:
         stats = sorted(
             self._summary_stats, key=lambda x: x[0].value, reverse=True)
         _PrintAndWriteToFile(f, '\nDiff Summary')
         for s, before, after in stats:
           _PrintAndWriteToFile(f, '{:>+10} {} {} for range: {}..{}',
                                s.value, s.units, s.name, before, after)
 
-  def _AddDiffSummaryStat(self, archives):
+  def _AddDiffSummaryStat(self, before, after):
     stat = None
     if self.build.IsAndroid():
       summary_diff_type = ResourceSizesDiff
     else:
       summary_diff_type = NativeDiff
     for d in self.diffs:
       if isinstance(d, summary_diff_type):
         stat = d.summary_stat
     if stat:
-      self._summary_stats.append((stat, archives[1].rev, archives[0].rev))
+      self._summary_stats.append((stat, before.rev, after.rev))
 
-  def _CanDiff(self, archives):
-    return all(a.Exists() for a in archives)
+  def _CanDiff(self, before, after):
+    return before.Exists() and after.Exists()
 
-  def _DiffFilePath(self, archives):
-    return os.path.join(self._DiffDir(archives), 'diff_results.txt')
+  def _DiffFilePath(self, before, after):
+    return os.path.join(self._DiffDir(before, after), 'diff_results.txt')
 
-  def _DiffMetadataPath(self, archives):
-    return os.path.join(self._DiffDir(archives), 'metadata.txt')
+  def _DiffMetadataPath(self, before, after):
+    return os.path.join(self._DiffDir(before, after), 'metadata.txt')
 
-  def _DiffDir(self, archives):
-    archive_range = '%s..%s' % (archives[1].rev, archives[0].rev)
+  def _DiffDir(self, before, after):
+    archive_range = '%s..%s' % (before.rev, after.rev)
     diff_path = os.path.join(self.archive_dir, 'diffs', archive_range)
     _EnsureDirsExist(diff_path)
     return diff_path
 
 
 def _EnsureDirsExist(path):
   if not os.path.exists(path):
     os.makedirs(path)
 
 
@@ skipping 94 matching lines @@
     _GitCmd(['checkout',  archive.rev], subrepo)
   else:
     # Move to a detached state since gclient sync doesn't work with local
     # commits on a branch.
     _GitCmd(['checkout', '--detach'], subrepo)
     _GclientSyncCmd(archive.rev, subrepo)
   retcode = build.Run()
   return retcode == 0
 
 
-def _GenerateRevList(with_patch, without_patch, all_in_range, subrepo):
+def _GenerateRevList(rev, reference_rev, all_in_range, subrepo):
   """Normalize and optionally generate a list of commits in the given range.
 
-  Returns a list of revisions ordered from newest to oldest.
+  Returns:
+    A list of revisions ordered from oldest to newest.
   """
-  cmd = ['git', '-C', subrepo, 'merge-base', '--is-ancestor', without_patch,
-         with_patch]
-  _, retcode = _RunCmd(cmd, exit_on_failure=False)
-  assert not retcode and with_patch != without_patch, (
-      'Invalid revision arguments, rev_without_patch (%s) is newer than '
-      'rev_with_patch (%s)' % (without_patch, with_patch))
-
-  rev_seq = '%s^..%s' % (without_patch, with_patch)
+  rev_seq = '%s^..%s' % (reference_rev, rev)
   stdout = _GitCmd(['rev-list', rev_seq], subrepo)
-  all_revs = stdout.splitlines()
+  all_revs = stdout.splitlines()[::-1]
   if all_in_range:
     revs = all_revs
   else:
     revs = [all_revs[0], all_revs[-1]]
-  _VerifyUserAckCommitCount(len(revs))
+  if len(revs) >= _COMMIT_COUNT_WARN_THRESHOLD:
+    _VerifyUserAccepts(
+        'You\'ve provided a commit range that contains %d commits' % len(revs))
   return revs
 
 
-def _VerifyUserAckCommitCount(count):
-  if count >= _COMMIT_COUNT_WARN_THRESHOLD:
-    _Print('You\'ve provided a commit range that contains {} commits, do you '
-           'want to proceed? [y/n]', count)
-    if raw_input('> ').lower() != 'y':
-      _global_restore_checkout_func()
-      sys.exit(1)
+def _ValidateRevs(rev, reference_rev, subrepo):
+  def git_fatal(args, message):
+    devnull = open(os.devnull, 'wb')
+    retcode = subprocess.call(
+        ['git', '-C', subrepo] + args, stdout=devnull, stderr=subprocess.STDOUT)
+    if retcode:
+      _Die(message)
+
+  if rev == reference_rev:
+    _Die('rev and reference-rev cannot be equal')
+  no_obj_message = ('%s either doesn\'t exist or your local repo is out of '
+                    'date, try "git fetch origin master"')
+  git_fatal(['cat-file', '-e', rev], no_obj_message % rev)
+  git_fatal(['cat-file', '-e', reference_rev], no_obj_message % reference_rev)
+  git_fatal(['merge-base', '--is-ancestor', reference_rev, rev],
+            'reference-rev is newer than rev')
+  return rev, reference_rev
+
+
+def _VerifyUserAccepts(message):
+  _Print(message + 'Do you want to proceed? [y/n]')
+  if raw_input('> ').lower() != 'y':
+    _global_restore_checkout_func()
+    sys.exit()
 
 
 def _EnsureDirectoryClean(subrepo):
   _Print('Checking source directory')
   stdout = _GitCmd(['status', '--porcelain'], subrepo)
   # Ignore untracked files.
   if stdout and stdout[:2] != '??':
     _Print('Failure: please ensure working directory is clean.')
     sys.exit()
 
@@ skipping 17 matching lines @@
 
   download_dir = tempfile.mkdtemp(dir=_SRC_ROOT)
   try:
     _DownloadAndArchive(
         gsutil_path, archive, download_dir, build, supersize_path)
   finally:
     shutil.rmtree(download_dir)
 
 
 def _DownloadAndArchive(gsutil_path, archive, dl_dir, build, supersize_path):
-  dl_file = 'full-build-linux_%s.zip' % archive.rev
-  dl_url = 'gs://chrome-perf/Android Builder/%s' % dl_file
-  dl_dst = os.path.join(dl_dir, dl_file)
+  dl_dst = os.path.join(dl_dir, archive.rev)
   _Print('Downloading build artifacts for {}', archive.rev)
   # gsutil writes stdout and stderr to stderr, so pipe stdout and stderr to
   # sys.stdout.
-  retcode = subprocess.call([gsutil_path, 'cp', dl_url, dl_dir],
-                             stdout=sys.stdout, stderr=subprocess.STDOUT)
+  retcode = subprocess.call(
+      [gsutil_path, 'cp', build.DownloadUrl(archive.rev), dl_dst],
+      stdout=sys.stdout, stderr=subprocess.STDOUT)
   if retcode:
       _Die('unexpected error while downloading {}. It may no longer exist on '
            'the server or it may not have been uploaded yet (check {}). '
            'Otherwise, you may not have the correct access permissions.',
-           dl_url, _BUILDER_URL)
+           build.DownloadUrl(archive.rev), _BUILDER_URL)
 
   # Files needed for supersize and resource_sizes. Paths relative to out dir.
-  to_extract = [build.main_lib_path, build.map_file_path, 'args.gn',
-                'build_vars.txt', build.apk_path]
-  extract_dir = os.path.join(os.path.splitext(dl_dst)[0], 'unzipped')
+  to_extract = [build.main_lib_path, build.map_file_path, 'args.gn']
+  if build.IsAndroid():
+    to_extract += ['build_vars.txt', build.apk_path]
+  extract_dir = dl_dst + '_' + 'unzipped'
   # Storage bucket stores entire output directory including out/Release prefix.
   _Print('Extracting build artifacts')
   with zipfile.ZipFile(dl_dst, 'r') as z:
-    _ExtractFiles(to_extract, _CLOUD_OUT_DIR, extract_dir, z)
-    dl_out = os.path.join(extract_dir, _CLOUD_OUT_DIR)
+    _ExtractFiles(to_extract, build.download_output_dir, extract_dir, z)
+    dl_out = os.path.join(extract_dir, build.download_output_dir)
     build.output_directory, output_directory = dl_out, build.output_directory
     archive.ArchiveBuildResults(supersize_path)
     build.output_directory = output_directory
 
 
 def _ExtractFiles(to_extract, prefix, dst, z):
   zip_infos = z.infolist()
   assert all(info.filename.startswith(prefix) for info in zip_infos), (
       'Storage bucket folder structure doesn\'t start with %s' % prefix)
   to_extract = [os.path.join(prefix, f) for f in to_extract]
@@ skipping 84 matching lines @@
                            choices=['android', 'linux'],
                            help='target_os gn arg. Default: android.')
   build_group.add_argument('--output-directory',
                            default=_DEFAULT_OUT_DIR,
                            help='ninja output directory. '
                            'Default: %s.' % _DEFAULT_OUT_DIR)
   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. '
-                           'Default %s.' % _DEFAULT_TARGET)
+                           default=_DEFAULT_ANDROID_TARGET,
+                           help='GN APK target to build. Ignored for Linux. '
+                           'Default %s.' % _DEFAULT_ANDROID_TARGET)
   if len(sys.argv) == 1:
     parser.print_help()
     sys.exit()
   args = parser.parse_args()
   build = _BuildHelper(args)
-  if build.IsCloud():
-    if build.IsLinux():
-      parser.error('--cloud only works for android')
-    if args.subrepo:
+  if build.IsCloud() and args.subrepo:
       parser.error('--subrepo doesn\'t work with --cloud')
 
   subrepo = args.subrepo or _SRC_ROOT
   _EnsureDirectoryClean(subrepo)
   _SetRestoreFunc(subrepo)
-  revs = _GenerateRevList(args.rev,
-                          args.reference_rev or args.rev + '^',
-                          args.all,
-                          subrepo)
+  if build.IsLinux():
+    _VerifyUserAccepts('Linux diffs have known deficiencies (crbug/717550).')
+
+  rev, reference_rev = _ValidateRevs(
+      args.rev, args.reference_rev or args.rev + '^', subrepo)
+  revs = _GenerateRevList(rev, reference_rev, args.all, subrepo)
   with _TmpCopyBinarySizeDir() as supersize_path:
     diffs = [NativeDiff(build.size_name, supersize_path)]
     if build.IsAndroid():
       diffs +=  [
           ResourceSizesDiff(
               build.apk_name, slow_options=args.include_slow_options)
       ]
     diff_mngr = _DiffArchiveManager(
         revs, args.archive_directory, diffs, build, subrepo)
     consecutive_failures = 0
@@ skipping 19 matching lines @@
       if i != 0:
         diff_mngr.MaybeDiff(i - 1, i)
 
     diff_mngr.Summarize()
 
   _global_restore_checkout_func()
 
 if __name__ == '__main__':
   sys.exit(main())