SuperSize: Fix zipalign overhead calculation

The previous code worked for .apk files created by our build
system (where Python performs alignment), but did not work
for .apk files that are aligned using Android's zipalign tool
(as is the case for bundles).

Also changes zipalign overhead to be stored in metadata
rather than as a symbol so that the supersize symbols sum
matches normalized apk size.

Bug: 1130754
Change-Id: Ia852ec1efec4e7a13faa5072cd67cd204148c8b8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2423409
Commit-Queue: Andrew Grieve <agrieve@chromium.org>
Reviewed-by: Samuel Huang <huangs@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809515}

build/android/gyp/util/zipalign.py

@@ -73,8 +73,9 @@ def _SetAlignment(zip_obj, zip_info, alignment):
       (cur_offset + header_size) % alignment)) % alignment
 
 
-  # Extra field used to 4-byte align classes.dex. Alignment speeds up
-  # execution when dex files are used via incremental install.
+  # Python writes |extra| to both the local file header and the central
+  # directory's file header. Android's zipalign tool writes only to the
+  # local file header, so there is more overhead in using python to align.
   zip_info.extra = b'\0' * padding_needed
 
 

build/android/resource_sizes.py

@@ -18,6 +18,7 @@ import logging
 import os
 import posixpath
 import re
+import struct
 import sys
 import tempfile
 import zipfile
@@ -94,6 +95,17 @@ def _PercentageDifference(a, b):
   return float(b - a) / a
 
 
+def _ReadZipInfoExtraFieldLength(zip_file, zip_info):
+  """Reads the value of |extraLength| from |zip_info|'s local file header.
+
+  |zip_info| has an |extra| field, but it's read from the central directory.
+  Android's zipalign tool sets the extra field only in local file headers.
+  """
+  # Refer to https://en.wikipedia.org/wiki/Zip_(file_format)#File_headers
+  zip_file.fp.seek(zip_info.header_offset + 28)
+  return struct.unpack('<H', zip_file.fp.read(2))[0]
+
+
 def _RunReadelf(so_path, options, tool_prefix=''):
   return cmd_helper.GetCmdOutput(
       [tool_prefix + 'readelf'] + options + [so_path])
@@ -303,6 +315,8 @@ def _DoApkAnalysis(apk_filename, apks_path, tool_prefix, out_dir, report_func):
 
   with zipfile.ZipFile(apk_filename, 'r') as apk:
     apk_contents = apk.infolist()
+    zipalign_overhead = sum(
+        _ReadZipInfoExtraFieldLength(apk, i) for i in apk_contents)
 
   sdk_version, skip_extract_lib = _ParseManifestAttributes(apk_filename)
 
@@ -473,7 +487,7 @@ def _DoApkAnalysis(apk_filename, apks_path, tool_prefix, out_dir, report_func):
   # causes size changes files that proceed aligned files to be rounded.
   # For APKs where classes.dex directly proceeds libchrome.so, this causes
   # small dex size changes to disappear into libchrome.so alignment.
-  normalized_apk_size -= sum(len(i.extra) for i in apk_contents)
+  normalized_apk_size -= zipalign_overhead
 
   # Unaligned size should be ~= uncompressed size or something is wrong.
   # As of now, padding_fraction ~= .007

tools/binary_size/libsupersize/archive.py

@@ -1253,7 +1253,8 @@ class _ResourcePathDeobfuscator(object):
 
 
 def _ParseApkOtherSymbols(section_ranges, apk_path, apk_so_path,
-                          resources_pathmap_path, size_info_prefix, knobs):
+                          resources_pathmap_path, size_info_prefix, metadata,
+                          knobs):
   res_source_mapper = _ResourceSourceMapper(size_info_prefix, knobs)
   resource_deobfuscator = _ResourcePathDeobfuscator(resources_pathmap_path)
   apk_symbols = []
@@ -1263,7 +1264,7 @@ def _ParseApkOtherSymbols(section_ranges, apk_path, apk_so_path,
   with zipfile.ZipFile(apk_path) as z:
     for zip_info in z.infolist():
       zip_info_total += zip_info.compress_size
-      zipalign_total += len(zip_info.extra)
+      zipalign_total += zip_util.ReadZipInfoExtraFieldLength(z, zip_info)
       # Skip main shared library, pak, and dex files as they are accounted for.
       if (zip_info.filename == apk_so_path
           or zip_info.filename.endswith('.pak')):
@@ -1288,11 +1289,9 @@ def _ParseApkOtherSymbols(section_ranges, apk_path, apk_so_path,
   zip_overhead_symbol = models.Symbol(
       models.SECTION_OTHER, overhead_size, full_name='Overhead: APK file')
   apk_symbols.append(zip_overhead_symbol)
-  if zipalign_total > 0:
-    zipalign_symbol = models.Symbol(models.SECTION_OTHER,
-                                    zipalign_total,
-                                    full_name='Overhead: zipalign')
-    apk_symbols.append(zipalign_symbol)
+  # Store as metadata rather than an Overhead: symbol so that the sum of symbols
+  # matches normalized apk size.
+  metadata[models.METADATA_ZIPALIGN_OVERHEAD] = zipalign_total
   _ExtendSectionRange(section_ranges, models.SECTION_OTHER,
                       sum(s.size for s in apk_symbols))
   return dex_size, apk_symbols
@@ -1597,9 +1596,11 @@ def CreateContainerAndSymbols(knobs=None,
                                                size_info_prefix)
 
     # Can modify |section_ranges|.
-    dex_size, other_symbols = _ParseApkOtherSymbols(
-        section_ranges, apk_path, apk_so_path, resources_pathmap_path,
-        size_info_prefix, knobs)
+    dex_size, other_symbols = _ParseApkOtherSymbols(section_ranges, apk_path,
+                                                    apk_so_path,
+                                                    resources_pathmap_path,
+                                                    size_info_prefix, metadata,
+                                                    knobs)
 
     if opts.analyze_java:
       dex_symbols = apkanalyzer.CreateDexSymbols(apk_path, mapping_path,

tools/binary_size/libsupersize/models.py

@@ -52,6 +52,7 @@ BUILD_CONFIG_KEYS = (
 
 METADATA_APK_FILENAME = 'apk_file_name'  # Path relative to output_directory.
 METADATA_APK_SIZE = 'apk_size'  # File size of apk in bytes.
+METADATA_ZIPALIGN_OVERHEAD = 'zipalign_padding'  # Overhead from zipalign.
 METADATA_MAP_FILENAME = 'map_file_name'  # Path relative to output_directory.
 METADATA_ELF_ARCHITECTURE = 'elf_arch'  # "Machine" field from readelf -h
 METADATA_ELF_FILENAME = 'elf_file_name'  # Path relative to output_directory.

tools/binary_size/libsupersize/testdata/Archive_Apk.golden

@@ -10,6 +10,7 @@ gn_args=var1=true var2="foo"
 linker_name=gold
 map_file_name=../../../test.map
 tool_prefix=tools/binary_size/libsupersize/testdata/mock_toolchain/
+zipalign_padding=16
 Section .text: has 100.0% of 35982248 bytes accounted for from 22 symbols. 0 bytes are unaccounted for.
 * Padding accounts for 13808 bytes (0.0%)
 * 16 have source paths. Accounts for 73986 bytes (0.2%).
@@ -74,8 +75,8 @@ Section .pak.nontranslated: has 100.0% of 737 bytes accounted for from 3 symbols
 * 0 have source paths. Accounts for 0 bytes (0.0%).
 * 0 have a component assigned. Accounts for 0 bytes (0.0%).
 * 0 symbols have shared ownership.
-Section .other: has 100.0% of 95595829 bytes accounted for from 28 symbols. 0 bytes are unaccounted for.
-* Padding accounts for 33903431 bytes (35.5%)
+Section .other: has 100.0% of 95595813 bytes accounted for from 27 symbols. 0 bytes are unaccounted for.
+* Padding accounts for 33903415 bytes (35.5%)
 * 3 have source paths. Accounts for 5243904 bytes (5.5%).
 * 1 have a component assigned. Accounts for 1048576 bytes (1.1%).
 * 22 placeholders exist (symbols that start with **). Accounts for 56448494 bytes (59.0%).
@@ -309,7 +310,6 @@ Section .other: has 100.0% of 95595829 bytes accounted for from 28 symbols. 0 by
 .other@0(size_without_padding=1048576,padding=0,full_name=assets/icudtl.dat,object_path=,source_path=third_party/icu/android/icudtl.dat,flags={},num_aliases=1,component=Internal>Android)
 .other@0(size_without_padding=1024,padding=0,full_name=res/drawable-v13/test.xml,object_path=,source_path=chrome/android/res/drawable/test.xml,flags={},num_aliases=1,component=)
 .other@0(size_without_padding=0,padding=780,full_name=Overhead: APK file,object_path=,source_path=,flags={},num_aliases=1,component=)
-.other@0(size_without_padding=0,padding=16,full_name=Overhead: zipalign,object_path=,source_path=,flags={},num_aliases=1,component=)
 .other@0(size_without_padding=0,padding=33902635,full_name=Overhead: ELF file,object_path=,source_path=,flags={},num_aliases=1,component=)
 .other@154(size_without_padding=19,padding=0,full_name=** ELF Section: .interp,object_path=,source_path=,flags={},num_aliases=1,component=)
 .other@168(size_without_padding=36,padding=0,full_name=** ELF Section: .note.gnu.build-id,object_path=,source_path=,flags={},num_aliases=1,component=)

tools/binary_size/libsupersize/testdata/Archive_MinimalApks.golden

@@ -11,6 +11,7 @@ gn_args=var1=true var2="foo"
 linker_name=gold
 map_file_name=../../../test.map
 tool_prefix=tools/binary_size/libsupersize/testdata/mock_toolchain/
+zipalign_padding=16
 Section .text: has 100.0% of 35982248 bytes accounted for from 22 symbols. 0 bytes are unaccounted for.
 * Padding accounts for 13808 bytes (0.0%)
 * 16 have source paths. Accounts for 73986 bytes (0.2%).
@@ -75,8 +76,8 @@ Section .pak.nontranslated: has 100.0% of 737 bytes accounted for from 3 symbols
 * 0 have source paths. Accounts for 0 bytes (0.0%).
 * 0 have a component assigned. Accounts for 0 bytes (0.0%).
 * 0 symbols have shared ownership.
-Section .other: has 100.0% of 95595829 bytes accounted for from 28 symbols. 0 bytes are unaccounted for.
-* Padding accounts for 33903431 bytes (35.5%)
+Section .other: has 100.0% of 95595813 bytes accounted for from 27 symbols. 0 bytes are unaccounted for.
+* Padding accounts for 33903415 bytes (35.5%)
 * 3 have source paths. Accounts for 5243904 bytes (5.5%).
 * 1 have a component assigned. Accounts for 1048576 bytes (1.1%).
 * 22 placeholders exist (symbols that start with **). Accounts for 56448494 bytes (59.0%).
@@ -310,7 +311,6 @@ Section .other: has 100.0% of 95595829 bytes accounted for from 28 symbols. 0 by
 .other@0(size_without_padding=1048576,padding=0,full_name=assets/icudtl.dat,object_path=,source_path=third_party/icu/android/icudtl.dat,flags={},num_aliases=1,component=Internal>Android)
 .other@0(size_without_padding=1024,padding=0,full_name=res/drawable-v13/test.xml,object_path=,source_path=chrome/android/res/drawable/test.xml,flags={},num_aliases=1,component=)
 .other@0(size_without_padding=0,padding=780,full_name=Overhead: APK file,object_path=,source_path=,flags={},num_aliases=1,component=)
-.other@0(size_without_padding=0,padding=16,full_name=Overhead: zipalign,object_path=,source_path=,flags={},num_aliases=1,component=)
 .other@0(size_without_padding=0,padding=33902635,full_name=Overhead: ELF file,object_path=,source_path=,flags={},num_aliases=1,component=)
 .other@154(size_without_padding=19,padding=0,full_name=** ELF Section: .interp,object_path=,source_path=,flags={},num_aliases=1,component=)
 .other@168(size_without_padding=36,padding=0,full_name=** ELF Section: .note.gnu.build-id,object_path=,source_path=,flags={},num_aliases=1,component=)

tools/binary_size/libsupersize/zip_util.py

@@ -7,6 +7,7 @@
 import contextlib
 import logging
 import os
+import struct
 import tempfile
 import zipfile
 
@@ -34,3 +35,14 @@ def UnzipToTemp(zip_path, inner_path):
     yield temp_file
   finally:
     os.unlink(temp_file)
+
+
+def ReadZipInfoExtraFieldLength(zip_file, zip_info):
+  """Reads the value of |extraLength| from |zip_info|'s local file header.
+
+  |zip_info| has an |extra| field, but it's read from the central directory.
+  Android's zipalign tool sets the extra field only in local file headers.
+  """
+  # Refer to https://en.wikipedia.org/wiki/Zip_(file_format)#File_headers
+  zip_file.fp.seek(zip_info.header_offset + 28)
+  return struct.unpack('<H', zip_file.fp.read(2))[0]