New issue
Advanced search Search tips

Issue 776032 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

supersize should create aliases for inline symbols rather than truncating paths

Project Member Reported by agrieve@chromium.org, Oct 18 2017

Issue description

If a symbol is found in multiple object files, supersize will change its object path to be the common ancestor of all paths it appeared in.

This is a problem because it would causes queries based on path to not find these symbols.

There are currently ~1.5mb worth of symbols that have their paths truncated, and are thus not properly accounted for by path-based breakdowns.

Rather than truncating the paths, supersize should create symbol aliases. However, we'll need to be crazy not to create too many aliases (e.g. for things like std::string that are used in 100s of files, probably not meaningful to record each and every one).
 
Project Member

Comment 1 by bugdroid1@chromium.org, Oct 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c36863be7d5d11c8f2989fc4e88d678fe75a3e03

commit c36863be7d5d11c8f2989fc4e88d678fe75a3e03
Author: Andrew Grieve <agrieve@chromium.org>
Date: Thu Oct 19 20:57:49 2017

Supersize: Create aliases for inline symbols

If a symbol was found in multiple object files, we would previously have
changed its object path to be the common ancestor of all paths it
appeared in. This is a problem because it would cause queries based on
path to not find these symbols.

On the flip side, we were creating string literal aliases for every
path a string literal appeared in. For small strings such as "", or
"\n", this would lead to an unreasonable number of aliases.

This changes supersize to first create symbols for all paths, and then
collapse groups with a large number of aliases into a single symbol with
ancestor paths.

Before this change:
.text: 48998 symbols have shared ownership (1553806 bytes)
.rodata: 541 symbols have shared ownership (285728 bytes)

After:
.text: 1586 symbols have shared ownership (48252 bytes)
.rodata: 141 symbols have shared ownership (2828 bytes)

This increases the symbol count 702115->912322 and the file size
9.67mb->10.39mb.

This increases the runtime on my machine from ~40s -> ~42s.

Bug:  776032 
Change-Id: I457361762b1f241f114b52464812e6035e881538
Reviewed-on: https://chromium-review.googlesource.com/726460
Commit-Queue: agrieve <agrieve@chromium.org>
Reviewed-by: Samuel Huang <huangs@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510205}
[modify] https://crrev.com/c36863be7d5d11c8f2989fc4e88d678fe75a3e03/tools/binary_size/README.md
[modify] https://crrev.com/c36863be7d5d11c8f2989fc4e88d678fe75a3e03/tools/binary_size/libsupersize/archive.py
[modify] https://crrev.com/c36863be7d5d11c8f2989fc4e88d678fe75a3e03/tools/binary_size/libsupersize/file_format.py
[modify] https://crrev.com/c36863be7d5d11c8f2989fc4e88d678fe75a3e03/tools/binary_size/libsupersize/integration_test.py
[modify] https://crrev.com/c36863be7d5d11c8f2989fc4e88d678fe75a3e03/tools/binary_size/libsupersize/nm.py
[modify] https://crrev.com/c36863be7d5d11c8f2989fc4e88d678fe75a3e03/tools/binary_size/libsupersize/testdata/Archive_Elf.golden
[modify] https://crrev.com/c36863be7d5d11c8f2989fc4e88d678fe75a3e03/tools/binary_size/libsupersize/testdata/Console.golden
[modify] https://crrev.com/c36863be7d5d11c8f2989fc4e88d678fe75a3e03/tools/binary_size/libsupersize/testdata/Csv.golden
[modify] https://crrev.com/c36863be7d5d11c8f2989fc4e88d678fe75a3e03/tools/binary_size/libsupersize/testdata/Diff_NullDiff.golden
[modify] https://crrev.com/c36863be7d5d11c8f2989fc4e88d678fe75a3e03/tools/binary_size/libsupersize/testdata/FullDescription.golden
[modify] https://crrev.com/c36863be7d5d11c8f2989fc4e88d678fe75a3e03/tools/binary_size/libsupersize/testdata/SymbolGroupMethods.golden
[modify] https://crrev.com/c36863be7d5d11c8f2989fc4e88d678fe75a3e03/tools/binary_size/libsupersize/testdata/mock_toolchain/mock_nm.py

Status: Fixed (was: Started)
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5e2d1bfcb9e8956a7e82ce8c6d262c5736ab30b6

commit 5e2d1bfcb9e8956a7e82ce8c6d262c5736ab30b6
Author: Andrew Grieve <agrieve@chromium.org>
Date: Mon Oct 23 19:35:01 2017

Supersize: Don't create two aliases for every constructor

This bug was recently introduced by:
c36863be7d5d11c8f2989fc4e88d678fe75a3e03

Multiple symbols were being created for most constructors.

Bug:  776032 
Change-Id: I8f5b0cfc7f1f0f673942d353b3191b1e4b224722
Reviewed-on: https://chromium-review.googlesource.com/733900
Commit-Queue: agrieve <agrieve@chromium.org>
Reviewed-by: Samuel Huang <huangs@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510874}
[modify] https://crrev.com/5e2d1bfcb9e8956a7e82ce8c6d262c5736ab30b6/tools/binary_size/libsupersize/nm.py
[modify] https://crrev.com/5e2d1bfcb9e8956a7e82ce8c6d262c5736ab30b6/tools/binary_size/libsupersize/testdata/mock_toolchain/mock_nm.py

Project Member

Comment 4 by bugdroid1@chromium.org, Oct 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/18bc8502b61bc45f17b1af4a2bead3e292359745

commit 18bc8502b61bc45f17b1af4a2bead3e292359745
Author: Andrew Grieve <agrieve@chromium.org>
Date: Tue Oct 24 03:22:03 2017

Supersize: Make Diff() smarter about aliases and string literals.

Before, adding a single string literal to a file would cause diffs to
say that all following literals have changed sizes. Now, it agressively
looks for symbols of the same name+size to match up, before falling back
on matching up symbols of different sizes.

This also changes the clustering of .symbols for diffs to group by path
aliases. It's useful to have path aliases ungrouped when querying by
path, but for symbol diffs, their low PSS cause inline symbols to appear
as many small symbols, rather than one symbol that just happened to be
defined in a header. Grouping by aliases of the same name fixes this for
diffs by having these symbols merged back into a single group.

Bug:  746761 , 776032 
Change-Id: Id871b5cc1e3aeb6d86d9e864088633a8d5a2f6fd
Reviewed-on: https://chromium-review.googlesource.com/731187
Commit-Queue: agrieve <agrieve@chromium.org>
Reviewed-by: Samuel Huang <huangs@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511028}
[modify] https://crrev.com/18bc8502b61bc45f17b1af4a2bead3e292359745/tools/binary_size/libsupersize/console.py
[modify] https://crrev.com/18bc8502b61bc45f17b1af4a2bead3e292359745/tools/binary_size/libsupersize/describe.py
[modify] https://crrev.com/18bc8502b61bc45f17b1af4a2bead3e292359745/tools/binary_size/libsupersize/diff.py
[modify] https://crrev.com/18bc8502b61bc45f17b1af4a2bead3e292359745/tools/binary_size/libsupersize/models.py
[modify] https://crrev.com/18bc8502b61bc45f17b1af4a2bead3e292359745/tools/binary_size/libsupersize/testdata/Console.golden
[modify] https://crrev.com/18bc8502b61bc45f17b1af4a2bead3e292359745/tools/binary_size/libsupersize/testdata/Diff_Basic.golden
[modify] https://crrev.com/18bc8502b61bc45f17b1af4a2bead3e292359745/tools/binary_size/libsupersize/testdata/Diff_NullDiff.golden

Sign in to add a comment