Handle shared memory symbols better in the binarysize tool.

Handle shared memory symbols better in the binarysize tool.

The linker can let two symbols share the same memory space and then
it is wrong to count that memory space twice. Better to let each symbol
contribute with a proportional part of that symbol.

This only affects the explain_binary_size_delta program. The graphical
treeview will still use the full symbol size since it's valuable
information in that context.

BUG=

Committed: https://crrev.com/5ec91b1d40fb6174d43d5f5cc84209b9a4505b6b
Cr-Commit-Position: refs/heads/master@{#316844}

[Daniel Bratell, 2014-07-21 15:01:12]

I was a bit worried that I had made some bad decisions
because of this bug, but it seems the shared symbols
are mostly small, rather meaningless, ones.

[Daniel Bratell, 2014-09-23 15:35:31]

andrewhayden, can you take a look at this? Without it there is a risk of over
estimating the size contribution of a symbol.

[Daniel Bratell, 2015-02-16 12:17:49]

ping

[Andrew Hayden (chromium.org), 2015-02-16 12:47:38]

Sorry for the delay. This looks good overall, my only potential issue is with
the way that we dole out the size attribution to everything round-robin with no
visibility to the end user of the tool. I think it would be nice if we could
"star" ('*') the results that are like this or something, so that it is clearer
to the user why the size attribution is "weird" for certain symbols.

Thank you for the continued attention to detail!

https://codereview.chromium.org/397593007/diff/20001/tools/binary_size/explai...
File tools/binary_size/explain_binary_size_delta.py (right):

https://codereview.chromium.org/397593007/diff/20001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:52: """Checks how many symbols
share the same memory space."""
Please document the return type.

https://codereview.chromium.org/397593007/diff/20001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:57: # metacount = Counter()
Please remove the commented lines 57-61 or uncomment and hide them behind a
debug/verbose flag.

https://codereview.chromium.org/397593007/diff/20001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:94: effective_symbol_size =
int(ceil(symbol_size / float(shared_count)))
For clarity, I'd prefer that we turn this line into a function like
"CalculateEffectiveSize" or something that we can potentially tweak (or at least
serves as a good way to document what we're trying to do here - there could be
other, better strategies that aren't just an equal distribution).

https://codereview.chromium.org/397593007/diff/20001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:204: assert not '&' in file_path,
"File path: " + file_path
Is there some context for this being added?

https://codereview.chromium.org/397593007/diff/20001/tools/binary_size/explai...
File tools/binary_size/explain_binary_size_delta_unittest.py (right):

https://codereview.chromium.org/397593007/diff/20001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta_unittest.py:15: class
ExplainBinarySizeDeltaTest(unittest.TestCase):
Can you add a test that exercises your new effective-size logic?

[Daniel Bratell, 2015-02-18 13:31:53]

A bit tricky to get the information that sizes were adjusted to compensate for
sharing all the way to the output but a good suggestion.

To keep the code readable I had to replace the tuples with a custom class. It
made the code a little bit longer but also had the benefit that the magic [1]
and similar disappeared from the code.

But please take a new look.

https://codereview.chromium.org/397593007/diff/20001/tools/binary_size/explai...
File tools/binary_size/explain_binary_size_delta.py (right):

https://codereview.chromium.org/397593007/diff/20001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:52: """Checks how many symbols
share the same memory space."""
On 2015/02/16 12:47:38, Andrew Hayden wrote:
> Please document the return type.

Done.

https://codereview.chromium.org/397593007/diff/20001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:57: # metacount = Counter()
On 2015/02/16 12:47:38, Andrew Hayden wrote:
> Please remove the commented lines 57-61 or uncomment and hide them behind a
> debug/verbose flag.

Done.

https://codereview.chromium.org/397593007/diff/20001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:94: effective_symbol_size =
int(ceil(symbol_size / float(shared_count)))
On 2015/02/16 12:47:38, Andrew Hayden wrote:
> For clarity, I'd prefer that we turn this line into a function like
> "CalculateEffectiveSize" or something that we can potentially tweak (or at
least
> serves as a good way to document what we're trying to do here - there could be
> other, better strategies that aren't just an equal distribution).

Done.

https://codereview.chromium.org/397593007/diff/20001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:204: assert not '&' in file_path,
"File path: " + file_path
On 2015/02/16 12:47:38, Andrew Hayden wrote:
> Is there some context for this being added?

I really can't remember. Probably remains from some debugging. I will remove it.

https://codereview.chromium.org/397593007/diff/20001/tools/binary_size/explai...
File tools/binary_size/explain_binary_size_delta_unittest.py (right):

https://codereview.chromium.org/397593007/diff/20001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta_unittest.py:15: class
ExplainBinarySizeDeltaTest(unittest.TestCase):
On 2015/02/16 12:47:38, Andrew Hayden wrote:
> Can you add a test that exercises your new effective-size logic?

Done.

[Andrew Hayden (chromium.org), 2015-02-18 13:48:24]

LGTM (looks great to me). Thanks very much for this contribution!

https://codereview.chromium.org/397593007/diff/40001/tools/binary_size/explai...
File tools/binary_size/explain_binary_size_delta_unittest.py (right):

https://codereview.chromium.org/397593007/diff/40001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta_unittest.py:606: print
"explain_binary_size_delta_unittest: All tests passed"
I have no idea why the first "All tests passed" got propagated into the other
methods that were already here but would you mind getting rid of them?

[Daniel Bratell, 2015-02-18 13:53:05]

New patchsets have been uploaded after l-g-t-m from andrewhayden@chromium.org

[Daniel Bratell, 2015-02-18 13:57:03]

> I have no idea why the first "All tests passed" got propagated into the other
> methods that were already here but would you mind getting rid of them?

I wonder if it could be copy-paste... Nah, never. :-)

Fixed now.

[Daniel Bratell, 2015-02-18 16:39:38]

The CQ bit was checked by bratell@opera.com

[commit-bot: I haz the power, 2015-02-18 16:40:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/397593007/60001

[commit-bot: I haz the power, 2015-02-18 16:46:27]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-18 16:47:32]

Patchset 4 (id:??) landed as
https://crrev.com/5ec91b1d40fb6174d43d5f5cc84209b9a4505b6b
Cr-Commit-Position: refs/heads/master@{#316844}