Binary size tool, handle symbols with no path better.

If symbols with the same name appeared in the same "file" (as in
'unknown file') then they collided and only the last such symbol was
remembered. This happened for me with string symbols where all
.L.str21 were in the ?? file.

Store a list of symbol sizes per symbol and file. If there are more
than one symbol size per symbol and file, compare them differently to
see what might have happened.

A bit hard to read code. I hope it won't have to be changed.

BUG=412335
R=andrewhayden@chromium.org

Review URL: https://codereview.chromium.org/551203004

Cr-Commit-Position: refs/heads/master@{#295033}

2 files changed, 175 insertions, 15 deletions

diff --git a/tools/binary_size/explain_binary_size_delta.py b/tools/binary_size/explain_binary_size_delta.py
index 88a517b..cb99fe6 100755
--- a/tools/binary_size/explain_binary_size_delta.py
+++ b/tools/binary_size/explain_binary_size_delta.py
@@ -37,6 +37,7 @@ dumps. Example:
   explain_binary_size_delta.py --nm1 /tmp/nm1.dump --nm2 /tmp/nm2.dump
 """
 
+import collections
 import operator
 import optparse
 import os
@@ -69,7 +70,8 @@ def Compare(symbols1, symbols2):
         file_path = '(No Path)'
       key = (file_path, symbol_type)
       bucket = cache.setdefault(key, {})
-      bucket[symbol_name] = symbol_size
+      size_list = bucket.setdefault(symbol_name, [])
+      size_list.append(symbol_size)
 
   # Now diff them. We iterate over the elements in cache1. For each symbol
   # that we find in cache2, we record whether it was deleted, changed, or
@@ -79,32 +81,61 @@ def Compare(symbols1, symbols2):
     bucket2 = cache2.get(key)
     if not bucket2:
       # A file was removed. Everything in bucket1 is dead.
-      for symbol_name, symbol_size in bucket1.items():
-        removed.append((key[0], key[1], symbol_name, symbol_size, None))
+      for symbol_name, symbol_size_list in bucket1.items():
+        for symbol_size in symbol_size_list:
+          removed.append((key[0], key[1], symbol_name, symbol_size, None))
     else:
       # File still exists, look for changes within.
-      for symbol_name, symbol_size in bucket1.items():
-        size2 = bucket2.get(symbol_name)
-        if size2 is None:
+      for symbol_name, symbol_size_list in bucket1.items():
+        size_list2 = bucket2.get(symbol_name)
+        if size_list2 is None:
           # Symbol no longer exists in bucket2.
-          removed.append((key[0], key[1], symbol_name, symbol_size, None))
+          for symbol_size in symbol_size_list:
+            removed.append((key[0], key[1], symbol_name, symbol_size, None))
         else:
           del bucket2[symbol_name] # Symbol is not new, delete from cache2.
+          if len(symbol_size_list) == 1 and len(size_list2) == 1:
+            symbol_size = symbol_size_list[0]
+            size2 = size_list2[0]
+            if symbol_size != size2:
+              # Symbol has change size in bucket.
+              changed.append((key[0], key[1], symbol_name, symbol_size, size2))
+            else:
+              # Symbol is unchanged.
+              unchanged.append((key[0], key[1], symbol_name, symbol_size,
+                                size2))
+          else:
+            # Complex comparison for when a symbol exists multiple times
+            # in the same file (where file can be "unknown file").
+            symbol_size_counter = collections.Counter(symbol_size_list)
+            delta_counter = collections.Counter(symbol_size_list)
+            delta_counter.subtract(size_list2)
+            for symbol_size in sorted(delta_counter.keys()):
+              delta = delta_counter[symbol_size]
+              unchanged_count = symbol_size_counter[symbol_size]
+              if delta > 0:
+                unchanged_count -= delta
+              for _ in range(unchanged_count):
+                unchanged.append((key[0], key[1], symbol_name, symbol_size,
+                                  symbol_size))
+              if delta > 0: # Used to be more of these than there is now.
+                for _ in range(delta):
+                  removed.append((key[0], key[1], symbol_name, symbol_size,
+                                  None))
+              elif delta < 0: # More of this (symbol,size) now.
+                for _ in range(-delta):
+                  added.append((key[0], key[1], symbol_name, None, symbol_size))
+
           if len(bucket2) == 0:
             del cache1[key] # Entire bucket is empty, delete from cache2
-          if symbol_size != size2:
-            # Symbol has change size in bucket.
-            changed.append((key[0], key[1], symbol_name, symbol_size, size2))
-          else:
-            # Symbol is unchanged.
-            unchanged.append((key[0], key[1], symbol_name, symbol_size, size2))
 
   # We have now analyzed all symbols that are in cache1 and removed all of
   # the encountered symbols from cache2. What's left in cache2 is the new
   # symbols.
   for key, bucket2 in cache2.iteritems():
-    for symbol_name, symbol_size in bucket2.items():
-      added.append((key[0], key[1], symbol_name, None, symbol_size))
+    for symbol_name, symbol_size_list in bucket2.items():
+      for symbol_size in symbol_size_list:
+        added.append((key[0], key[1], symbol_name, None, symbol_size))
   return (added, removed, changed, unchanged)
 
 def DeltaStr(number):
diff --git a/tools/binary_size/explain_binary_size_delta_unittest.py b/tools/binary_size/explain_binary_size_delta_unittest.py
index a5f696f..87ecb36 100755
--- a/tools/binary_size/explain_binary_size_delta_unittest.py
+++ b/tools/binary_size/explain_binary_size_delta_unittest.py
@@ -221,5 +221,134 @@ Per-source Analysis:
     print "explain_binary_size_delta_unittest: All tests passed"
 
 
+  def testCompareStringEntries(self):
+    # List entries have form: symbol_name, symbol_type, symbol_size, file_path
+    symbol_list1 = (
+      # File with one string.
+      ( '.L.str107', 'r', 8, '/file_with_strs' ),
+    )
+
+    symbol_list2 = (
+      # Two files with one string each, same name.
+      ( '.L.str107', 'r', 8, '/file_with_strs' ),
+      ( '.L.str107', 'r', 7, '/other_file_with_strs' ),
+    )
+
+    # Here we go
+    (added, removed, changed, unchanged) = \
+        explain_binary_size_delta.Compare(symbol_list1, symbol_list2)
+
+
+    # Now check final stats.
+    orig_stdout = sys.stdout
+    output_collector = cStringIO.StringIO()
+    sys.stdout = output_collector
+    try:
+      explain_binary_size_delta.CrunchStats(added, removed, changed,
+                                            unchanged, True, True)
+    finally:
+      sys.stdout = orig_stdout
+    result = output_collector.getvalue()
+
+    expected_output = """\
+Total change: +7 bytes
+======================
+  1 added, totalling +7 bytes across 1 sources
+  1 unchanged, totalling 8 bytes
+Source stats:
+  2 sources encountered.
+  1 completely new.
+  0 removed completely.
+  0 partially changed.
+  1 completely unchanged.
+Per-source Analysis:
+
+--------------------------------------------------------
+ +7 - Source: /other_file_with_strs - (gained 7, lost 0)
+--------------------------------------------------------
+  New symbols:
+         +7: .L.str107 type=r, size=7 bytes
+"""
+
+    self.maxDiff = None
+    self.assertMultiLineEqual(expected_output, result)
+    print "explain_binary_size_delta_unittest: All tests passed"
+
+  def testCompareStringEntriesWithNoFile(self):
+    # List entries have form: symbol_name, symbol_type, symbol_size, file_path
+    symbol_list1 = (
+      ( '.L.str104', 'r', 21, '??' ), # Will change size.
+      ( '.L.str105', 'r', 17, '??' ), # Same.
+      ( '.L.str106', 'r', 13, '??' ), # Will be removed.
+      ( '.L.str106', 'r', 3, '??' ), # Same.
+      ( '.L.str106', 'r', 3, '??' ), # Will be removed.
+      ( '.L.str107', 'r', 8, '??' ), # Will be removed (other sizes).
+    )
+
+    symbol_list2 = (
+      # Two files with one string each, same name.
+      ( '.L.str104', 'r', 19, '??' ), # Changed.
+      ( '.L.str105', 'r', 11, '??' ), # New size for multi-symbol.
+      ( '.L.str105', 'r', 17, '??' ), # New of same size for multi-symbol.
+      ( '.L.str105', 'r', 17, '??' ), # Same.
+      ( '.L.str106', 'r', 3, '??' ), # Same.
+      ( '.L.str107', 'r', 5, '??' ), # New size for symbol.
+      ( '.L.str107', 'r', 7, '??' ), # New size for symbol.
+      ( '.L.str108', 'r', 8, '??' ), # New symbol.
+    )
+
+    # Here we go
+    (added, removed, changed, unchanged) = \
+        explain_binary_size_delta.Compare(symbol_list1, symbol_list2)
+
+
+    # Now check final stats.
+    orig_stdout = sys.stdout
+    output_collector = cStringIO.StringIO()
+    sys.stdout = output_collector
+    try:
+      explain_binary_size_delta.CrunchStats(added, removed, changed,
+                                            unchanged, True, True)
+    finally:
+      sys.stdout = orig_stdout
+    result = output_collector.getvalue()
+
+    expected_output = """\
+Total change: +22 bytes
+=======================
+  5 added, totalling +48 bytes across 1 sources
+  3 removed, totalling -24 bytes across 1 sources
+  1 shrunk, for a net change of -2 bytes (21 bytes before, 19 bytes after) \
+across 1 sources
+  2 unchanged, totalling 20 bytes
+Source stats:
+  1 sources encountered.
+  0 completely new.
+  0 removed completely.
+  1 partially changed.
+  0 completely unchanged.
+Per-source Analysis:
+
+----------------------------------------
+ +22 - Source: ?? - (gained 48, lost 26)
+----------------------------------------
+  New symbols:
+        +17: .L.str105 type=r, size=17 bytes
+        +11: .L.str105 type=r, size=11 bytes
+         +8: .L.str108 type=r, size=8 bytes
+         +7: .L.str107 type=r, size=7 bytes
+         +5: .L.str107 type=r, size=5 bytes
+  Removed symbols:
+         -3: .L.str106 type=r, size=3 bytes
+         -8: .L.str107 type=r, size=8 bytes
+        -13: .L.str106 type=r, size=13 bytes
+  Shrunk symbols:
+         -2: .L.str104 type=r, (was 21 bytes, now 19 bytes)
+"""
+
+    self.maxDiff = None
+    self.assertMultiLineEqual(expected_output, result)
+    print "explain_binary_size_delta_unittest: All tests passed"
+
 if __name__ == '__main__':
   unittest.main()