V2 of //tools/binary_size rewrite (diffs).

V2 of //tools/binary_size rewrite (diffs).

Renames:
analyze.py -> map2size.py
symbols.py -> models.py
query.py -> console.py
mapfileparser.py -> linker_map_parser.py

Main features:
* Removes explain_binary_size_delta.py in favour of console.py.
* Ability to diff two .size results.
* Many improvements to console.py.
* Added some integration tests.

BUG=681694
Review-Url: https://codereview.chromium.org/2769933002
Cr-Commit-Position: refs/heads/master@{#459878}
Committed: https://chromium.googlesource.com/chromium/src/+/35ed9edd145d324bdec5c10a1f99c5863b93b775

[agrieve, 2017-03-24 02:50:38]

Description was changed from

==========
V2 of //tools/binary_size rewrite (diffs).

* Removes explain_binary_size_delta.py in favour of console.py
  (formally query.py).
* Ability to diff two .size results.
* Many improvements to console.py.
* Renamed a bunch of classes / modules.

BUG=681694
==========

to

==========
V2 of //tools/binary_size rewrite (diffs).

Renames:
analyze.py -> map2size.py
symbols.py -> models.py
query.py -> console.py
mapfileparser.py -> linker_map_parser.py

Main features:
* Removes explain_binary_size_delta.py in favour of console.py.
* Ability to diff two .size results.
* Many improvements to console.py.
* Added some integration tests.

BUG=681694
==========

[agrieve, 2017-03-24 16:43:51]

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

[agrieve, 2017-03-24 16:43:52]

https://codereview.chromium.org/2769933002/diff/40001/tools/binary_size/file_...
File tools/binary_size/file_format.py (right):

https://codereview.chromium.org/2769933002/diff/40001/tools/binary_size/file_...
tools/binary_size/file_format.py:1: # Copyright 2017 The Chromium Authors. All
rights reserved.
Not changes to any logic here. Just moved from analyze.py

https://codereview.chromium.org/2769933002/diff/40001/tools/binary_size/linke...
File tools/binary_size/linker_map_parser.py (right):

https://codereview.chromium.org/2769933002/diff/40001/tools/binary_size/linke...
tools/binary_size/linker_map_parser.py:170: if address_str ==
'0xffffffffffffffff':
Moved this to see whether the gaps were stable against symbol afterwards (vs
before).

They aren't, but I think the logic is nicer here anyways. Only logic change is
naming of the sections by count (so that they diff more nicely).

https://codereview.chromium.org/2769933002/diff/40001/tools/binary_size/model...
File tools/binary_size/models.py (right):

https://codereview.chromium.org/2769933002/diff/40001/tools/binary_size/model...
tools/binary_size/models.py:1: # Copyright 2017 The Chromium Authors. All rights
reserved.
Yikes, there's actually not much changed here. Sad the diff didn't pick up the
rename.

Changes are:
- Dropped include_filtered param (confusing)
- Added __add__, __sub__, SymbolDiff, Diff()
- Made name and path default to '' rather than None
- Converted the __class__() usage to _CreateTransformed(), which allows
SymbolDiff to extend SymbolGroup without having to have their constructors
match.

[estevenson, 2017-03-27 17:50:41]

lgtm!

https://codereview.chromium.org/2769933002/diff/60001/tools/binary_size/conso...
File tools/binary_size/console.py (right):

https://codereview.chromium.org/2769933002/diff/60001/tools/binary_size/conso...
tools/binary_size/console.py:128: 'Variables: %s' % ', '.join(variables),
Most using this tool will know this already, but you could add something like:

For more info:
import models
help(models)

https://codereview.chromium.org/2769933002/diff/60001/tools/binary_size/model...
File tools/binary_size/models.py (right):

https://codereview.chromium.org/2769933002/diff/60001/tools/binary_size/model...
tools/binary_size/models.py:80: self.section_name = section_name
Is this actually necessary to use intern?

https://codereview.chromium.org/2769933002/diff/60001/tools/binary_size/model...
tools/binary_size/models.py:355: old_sym = matching_syms.pop(0)
Curious, when will there be symbols with the same key? What does that mean?

https://codereview.chromium.org/2769933002/diff/60001/tools/binary_size/model...
tools/binary_size/models.py:361: merged_sym.function_signature =
old_sym.function_signature
nit: Might as well make the init have a function_signature='' param (since
that's the default value anyway)?

https://codereview.chromium.org/2769933002/diff/100001/tools/binary_size/READ...
File tools/binary_size/README.md (right):

https://codereview.chromium.org/2769933002/diff/100001/tools/binary_size/READ...
tools/binary_size/README.md:10: tools/binary_size/map2size.py
out/Release/lib.unstripped/libchrome.so.map.gz --output chrome.size -v
Needs to be updated to match map2size.py args. Same with Linux below.

https://codereview.chromium.org/2769933002/diff/100001/tools/binary_size/map2...
File tools/binary_size/map2size.py (right):

https://codereview.chromium.org/2769933002/diff/100001/tools/binary_size/map2...
tools/binary_size/map2size.py:175: tool_prefix =
build_vars['android_tool_prefix']
Path not found:
../../third_party/android_tools/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-c++filt

Now that android_tool_prefix is relative to the out dir
(https://codereview.chromium.org/2761883002/) this fails.

[agrieve, 2017-03-27 19:47:22]

Thanks for the thorough review. You found two real bugs!

https://codereview.chromium.org/2769933002/diff/60001/tools/binary_size/conso...
File tools/binary_size/console.py (right):

https://codereview.chromium.org/2769933002/diff/60001/tools/binary_size/conso...
tools/binary_size/console.py:128: 'Variables: %s' % ', '.join(variables),
On 2017/03/27 17:50:41, estevenson wrote:
> Most using this tool will know this already, but you could add something like:
> 
> For more info:
> import models
> help(models)

Done (and added at least class-level pydoc for models.*)

https://codereview.chromium.org/2769933002/diff/60001/tools/binary_size/model...
File tools/binary_size/models.py (right):

https://codereview.chromium.org/2769933002/diff/60001/tools/binary_size/model...
tools/binary_size/models.py:80: self.section_name = section_name
On 2017/03/27 17:50:41, estevenson wrote:
> Is this actually necessary to use intern?

This "optimization" was indeed causing bugs. Removed in future snapshot.

https://codereview.chromium.org/2769933002/diff/60001/tools/binary_size/model...
tools/binary_size/models.py:355: old_sym = matching_syms.pop(0)
On 2017/03/27 17:50:41, estevenson wrote:
> Curious, when will there be symbols with the same key? What does that mean?

The main place this comes up is ** symbols. E.g. "** merge strings"

Added a comment in the _Key() pydoc.

https://codereview.chromium.org/2769933002/diff/60001/tools/binary_size/model...
tools/binary_size/models.py:361: merged_sym.function_signature =
old_sym.function_signature
On 2017/03/27 17:50:41, estevenson wrote:
> nit: Might as well make the init have a function_signature='' param (since
> that's the default value anyway)?

Done.

https://codereview.chromium.org/2769933002/diff/100001/tools/binary_size/READ...
File tools/binary_size/README.md (right):

https://codereview.chromium.org/2769933002/diff/100001/tools/binary_size/READ...
tools/binary_size/README.md:10: tools/binary_size/map2size.py
out/Release/lib.unstripped/libchrome.so.map.gz --output chrome.size -v
On 2017/03/27 17:50:41, estevenson wrote:
> Needs to be updated to match map2size.py args. Same with Linux below.

Done (good catch!)

https://codereview.chromium.org/2769933002/diff/100001/tools/binary_size/map2...
File tools/binary_size/map2size.py (right):

https://codereview.chromium.org/2769933002/diff/100001/tools/binary_size/map2...
tools/binary_size/map2size.py:175: tool_prefix =
build_vars['android_tool_prefix']
On 2017/03/27 17:50:41, estevenson wrote:
> Path not found:
>
../../third_party/android_tools/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-c++filt
> 
> Now that android_tool_prefix is relative to the out dir
> (https://codereview.chromium.org/2761883002/) this fails.

Done.

[agrieve, 2017-03-27 19:47:31]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2017-03-27 19:47:32]

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/2769933002/#ps120001
(title: "Review comments")

[commit-bot: I haz the power, 2017-03-27 19:48:19]

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

[commit-bot: I haz the power, 2017-03-27 21:13:41]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490644051829680,
"parent_rev": "b20148e73c9d380f84968dac00dae26425d05b78", "commit_rev":
"35ed9edd145d324bdec5c10a1f99c5863b93b775"}

[commit-bot: I haz the power, 2017-03-27 21:14:28]

Description was changed from

==========
V2 of //tools/binary_size rewrite (diffs).

Renames:
analyze.py -> map2size.py
symbols.py -> models.py
query.py -> console.py
mapfileparser.py -> linker_map_parser.py

Main features:
* Removes explain_binary_size_delta.py in favour of console.py.
* Ability to diff two .size results.
* Many improvements to console.py.
* Added some integration tests.

BUG=681694
==========

to

==========
V2 of //tools/binary_size rewrite (diffs).

Renames:
analyze.py -> map2size.py
symbols.py -> models.py
query.py -> console.py
mapfileparser.py -> linker_map_parser.py

Main features:
* Removes explain_binary_size_delta.py in favour of console.py.
* Ability to diff two .size results.
* Many improvements to console.py.
* Added some integration tests.

BUG=681694

Review-Url: https://codereview.chromium.org/2769933002
Cr-Commit-Position: refs/heads/master@{#459878}
Committed:
https://chromium.googlesource.com/chromium/src/+/35ed9edd145d324bdec5c10a1f99...
==========

[commit-bot: I haz the power, 2017-03-27 21:14:30]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/35ed9edd145d324bdec5c10a1f99...

[agrieve, 2017-03-28 13:50:25]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2778963003/ by agrieve@chromium.org.

The reason for reverting is: Typo'ed in final set of review comments :(.