Supersize now understand shortened resource paths

Supersize the tool that is used by the binary size trybot is now able to
deobfuscate the resource paths for APKs and AABs that enable resource
path shortening. This should allow the binary size trybot to blame
resource size diffs on the correct directories.

Bug: 753402
Change-Id: I396fd36b284a184bb5b07abf67d6b342920f0bc7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1693172
Commit-Queue: Mohamed Heikal <mheikal@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#677082}

build/android/gyp/create_app_bundle.py

@@ -68,7 +68,9 @@ def _ParseArgs(args):
   parser.add_argument(
       '--pathmap-in-paths',
       action='append',
-      help='GN-list of module pathmap files.')
+      help='List of module pathmap files.')
+  parser.add_argument(
+      '--module-names', action='append', help='List of module names.')
   parser.add_argument(
       '--pathmap-out-path', help='Path to combined pathmap file for bundle.')
   parser.add_argument(
@@ -329,6 +331,46 @@ def _ConcatTextFiles(in_paths, out_path):
         out_file.write(in_file.read())
 
 
+def _LoadPathmap(pathmap_path):
+  """Load the pathmap of obfuscated resource paths.
+
+  Returns: A dict mapping from obfuscated paths to original paths or an
+           empty dict if passed a None |pathmap_path|.
+  """
+  if pathmap_path is None:
+    return {}
+
+  pathmap = {}
+  with open(pathmap_path, 'r') as f:
+    for line in f:
+      line = line.strip()
+      if line.startswith('--') or line == '':
+        continue
+      original, renamed = line.split(' -> ')
+      pathmap[renamed] = original
+  return pathmap
+
+
+def _WriteBundlePathmap(module_pathmap_paths, module_names,
+                        bundle_pathmap_path):
+  """Combine the contents of module pathmaps into a bundle pathmap.
+
+  This rebases the resource paths inside the module pathmap before adding them
+  to the bundle pathmap. So res/a.xml inside the base module pathmap would be
+  base/res/a.xml in the bundle pathmap.
+  """
+  with open(bundle_pathmap_path, 'w') as bundle_pathmap_file:
+    for module_pathmap_path, module_name in zip(module_pathmap_paths,
+                                                module_names):
+      if not os.path.exists(module_pathmap_path):
+        continue
+      module_pathmap = _LoadPathmap(module_pathmap_path)
+      for short_path, long_path in module_pathmap.iteritems():
+        rebased_long_path = '{}/{}'.format(module_name, long_path)
+        rebased_short_path = '{}/{}'.format(module_name, short_path)
+        line = '{} -> {}\n'.format(rebased_long_path, rebased_short_path)
+        bundle_pathmap_file.write(line)
+
 
 def main(args):
   args = build_utils.ExpandFileArgs(args)
@@ -371,6 +413,7 @@ def main(args):
     cmd_args += ['--output=%s' % tmp_unsigned_bundle]
     cmd_args += ['--config=%s' % tmp_bundle_config]
 
+    print ' '.join(cmd_args)
     build_utils.CheckOutput(cmd_args, print_stdout=True, print_stderr=True)
 
     if options.keystore_path:
@@ -394,7 +437,8 @@ def main(args):
     _ConcatTextFiles(options.rtxt_in_paths, options.rtxt_out_path)
 
   if options.pathmap_out_path:
-    _ConcatTextFiles(options.pathmap_in_paths, options.pathmap_out_path)
+    _WriteBundlePathmap(options.pathmap_in_paths, options.module_names,
+                        options.pathmap_out_path)
 
 
 if __name__ == '__main__':

build/config/android/rules.gni

@@ -4697,8 +4697,9 @@ if (enable_java_templates) {
         ]
       }
 
-      foreach(build_config, _all_module_build_configs) {
-        _rebased_build_config = rebase_path(build_config, root_build_dir)
+      foreach(_module, _all_modules) {
+        _rebased_build_config =
+            rebase_path(_module.build_config, root_build_dir)
         args += [
           "--uncompressed-assets=@FileArg(" +
               "$_rebased_build_config:uncompressed_assets)",
@@ -4706,6 +4707,7 @@ if (enable_java_templates) {
               "$_rebased_build_config:deps_info:r_text_path)",
           "--pathmap-in-paths=@FileArg(" +
               "$_rebased_build_config:deps_info:module_pathmap_path)",
+          "--module-names=" + _module.name,
         ]
       }
     }

chrome/android/chrome_public_apk_tmpl.gni

@@ -170,7 +170,7 @@ template("chrome_public_common_apk_or_module_tmpl") {
     strip_resource_names = true
 
     # TODO(753402): enable once aapt2>=3.6.0 and bundletool>0.9.0 are rolled in.
-    short_resource_paths = false
+    short_resource_paths = true
 
     if (defined(shared_libraries) && shared_libraries != []) {
       _native_lib_file =

tools/binary_size/libsupersize/archive.py

@@ -1116,9 +1116,30 @@ def _ParseApkElfSectionSize(section_sizes, metadata, apk_elf_result):
   return section_sizes, 0
 
 
+def _LoadResourcesPathmap(pathmap_path):
+  """Load the pathmap of obfuscated resource paths.
+
+  Returns: A dict mapping from obfuscated paths to original paths or an
+           empty dict if passed a None |pathmap_path|.
+  """
+  if pathmap_path is None:
+    return {}
+
+  pathmap = {}
+  with open(pathmap_path, 'r') as f:
+    for line in f:
+      line = line.strip()
+      if line.startswith('--') or line == '':
+        continue
+      original, renamed = line.split(' -> ')
+      pathmap[renamed] = original
+  return pathmap
+
+
 def _ParseApkOtherSymbols(section_sizes, apk_path, apk_so_path,
-                          size_info_prefix, knobs):
+                          resources_pathmap_path, size_info_prefix, knobs):
   res_source_mapper = _ResourceSourceMapper(size_info_prefix, knobs)
+  resources_pathmap = _LoadResourcesPathmap(resources_pathmap_path)
   apk_symbols = []
   dex_size = 0
   zip_info_total = 0
@@ -1133,13 +1154,17 @@ def _ParseApkOtherSymbols(section_sizes, apk_path, apk_so_path,
         dex_size += zip_info.file_size
         continue
 
-      source_path = res_source_mapper.FindSourceForPath(zip_info.filename)
+      resource_filename = resources_pathmap.get(zip_info.filename,
+                                                zip_info.filename)
+      source_path = res_source_mapper.FindSourceForPath(resource_filename)
       if source_path is None:
-        source_path = os.path.join(models.APK_PREFIX_PATH, zip_info.filename)
-      apk_symbols.append(models.Symbol(
-            models.SECTION_OTHER, zip_info.compress_size,
-            source_path=source_path,
-            full_name=zip_info.filename))  # Full name must disambiguate
+        source_path = os.path.join(models.APK_PREFIX_PATH, resource_filename)
+      apk_symbols.append(
+          models.Symbol(
+              models.SECTION_OTHER,
+              zip_info.compress_size,
+              source_path=source_path,
+              full_name=resource_filename))  # Full name must disambiguate
   overhead_size = os.path.getsize(apk_path) - zip_info_total
   assert overhead_size >= 0, 'Apk overhead must be non-negative'
   zip_overhead_symbol = models.Symbol(
@@ -1225,11 +1250,21 @@ def _CalculateElfOverhead(section_sizes, elf_path):
   return 0
 
 
-def CreateSectionSizesAndSymbols(
-      map_path=None, tool_prefix=None, output_directory=None, elf_path=None,
-      apk_path=None, mapping_path=None, track_string_literals=True,
-      metadata=None, apk_so_path=None, pak_files=None, pak_info_file=None,
-      linker_name=None, size_info_prefix=None, knobs=None):
+def CreateSectionSizesAndSymbols(map_path=None,
+                                 tool_prefix=None,
+                                 output_directory=None,
+                                 elf_path=None,
+                                 apk_path=None,
+                                 mapping_path=None,
+                                 resources_pathmap_path=None,
+                                 track_string_literals=True,
+                                 metadata=None,
+                                 apk_so_path=None,
+                                 pak_files=None,
+                                 pak_info_file=None,
+                                 linker_name=None,
+                                 size_info_prefix=None,
+                                 knobs=None):
   """Creates sections sizes and symbols for a SizeInfo.
 
   Args:
@@ -1240,6 +1275,8 @@ def CreateSectionSizesAndSymbols(
     elf_path: Path to the corresponding unstripped ELF file. Used to find symbol
         aliases and inlined functions. Can be None.
     apk_path: Path to the .apk file to measure.
+    resources_pathmap_path: Path to the pathmap file that maps original
+        resource paths to shortened resource paths.
     track_string_literals: Whether to break down "** merge string" sections into
         smaller symbols (requires output_directory).
     metadata: Metadata dict from CreateMetadata().
@@ -1315,7 +1352,8 @@ def CreateSectionSizesAndSymbols(
         apk_path, mapping_path, size_info_prefix, output_directory)
     raw_symbols.extend(dex_symbols)
     dex_size, other_symbols = _ParseApkOtherSymbols(
-        section_sizes, apk_path, apk_so_path, size_info_prefix, knobs)
+        section_sizes, apk_path, apk_so_path, resources_pathmap_path,
+        size_info_prefix, knobs)
     raw_symbols.extend(other_symbols)
 
     # We can't meaningfully track section size of dex methods vs other, so just
@@ -1486,6 +1524,9 @@ def AddMainPathsArguments(parser):
   parser.add_argument('--apk-file',
                       help='.apk file to measure. Other flags can generally be '
                            'derived when this is used.')
+  parser.add_argument('--resources-pathmap-file',
+                      help='.pathmap.txt file that contains a maping from '
+                      'original resource paths to shortened resource paths.')
   parser.add_argument('--minimal-apks-file',
                       help='.minimal.apks file to measure. Other flags can '
                            'generally be derived when this is used.')
@@ -1537,6 +1578,7 @@ def _DeduceMainPaths(args, parser, extracted_minimal_apk_path=None):
 
   aab_or_apk = args.apk_file or args.minimal_apks_file
   mapping_path = args.mapping_file
+  resources_pathmap_path = args.resources_pathmap_file
   if aab_or_apk:
     # Allow either .minimal.apks or just .apks.
     aab_or_apk = aab_or_apk.replace('.minimal.apks', '.aab')
@@ -1544,6 +1586,17 @@ def _DeduceMainPaths(args, parser, extracted_minimal_apk_path=None):
     if not mapping_path:
       mapping_path = aab_or_apk + '.mapping'
       logging.debug('Detected --mapping-file=%s', mapping_path)
+    if not resources_pathmap_path:
+      possible_pathmap_path = aab_or_apk + '.pathmap.txt'
+      # This could be pointing to a stale pathmap file if path shortening was
+      # previously enabled but is disabled for the current build. However, since
+      # current apk/aab will have unshortened paths, looking those paths up in
+      # the stale pathmap which is keyed by shortened paths would not find any
+      # mapping and thus should not cause any issues.
+      if os.path.exists(possible_pathmap_path):
+        resources_pathmap_path = possible_pathmap_path
+        logging.debug('Detected --resources-pathmap-file=%s',
+                      resources_pathmap_path)
 
   apk_path = extracted_minimal_apk_path or args.apk_file
   apk_so_path = None
@@ -1594,7 +1647,8 @@ def _DeduceMainPaths(args, parser, extracted_minimal_apk_path=None):
         output_directory, 'size-info', os.path.basename(aab_or_apk))
 
   return (output_directory, tool_prefix, apk_path, mapping_path, apk_so_path,
-          elf_path, map_path, linker_name, size_info_prefix)
+          elf_path, map_path, resources_pathmap_path, linker_name,
+          size_info_prefix)
 
 
 def Run(args, parser):
@@ -1625,7 +1679,8 @@ def Run(args, parser):
 
 def _RunInternal(args, parser, extracted_minimal_apk_path):
   (output_directory, tool_prefix, apk_path, mapping_path, apk_so_path, elf_path,
-   map_path, linker_name, size_info_prefix) = _DeduceMainPaths(
+   map_path, resources_pathmap_path,
+   linker_name, size_info_prefix) = _DeduceMainPaths(
        args, parser, extracted_minimal_apk_path)
 
   metadata = CreateMetadata(map_path, elf_path, args.apk_file,
@@ -1637,13 +1692,21 @@ def _RunInternal(args, parser, extracted_minimal_apk_path):
     knobs.src_root = args.source_directory
 
   section_sizes, raw_symbols = CreateSectionSizesAndSymbols(
-      map_path=map_path, tool_prefix=tool_prefix, elf_path=elf_path,
-      apk_path=apk_path, mapping_path=mapping_path,
+      map_path=map_path,
+      tool_prefix=tool_prefix,
+      elf_path=elf_path,
+      apk_path=apk_path,
+      mapping_path=mapping_path,
       output_directory=output_directory,
+      resources_pathmap_path=resources_pathmap_path,
       track_string_literals=args.track_string_literals,
-      metadata=metadata, apk_so_path=apk_so_path,
-      pak_files=args.pak_file, pak_info_file=args.pak_info_file,
-      linker_name=linker_name, size_info_prefix=size_info_prefix, knobs=knobs)
+      metadata=metadata,
+      apk_so_path=apk_so_path,
+      pak_files=args.pak_file,
+      pak_info_file=args.pak_info_file,
+      linker_name=linker_name,
+      size_info_prefix=size_info_prefix,
+      knobs=knobs)
   size_info = CreateSizeInfo(
       section_sizes, raw_symbols, metadata=metadata, normalize_names=False)