SuperSize: Ignore .grdp path when matching up pak symbols in diffs

This will match up pak symbols when they have moved between grdp files.

Change-Id: I55ae383498c66a11c675cb1b0eb4866da4cae721
Reviewed-on: https://chromium-review.googlesource.com/1168456Reviewed-by: Peter Wen <wnwen@chromium.org>
Commit-Queue: Peter Wen <wnwen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581881}

tools/binary_size/libsupersize/archive.py

@@ -146,10 +146,15 @@ def _NormalizeNames(raw_symbols):
 
     # See comment in _CalculatePadding() about when this can happen. Don't
     # process names for non-native sections.
-    if (full_name.startswith('*') or
+    if symbol.IsPak():
+      # full_name: "about_ui_resources.grdp: IDR_ABOUT_UI_CREDITS_HTML".
+      space_idx = full_name.rindex(' ')
+      name = full_name[space_idx + 1:]
+      symbol.template_name = name
+      symbol.name = name
+    elif (full_name.startswith('*') or
         symbol.IsOverhead() or
-        symbol.IsOther() or
-        symbol.IsPak()):
+        symbol.IsOther()):
       symbol.template_name = full_name
       symbol.name = full_name
     elif symbol.IsDex():
@@ -685,10 +690,10 @@ def _AddNmAliases(raw_symbols, names_by_address):
   return ret
 
 
-def LoadAndPostProcessSizeInfo(path, fileobj=None):
+def LoadAndPostProcessSizeInfo(path, file_obj=None):
   """Returns a SizeInfo for the given |path|."""
   logging.debug('Loading results from: %s', path)
-  size_info = file_format.LoadSizeInfo(path, fileobj)
+  size_info = file_format.LoadSizeInfo(path, file_obj=file_obj)
   logging.info('Normalizing symbol names')
   _NormalizeNames(size_info.raw_symbols)
   logging.info('Calculating padding')

tools/binary_size/libsupersize/diff.py

@@ -39,13 +39,18 @@ def _GoodMatchKey(symbol):
     "._468", "._467"
     ".L__unnamed_1193", ".L__unnamed_712"
   """
-  name = _STRIP_NUMBER_SUFFIX_PATTERN.sub('', symbol.full_name)
-  clone_idx = name.find(' [clone ')
-  if clone_idx != -1:
-    name = name[:clone_idx]
-  if name.startswith('*'):
-    # "symbol gap 3 (bar)" -> "symbol gaps"
-    name = _NORMALIZE_STAR_SYMBOLS_PATTERN.sub('s', name)
+  if symbol.IsPak():
+    # full_name looks like "about_ui_resources.grdp: IDR_ABOUT_UI_CREDITS_HTML".
+    # name is just "IDR_ABOUT_UI_CREDITS_HTML".
+    name = symbol.name
+  else:
+    name = _STRIP_NUMBER_SUFFIX_PATTERN.sub('', symbol.full_name)
+    clone_idx = name.find(' [clone ')
+    if clone_idx != -1:
+      name = name[:clone_idx]
+    if name.startswith('*'):
+      # "symbol gap 3 (bar)" -> "symbol gaps"
+      name = _NORMALIZE_STAR_SYMBOLS_PATTERN.sub('s', name)
 
   return symbol.section, symbol.object_path, name
 

tools/binary_size/libsupersize/file_format.py

@@ -341,17 +341,21 @@ def _LoadSizeInfoFromFile(file_obj, size_path):
 
 
 @contextlib.contextmanager
-def _OpenGzipForWrite(path):
+def _OpenGzipForWrite(path, file_obj=None):
   # Open in a way that doesn't set any gzip header fields.
-  with open(path, 'wb') as f:
-    with gzip.GzipFile(filename='', mode='wb', fileobj=f, mtime=0) as fz:
+  if file_obj:
+    with gzip.GzipFile(filename='', mode='wb', fileobj=file_obj, mtime=0) as fz:
       yield fz
+  else:
+    with open(path, 'wb') as f:
+      with gzip.GzipFile(filename='', mode='wb', fileobj=f, mtime=0) as fz:
+        yield fz
 
 
-def SaveSizeInfo(size_info, path):
+def SaveSizeInfo(size_info, path, file_obj=None):
   """Saves |size_info| to |path}."""
   if os.environ.get('SUPERSIZE_MEASURE_GZIP') == '1':
-    with _OpenGzipForWrite(path) as f:
+    with _OpenGzipForWrite(path, file_obj=file_obj) as f:
       _SaveSizeInfoToFile(size_info, f)
   else:
     # It is seconds faster to do gzip in a separate step. 6s -> 3.5s.
@@ -360,11 +364,11 @@ def SaveSizeInfo(size_info, path):
 
     logging.debug('Serialization complete. Gzipping...')
     stringio.seek(0)
-    with _OpenGzipForWrite(path) as f:
+    with _OpenGzipForWrite(path, file_obj=file_obj) as f:
       shutil.copyfileobj(stringio, f)
 
 
-def LoadSizeInfo(filename, fileobj=None):
+def LoadSizeInfo(filename, file_obj=None):
   """Returns a SizeInfo loaded from |filename|."""
-  with gzip.GzipFile(filename=filename, fileobj=fileobj) as f:
+  with gzip.GzipFile(filename=filename, fileobj=file_obj) as f:
     return _LoadSizeInfoFromFile(f, filename)

tools/binary_size/libsupersize/integration_test.py

@@ -3,6 +3,7 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+import cStringIO
 import contextlib
 import copy
 import difflib
@@ -326,8 +327,8 @@ class IntegrationTest(unittest.TestCase):
 
   @_CompareWithGolden()
   def test_Diff_Basic(self):
-    size_info1 = self._CloneSizeInfo(use_elf=False)
-    size_info2 = self._CloneSizeInfo(use_elf=False)
+    size_info1 = self._CloneSizeInfo(use_elf=False, use_pak=True)
+    size_info2 = self._CloneSizeInfo(use_elf=False, use_pak=True)
     size_info1.metadata = {"foo": 1, "bar": [1,2,3], "baz": "yes"}
     size_info2.metadata = {"foo": 1, "bar": [1,3], "baz": "yes"}
 
@@ -338,16 +339,30 @@ class IntegrationTest(unittest.TestCase):
     padding_sym = size_info2.raw_symbols.WhereNameMatches('symbol gap 0')[0]
     padding_sym.padding += 20
     padding_sym.size += 20
+    pak_sym = size_info2.raw_symbols.WhereInSection(
+        models.SECTION_PAK_TRANSLATIONS)[0]
+    pak_sym.full_name = 'foo: ' + pak_sym.full_name.split()[-1]
+
+    # Serialize & de-serialize so that name normalization runs again for the pak
+    # symbol.
+    stringio = cStringIO.StringIO()
+    file_format.SaveSizeInfo(size_info2, 'path', file_obj=stringio)
+    stringio.seek(0)
+    size_info2 = archive.LoadAndPostProcessSizeInfo('path', file_obj=stringio)
+
     d = diff.Diff(size_info1, size_info2)
     d.raw_symbols = d.raw_symbols.Sorted()
     self.assertEquals(d.raw_symbols.CountsByDiffStatus()[1:], [2, 2, 3])
     changed_sym = d.raw_symbols.WhereNameMatches('Patcher::Name_')[0]
     padding_sym = d.raw_symbols.WhereNameMatches('symbol gap 0')[0]
+    bss_sym = d.raw_symbols.WhereInSection(models.SECTION_BSS)[0]
     # Padding-only deltas should sort after all non-padding changes.
     padding_idx = d.raw_symbols.index(padding_sym)
-    self.assertLess(d.raw_symbols.index(changed_sym), padding_idx)
+    changed_idx = d.raw_symbols.index(changed_sym)
+    bss_idx = d.raw_symbols.index(bss_sym)
+    self.assertLess(changed_idx, padding_idx)
     # And before bss.
-    self.assertTrue(d.raw_symbols[padding_idx + 1].IsBss())
+    self.assertLess(padding_idx, bss_idx)
 
     return describe.GenerateLines(d, verbose=True)
 

tools/binary_size/libsupersize/testdata/Diff_Basic.golden

@@ -10,6 +10,7 @@ Section Sizes (Total=0 bytes (0 bytes)):
     .bss: 0 bytes (0 bytes) (not included in totals)
     .data: 0 bytes (0 bytes) (0.0%)
     .data.rel.ro: 0 bytes (0 bytes) (0.0%)
+    .pak.translations: 0 bytes (0 bytes) (0.0%)
     .rel.dyn: 0 bytes (0 bytes) (0.0%)
     .rodata: 0 bytes (0 bytes) (0.0%)
     .text: 0 bytes (0 bytes) (0.0%)
@@ -38,42 +39,45 @@ Other section sizes:
     .strtab: 0 bytes (0 bytes)
     .symtab: 0 bytes (0 bytes)
 
-2 symbols added (+), 2 changed (~), 3 removed (-), 34 unchanged (not shown)
-Of changed symbols, 4 grew, 3 shrank
-Number of unique symbols 44 -> 43 (-1)
+2 symbols added (+), 2 changed (~), 3 removed (-), 242 unchanged (not shown)
+Of changed symbols, 3 grew, 4 shrank
+Number of unique symbols 251 -> 250 (-1)
 0 paths added, 0 removed, 2 changed
 Changed files:
   
   third_party/container/container.c
 
-Showing 7 symbols (5 -> 4 unique) with total pss: 38 bytes
+Showing 7 symbols (5 -> 4 unique) with total pss: -112 bytes
 Histogram of symbols based on PSS:
-    (-256,-128]: 1   (-32,-16]: 1   (-8,-4]: 1   [4,8): 2   [8,16): 1   [16,32): 1
-.text=0 bytes    .rodata=10 bytes   .data.rel.ro=0 bytes    .data=28 bytes   .bss=-232 bytes total=38 bytes
-Number of unique paths: 4
+    (-128,-64]: 1   (-32,-16]: 2   (-16,-8]: 1   [4,8): 2   [8,16): 1
+.text=0 bytes    .rodata=10 bytes   .data.rel.ro=0 bytes    .data=8 bytes    .bss=0 bytes    .pak.translations=-130 bytes total=-112 bytes
+Number of unique paths: 3
 
-Section Legend: t=.text, r=.rodata, R=.data.rel.ro, d=.data, b=.bss
+Section Legend: t=.text, r=.rodata, R=.data.rel.ro, d=.data, b=.bss, p=.pak.translations
 Index | Running Total | Section@Address | ...
 ------------------------------------------------------------
-~ 0)         10 (26.3%) r@0x284e398  +10 (22->32)  num_aliases=1
+- 0)        -80 (71.4%) p@0x0        -80 (80->0)  num_aliases=1
+               source_path= 	object_path=
+               flags={}  name=IDS_AW_WEBPAGE_PARENTAL_PERMISSION_NEEDED
+                    full_name=../../android_webview/ui/aw_strings.grd: IDS_AW_WEBPAGE_PARENTAL_PERMISSION_NEEDED
+- 1)       -103 (92.0%) p@0x0        -23 (23->0)  num_aliases=1
+               source_path= 	object_path=
+               flags={}  name=IDS_WEB_FONT_FAMILY
+                    full_name=../../ui/strings/app_locale_settings.grd: IDS_WEB_FONT_FAMILY
+- 2)       -112 (100.0%) p@0x0        -9 (9->0)  num_aliases=1
+               source_path= 	object_path=
+               flags={}  name=IDS_WEB_FONT_SIZE
+                    full_name=../../ui/strings/app_locale_settings.grd: IDS_WEB_FONT_SIZE
+~ 3)       -130 (116.1%) p@0x0        -18 (0->0)  num_aliases=1
+               source_path= 	object_path=
+               flags={}  name=../../../mock_apk/assets/en-US.pak
+                    full_name=foo: ../../../mock_apk/assets/en-US.pak
+~ 4)       -120 (107.1%) r@0x284e398  +10 (22->32)  num_aliases=1
                source_path=third_party/container/container.c 	object_path=third_party/sub/ContiguousContainer.o
                flags={}  name=chrome::mojom::FilePatcher::Name_
-+ 1)         14 (36.8%) d@0x2de7000  +4 (0->4)  num_aliases=1
++ 5)       -116 (103.6%) d@0x2de7000  +4 (0->4)  num_aliases=1
                source_path=base/page_allocator.cc 	object_path=base/base/page_allocator.o
                flags={}  name=google::protobuf::internal::pLinuxKernelCmpxchg
-+ 2)         18 (47.4%) d@0x2de7004  +4 (0->4)  num_aliases=1
++ 6)       -112 (100.0%) d@0x2de7004  +4 (0->4)  num_aliases=1
                source_path=third_party/container/container.c 	object_path=third_party/sub/ContiguousContainer.o
                flags={}  name=google::protobuf::internal::pLinuxKernelMemoryBarrier
-~ 3)         38 (100.0%) d@0x2dffd88  20 (0->0)  num_aliases=1
-               source_path= 	object_path=
-               flags={}  name=** symbol gap 0 (end of section)
-- 4)         38 (100.0%) b@0x0        -200 (4->0)  num_aliases=1
-               source_path=third_party/icu/ucnv_ext.c 	object_path=third_party/icu/icuuc/ucnv_ext.o
-               flags={gen}  name=SaveHistogram::atomic_histogram_pointer
-                    full_name=SaveHistogram(_JNIEnv*, base::android::JavaParamRef<_jobject*> const&, base::android::JavaParamRef<_jstring*> const&, base::android::JavaParamRef<_jlongArray*> const&, int)::atomic_histogram_pointer
-- 5)         38 (100.0%) b@0x0        -28 (28->0)  num_aliases=1
-               source_path=third_party/icu/ucnv_ext.c 	object_path=third_party/icu/icuuc/ucnv_ext.o
-               flags={gen}  name=g_chrome_content_browser_client
-- 6)         38 (100.0%) b@0x0        -4 (4->0)  num_aliases=1
-               source_path=third_party/icu/ucnv_ext.c 	object_path=third_party/icu/icuuc/ucnv_ext.o
-               flags={anon,gen}  name=g_AnimationFrameTimeHistogram_clazz