supersize: Track symbol aliases and shared symbols

supersize: Track symbol aliases and shared symbols

An alias is when multiple names map to the same symbol. This happens a
lot with small member functions (e.g. getters), as well as with C++
templates. The .map file choses only one name for each symbol, but using
nm we can recover the complete list.

A shared symbol is one that exists in multiple .o files. The linker map
picks one path to attribute the symbol to, but using nm we can recover
the complete list of .o files containing the symbol.

For aliases, each one is stored as a separate Symbol entry, and a new
attribute "pss" returns size / num_aliases.

For shared symbols, rather than storing all paths for a symbol, the
paths are altered to be the common ancestor. E.g.:
/a/b/c.o and /a/d/f.o --> /a/{shared}/2  (2 is the number of paths)

For ChromePublic.apk:
* 17717 symbols have shared ownership (991402 bytes)
* Contains 83869 aliases, mapped to 9209 addresses (550582 bytes)

BUG=708865
Review-Url: https://codereview.chromium.org/2851473003
Cr-Commit-Position: refs/heads/master@{#468362}
Committed: https://chromium.googlesource.com/chromium/src/+/e2db5397b49842931f2f2a6b2c666c2a8892fd89

[agrieve, 2017-04-27 18:44:48]

Description was changed from

==========
supersize: Track symbol aliases and shared symbols

An alias is when multiple names map to the same symbol. This happens a
lot with small member functions (e.g. getters), as well as with C++
templates. The .map file choses only one name for each symbol, but using
nm we can recover the complete list.

A shared symbol is one that exists in multiple .o files. The linker map
picks one path to attribute the symbol to, but using nm we can recover
the complete list of .o files containing the symbol.

For aliases, each one is stored as a separate Symbol entry, and a new
attribute "pss" returns size / num_aliases.

For shared symbols, rather than storing all paths for a symbol, the
paths are altered to be the common ancestor. E.g.:
/a/b/c.o and /a/d/f.o --> /a/{shared}/2  (2 is the number of paths)

For ChromePublic.apk:
* 17717 symbols have shared ownership (991402 bytes)
* Contains 83869 aliases, mapped to 9209 addresses (550582 bytes)

BUG=708865
==========

to

==========
supersize: Track symbol aliases and shared symbols

An alias is when multiple names map to the same symbol. This happens a
lot with small member functions (e.g. getters), as well as with C++
templates. The .map file choses only one name for each symbol, but using
nm we can recover the complete list.

A shared symbol is one that exists in multiple .o files. The linker map
picks one path to attribute the symbol to, but using nm we can recover
the complete list of .o files containing the symbol.

For aliases, each one is stored as a separate Symbol entry, and a new
attribute "pss" returns size / num_aliases.

For shared symbols, rather than storing all paths for a symbol, the
paths are altered to be the common ancestor. E.g.:
/a/b/c.o and /a/d/f.o --> /a/{shared}/2  (2 is the number of paths)

For ChromePublic.apk:
* 17717 symbols have shared ownership (991402 bytes)
* Contains 83869 aliases, mapped to 9209 addresses (550582 bytes)

BUG=708865
==========

[agrieve, 2017-04-27 18:44:49]

agrieve@chromium.org changed reviewers:
+ estevenson@chromium.org

[agrieve, 2017-04-27 18:45:00]

On 2017/04/27 18:44:49, agrieve wrote:
> mailto:agrieve@chromium.org changed reviewers:
> + mailto:estevenson@chromium.org

🚢

[estevenson, 2017-04-28 17:06:11]

Sorry for the delay, lgtm

https://codereview.chromium.org/2851473003/diff/20001/tools/binary_size/libsu...
File tools/binary_size/libsupersize/archive.py (right):

https://codereview.chromium.org/2851473003/diff/20001/tools/binary_size/libsu...
tools/binary_size/libsupersize/archive.py:62: def _UnmangleNames(names,
tool_prefix):
revert this part

https://codereview.chromium.org/2851473003/diff/20001/tools/binary_size/libsu...
tools/binary_size/libsupersize/archive.py:284: prev_is_padding_only =
prev_symbol.size_without_padding == 0
This would be better to check that the symbol is in symbol.aliases

https://codereview.chromium.org/2851473003/diff/20001/tools/binary_size/libsu...
File tools/binary_size/libsupersize/diff.py (right):

https://codereview.chromium.org/2851473003/diff/20001/tools/binary_size/libsu...
tools/binary_size/libsupersize/diff.py:52: ret.aliases = [ret]
Already forgot - what does this mean again?

https://codereview.chromium.org/2851473003/diff/20001/tools/binary_size/libsu...
File tools/binary_size/libsupersize/integration_test.py (right):

https://codereview.chromium.org/2851473003/diff/20001/tools/binary_size/libsu...
tools/binary_size/libsupersize/integration_test.py:49: golden_path =
os.path.join(_TEST_DATA_DIR, basename+ '.golden')
nit: space after basename.

https://codereview.chromium.org/2851473003/diff/20001/tools/binary_size/libsu...
File tools/binary_size/libsupersize/nm.py (right):

https://codereview.chromium.org/2851473003/diff/20001/tools/binary_size/libsu...
tools/binary_size/libsupersize/nm.py:27: if section not in 'tT' or not name or
name[0] == '$':
should readonly symbols be included in this?

https://codereview.chromium.org/2851473003/diff/20001/tools/binary_size/libsu...
tools/binary_size/libsupersize/nm.py:30: address = int(address_str, 16) &
0xfffffffffffffffe
This needs a comment

[agrieve, 2017-04-28 19:26:59]

https://codereview.chromium.org/2851473003/diff/20001/tools/binary_size/libsu...
File tools/binary_size/libsupersize/archive.py (right):

https://codereview.chromium.org/2851473003/diff/20001/tools/binary_size/libsu...
tools/binary_size/libsupersize/archive.py:62: def _UnmangleNames(names,
tool_prefix):
On 2017/04/28 17:06:11, estevenson wrote:
> revert this part

Done.

https://codereview.chromium.org/2851473003/diff/20001/tools/binary_size/libsu...
tools/binary_size/libsupersize/archive.py:284: prev_is_padding_only =
prev_symbol.size_without_padding == 0
On 2017/04/28 17:06:11, estevenson wrote:
> This would be better to check that the symbol is in symbol.aliases

Done.

https://codereview.chromium.org/2851473003/diff/20001/tools/binary_size/libsu...
File tools/binary_size/libsupersize/diff.py (right):

https://codereview.chromium.org/2851473003/diff/20001/tools/binary_size/libsu...
tools/binary_size/libsupersize/diff.py:52: ret.aliases = [ret]
On 2017/04/28 17:06:11, estevenson wrote:
> Already forgot - what does this mean again?

Comment added.

https://codereview.chromium.org/2851473003/diff/20001/tools/binary_size/libsu...
File tools/binary_size/libsupersize/integration_test.py (right):

https://codereview.chromium.org/2851473003/diff/20001/tools/binary_size/libsu...
tools/binary_size/libsupersize/integration_test.py:49: golden_path =
os.path.join(_TEST_DATA_DIR, basename+ '.golden')
On 2017/04/28 17:06:11, estevenson wrote:
> nit: space after basename.

Done.

https://codereview.chromium.org/2851473003/diff/20001/tools/binary_size/libsu...
File tools/binary_size/libsupersize/nm.py (right):

https://codereview.chromium.org/2851473003/diff/20001/tools/binary_size/libsu...
tools/binary_size/libsupersize/nm.py:27: if section not in 'tT' or not name or
name[0] == '$':
On 2017/04/28 17:06:11, estevenson wrote:
> should readonly symbols be included in this?

Added a comment. Looks like no.

https://codereview.chromium.org/2851473003/diff/20001/tools/binary_size/libsu...
tools/binary_size/libsupersize/nm.py:30: address = int(address_str, 16) &
0xfffffffffffffffe
On 2017/04/28 17:06:11, estevenson wrote:
> This needs a comment

Turns out it wasn't needed :P

[agrieve, 2017-04-28 19:30:27]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2017-04-28 19:30:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
estevenson@chromium.org
Link to the patchset: https://codereview.chromium.org/2851473003/#ps40001
(title: "review comments")

[commit-bot: I haz the power, 2017-04-28 19:31:33]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[agrieve, 2017-04-28 19:40:32]

The CQ bit was unchecked by agrieve@chromium.org

[agrieve, 2017-04-28 19:52:26]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2017-04-28 19:52:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
estevenson@chromium.org
Link to the patchset: https://codereview.chromium.org/2851473003/#ps60001
(title: "review comments")

[commit-bot: I haz the power, 2017-04-28 19:53:39]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[agrieve, 2017-04-28 19:57:32]

The CQ bit was unchecked by agrieve@chromium.org

[agrieve, 2017-04-28 20:30:55]

On 2017/04/28 19:53:39, commit-bot: I haz the power wrote:
> CQ is trying da patch.
> 
> Follow status at:
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

Holding off on this atm. Tried it out on an actual perf alert and the diff looks
wrong.

[estevenson, 2017-04-28 20:34:08]

On 2017/04/28 20:30:55, agrieve wrote:
> On 2017/04/28 19:53:39, commit-bot: I haz the power wrote:
> > CQ is trying da patch.
> > 
> > Follow status at:
> >
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> 
> Holding off on this atm. Tried it out on an actual perf alert and the diff
looks
> wrong.

I missed the diagnose_apk_bloat.py change - I think the order is wrong for size
files there, is that it?

[agrieve, 2017-05-01 17:12:59]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2017-05-01 17:13:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
estevenson@chromium.org
Link to the patchset: https://codereview.chromium.org/2851473003/#ps80001
(title: "fix regression in calculate padding introduced in ps3")

[commit-bot: I haz the power, 2017-05-01 17:13:38]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[agrieve, 2017-05-01 17:14:04]

On 2017/04/28 20:34:08, estevenson wrote:
> On 2017/04/28 20:30:55, agrieve wrote:
> > On 2017/04/28 19:53:39, commit-bot: I haz the power wrote:
> > > CQ is trying da patch.
> > > 
> > > Follow status at:
> > >
> >
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> > 
> > Holding off on this atm. Tried it out on an actual perf alert and the diff
> looks
> > wrong.
> 
> I missed the diagnose_apk_bloat.py change - I think the order is wrong for
size
> files there, is that it?

Yeah, the diagnose_apk_bloat.py change wasn't there in the initial patchset, and
was just me trying to fix the numbers being wrong. I've reverted it now though.
ps5 shows what the actual bug was (regression when going through review comments
:P)

[commit-bot: I haz the power, 2017-05-01 18:34:28]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493658779638640,
"parent_rev": "66a9a083d5158d54b7db7aecd60175fb96e6dc0b", "commit_rev":
"e2db5397b49842931f2f2a6b2c666c2a8892fd89"}

[commit-bot: I haz the power, 2017-05-01 18:34:40]

Description was changed from

==========
supersize: Track symbol aliases and shared symbols

An alias is when multiple names map to the same symbol. This happens a
lot with small member functions (e.g. getters), as well as with C++
templates. The .map file choses only one name for each symbol, but using
nm we can recover the complete list.

A shared symbol is one that exists in multiple .o files. The linker map
picks one path to attribute the symbol to, but using nm we can recover
the complete list of .o files containing the symbol.

For aliases, each one is stored as a separate Symbol entry, and a new
attribute "pss" returns size / num_aliases.

For shared symbols, rather than storing all paths for a symbol, the
paths are altered to be the common ancestor. E.g.:
/a/b/c.o and /a/d/f.o --> /a/{shared}/2  (2 is the number of paths)

For ChromePublic.apk:
* 17717 symbols have shared ownership (991402 bytes)
* Contains 83869 aliases, mapped to 9209 addresses (550582 bytes)

BUG=708865
==========

to

==========
supersize: Track symbol aliases and shared symbols

An alias is when multiple names map to the same symbol. This happens a
lot with small member functions (e.g. getters), as well as with C++
templates. The .map file choses only one name for each symbol, but using
nm we can recover the complete list.

A shared symbol is one that exists in multiple .o files. The linker map
picks one path to attribute the symbol to, but using nm we can recover
the complete list of .o files containing the symbol.

For aliases, each one is stored as a separate Symbol entry, and a new
attribute "pss" returns size / num_aliases.

For shared symbols, rather than storing all paths for a symbol, the
paths are altered to be the common ancestor. E.g.:
/a/b/c.o and /a/d/f.o --> /a/{shared}/2  (2 is the number of paths)

For ChromePublic.apk:
* 17717 symbols have shared ownership (991402 bytes)
* Contains 83869 aliases, mapped to 9209 addresses (550582 bytes)

BUG=708865

Review-Url: https://codereview.chromium.org/2851473003
Cr-Commit-Position: refs/heads/master@{#468362}
Committed:
https://chromium.googlesource.com/chromium/src/+/e2db5397b49842931f2f2a6b2c66...
==========

[commit-bot: I haz the power, 2017-05-01 18:34:41]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/e2db5397b49842931f2f2a6b2c66...