Graphical version of the run_binary_size_analysis tool.

Graphical version of the run_binary_size_analysis tool.

The binary_size tool suit includes tools that are useful when trying
to reduce binary size of a program, and chromium related programs
in particular.

This commit (mostly written by andrewhayden@chromium.org for 
Android but ported to generic Linux by bratell@opera.com) adds 
a graphical HTML based output for run_binary_size_analysis.py.
In the generated web page it is possible to dynamically and 
graphically browse the binary and each part of the source tree 
is given a size that reflects its contribution to the binary size.

The run_binary_size_analysis tool is run on compiled binaries 
with symbols and uses nm and addr2line to map parts of the
binary to source code. Since addr2line is slow the operation to map 
binary symbols to source files takes a while but the output is 
well worth it when shrinking programs. See its usage information
for details about how to run it.

This commit also includes the tool explain_binary_size_delta.py 
(textual output) which can be used to understand why a binary 
changed size and in what way. See its usage information for 
details about how to run it.

There are many further improvements possible to to do on these tools.
Search the bug database for Label:Tools-BinarySize for suggestions.

BUG=339059
R=primiano@chromium.org,andrewhayden@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272255

[Daniel Bratell, 2014-05-20 13:26:40]

I think it might be time to get this into the chromium tree rather than it just
hanging around in private branches. What do you think?

[Andrew Hayden (chromium.org), 2014-05-20 15:10:50]

Agreed, and a HUGE thanks for doing the work to clean up the Java code. Let's
get this checked in and iteratively improve it. LGTM!

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

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:372: Runs a spatial analysis on a
given library, looking up the source locations
This stuff is out of date. As you can tell, I started by copying and pasting
from the binary size tool to create this file. Let me provide some cleaned up
text:

Analyzes the symbolic differences between two binary files (typically, not not
necessarily, two different builds of the same library) and produces a detailed
description of symbols that have been added, removed, or whose size has changed.

[Primiano Tucci (use gerrit), 2014-05-20 15:22:53]

Not very familiar with the binary_size stuff, but some python-related drive-by.

Note: I'd move the nm-related parser in a separate python module,
binary_size_analysis would look probably nicer to read :)

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/binary...
File tools/binary_size/binary_size_utils.py (right):

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/binary...
tools/binary_size/binary_size_utils.py:10: 
Nit: add an extra newline here

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/binary...
tools/binary_size/binary_size_utils.py:14: Argument: an iterable over lines of
nm output.
Nit: the format should look like: this

Parse nm output (Skipping BSS).

Args:
    input: an iterable over lines of nm output.

Yields:
    (symbol name, symbol type, symbol size, source file path)


*
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Commen...

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/binary...
tools/binary_size/binary_size_utils.py:27: addr_re = re.compile(r'^[0-9a-f]{8,}
(.) ([^\t]+)(?:\t.*)?$')
Hmm what is the sense of the last non capturing group (?:\t.*)?
Can't you just .*?

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/binary...
tools/binary_size/binary_size_utils.py:46: type, sym = match.groups()[0:2]
Uhm I'm missing something? why you get type and sym here if then you just skip
without yielding anything?

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/binary...
tools/binary_size/binary_size_utils.py:53: # external or weak symbol
continue  # external or weak symbol
(save one line?) ;)
Ditto below

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/binary...
tools/binary_size/binary_size_utils.py:60: print >>sys.stderr, 'unparsed:',
repr(line)
What about using logging.error()? 
Most of the code that we have strives to use logging which is easier to reroute
when building things up.

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

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:1: #!/usr/bin/python
#!/usr/bin/env python ?
The difference is that the former (/usr/bin/python) doesn't give the possibility
to users to use a different version of python.
For instance on my laptop I've an ancient version of python under /usr/bin which
is required by all the corp scripts (so I can't update that), and I have a side
(and more recent) installation of python in /usr/local/somewhere.
When I do python developement I put /usr/local/somewhere in the beginning of my
PATH.
If you use env python, that will use the right version (the one I expect in
/usr/local/somewhere).
If you use /usr/bin/python, you will force to use the default system one,
irrespectful of whatever the user puts in its PATH.

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:6: """Generate a description of
the size differences between two binaries
The first line of a docstring should fit in one line (see PEP 257),
it should be something like:

"""Diff symbol sizes of two binaries.

This tool.....

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:11: binary that you want to
anaylze. You can obtain the necessary dumps by
Out of curiosity, why you need the source?
Isn't the gigantic elf with debug symbols enough?

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:48: 
Nit, add an extra line (two lines between top levels)

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:49: def compare(symbols1,
symbols2):
Nit: Compare (capital case)
Also top level functions require a docstring.

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:68: if not bucket:
lines 68.70 can be just:
bucket = cache.setdefault(key, {})

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:77: for key, bucket1 in
cache1.items():
s/items/iteritems/

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:78: bucket2 = cache2.get(key,
None)
", None" is  redundant, the default value for the 2nd arg is already None :)
Ditto on line 86

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:104: for key, bucket2 in
cache2.items():
s/items/iteritems/ here and below

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:107: return [added, removed,
changed, unchanged]
Shouldn't this be a tuple rather than a list?
You know a priori the number of the arguments and their position has a strong
semantic (the first one is the "added", 2nd one "removed" etc).
They're not just a collection of the same thing.

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:110: def test_compare():
Many thanks for the test, yay! :)
Just, we typically put the tests in a side-file name after the actual source.
In more concrete words, please move this to
explain_binary_size_delta_unittest.py

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:174: assert ('/file_unchanged',
't', 'unchanged', 1000, 1000) in unchanged
Also, you might want to take a look to python unittest, and using
"self.assertIn" in cases like this
(you can see
src/tools/memory_inspector/memory_inspector/classification/mmap_classifier_unittest.py)
for an example

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:219: 
Nit: two lines betwen top levels

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:220: def crunchStats(added,
removed, changed, unchanged, showsources, showsymbols):
Nit: casing s/crunch/Crunch/ + docstring

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:228: bucket =
new_symbols_by_path.get(file_path, None)
s/, None//

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:231:
new_symbols_by_path[file_path] = bucket
229-231:  bucket = new_symbols_by_path.setdefault(file_path, [])

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:233: print('  ' + str(len(added))
+ ' added, totalling ' +
Use string formatting?
i.e. print('  %d added, totalling %d bytes across...'  % (len(added), ...)

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:244: if not bucket:
ditto

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:261: if not bucket:
ditto (setdefault) here and below

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:265: print('  ' +
str(len(changed)) + ' changed, resulting in a net change of ' +
ditto (string format) here and below

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/run_bi...
File tools/binary_size/run_binary_size_analysis.py (right):

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/run_bi...
tools/binary_size/run_binary_size_analysis.py:1: #!/usr/bin/python
see my previous comment, on env vs /usr/bin/python

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/run_bi...
tools/binary_size/run_binary_size_analysis.py:63: def _MkChild(node, name):
_MkChild looks a "long"cut for
return node.setdefault(name, {'n': name, 'children': {}})

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/run_bi...
tools/binary_size/run_binary_size_analysis.py:84: 
Nit: add extra newline

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/run_bi...
tools/binary_size/run_binary_size_analysis.py:137: print('Symbols lack paths.
Data will not be structured.')
shouldn't all these prints be logging.warning/error statements?

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/run_bi...
tools/binary_size/run_binary_size_analysis.py:285: out.write('var tree_data = '
+ json_string)
I've no idea how big this string is going to be.
If it's large, is going to be much more efficient doing:
with open(outfile, 'w') as out:
  out.write('var tree_data = ')
  json.dump(tree_root, out)

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/run_bi...
tools/binary_size/run_binary_size_analysis.py:497: def run_nm(binary,
nm_binary):
nit: RunNm

[Primiano Tucci (use gerrit), 2014-05-20 16:20:23]

Ah, also please mind the commit message!
"Random changes to make binary_size run for me" does't sound like something we
want in the commit log :)

[Daniel Bratell, 2014-05-21 08:42:12]

Updated source and commented on comments. Please take another look to see that I
didn't screw anything up. :-)

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/binary...
File tools/binary_size/binary_size_utils.py (right):

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/binary...
tools/binary_size/binary_size_utils.py:10: 
On 2014/05/20 15:22:57, Primiano Tucci wrote:
> Nit: add an extra newline here

Done.

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/binary...
tools/binary_size/binary_size_utils.py:14: Argument: an iterable over lines of
nm output.
On 2014/05/20 15:22:57, Primiano Tucci wrote:
> Nit: the format should look like: this
> 
> Parse nm output (Skipping BSS).
> 
> Args:
>     input: an iterable over lines of nm output.
> 
> Yields:
>     (symbol name, symbol type, symbol size, source file path)
> 
> 
> *
>
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Commen...

Done.

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/binary...
tools/binary_size/binary_size_utils.py:27: addr_re = re.compile(r'^[0-9a-f]{8,}
(.) ([^\t]+)(?:\t.*)?$')
On 2014/05/20 15:22:57, Primiano Tucci wrote:
> Hmm what is the sense of the last non capturing group (?:\t.*)?
> Can't you just .*?

I suspect it is to document the format and reject lines that don't fit what we
expect. I've not looked much at this code except to make it 64 bit compatible.

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/binary...
tools/binary_size/binary_size_utils.py:46: type, sym = match.groups()[0:2]
On 2014/05/20 15:22:57, Primiano Tucci wrote:
> Uhm I'm missing something? why you get type and sym here if then you just skip
> without yielding anything?

For documentation I guess. I'm making it a comment.

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

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:11: binary that you want to
anaylze. You can obtain the necessary dumps by
On 2014/05/20 15:22:57, Primiano Tucci wrote:
> Out of curiosity, why you need the source?
> Isn't the gigantic elf with debug symbols enough?

I don't know for sure, but we've talked about complementing the imperfect output
from addr2line with information from the source tree. Maybe that is what Andrew
is thinking about here? I don't believe it's currently true though.

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:77: for key, bucket1 in
cache1.items():
On 2014/05/20 15:22:57, Primiano Tucci wrote:
> s/items/iteritems/

cache1 is mutated during the iteration so it has to be items(). I've changed the
other places.

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:110: def test_compare():
On 2014/05/20 15:22:57, Primiano Tucci wrote:
> Many thanks for the test, yay! :)
> Just, we typically put the tests in a side-file name after the actual source.
> In more concrete words, please move this to
> explain_binary_size_delta_unittest.py

Let me make a case for keeping it in the same file here. As this is a rather
small file, keeping the tests close to the source increases the likelihood they
are both maintained and executed as part of changes to this files functionality.
It is so easy that external tests otherwise just gather dust as nobody even
consider them when doing minor, trivial changes.

Putting tests in other packages also introduce problems with visibility and
scoping. How annoying is it not to have to expose something to the world just
because you want it in a test?

I know the counter arguments, but please think about it.

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/run_bi...
File tools/binary_size/run_binary_size_analysis.py (right):

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/run_bi...
tools/binary_size/run_binary_size_analysis.py:63: def _MkChild(node, name):
On 2014/05/20 15:22:57, Primiano Tucci wrote:
> _MkChild looks a "long"cut for
> return node.setdefault(name, {'n': name, 'children': {}})

Hmm, yes. Border case. Using setdefault forces you to build the dict even when
it's not needed.

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/run_bi...
tools/binary_size/run_binary_size_analysis.py:285: out.write('var tree_data = '
+ json_string)
On 2014/05/20 15:22:57, Primiano Tucci wrote:
> I've no idea how big this string is going to be.
> If it's large, is going to be much more efficient doing:
> with open(outfile, 'w') as out:
>   out.write('var tree_data = ')
>   json.dump(tree_root, out)

It can be big so this is a nice suggestion. Won't change anything that is
noticed but I will feel better knowing I don't juggle multi-MB strings.

[Andrew Hayden (chromium.org), 2014-05-21 08:59:57]

Another change I plan to make, but that can wait a bit, is to rename "children"
to just "c". There turns out to be function in d3js treemap layout that fetches
the children element of the current node, and the default (overridable) impl is
to return node.children. It could just as easily be node.c, which would shave a
few hundred k more off the output JSON, which is already stupidly huge.

Just mentioning this in passing, no need to do it now.

[Primiano Tucci (use gerrit), 2014-05-21 10:05:59]

Note that you have lint wargnings.
Yet another reason for having a PRESUBMIT.py (see my  comment later in the code
about PRESUBMIT), which will run pylint together with the unittests :)

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/binary...
File tools/binary_size/binary_size_utils.py (right):

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/binary...
tools/binary_size/binary_size_utils.py:14: Argument: an iterable over lines of
nm output.
On 2014/05/21 08:42:13, Daniel Bratell wrote:
> On 2014/05/20 15:22:57, Primiano Tucci wrote:
> > Nit: the format should look like: this
> > 
> > Parse nm output (Skipping BSS).
> > 
> > Args:
> >     input: an iterable over lines of nm output.
> > 
> > Yields:
> >     (symbol name, symbol type, symbol size, source file path)
> > 
> > 
> > *
> >
>
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Commen...
> 
> Done.

Ehm, I think the first line of the docstring should fit one line, e.g.,

"""Single line

Longer description

Args..."

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/binary...
tools/binary_size/binary_size_utils.py:27: addr_re = re.compile(r'^[0-9a-f]{8,}
(.) ([^\t]+)(?:\t.*)?$')
On 2014/05/21 08:42:13, Daniel Bratell wrote:
> On 2014/05/20 15:22:57, Primiano Tucci wrote:
> > Hmm what is the sense of the last non capturing group (?:\t.*)?
> > Can't you just .*?
> 
> I suspect it is to document the format and reject lines that don't fit what we
> expect. I've not looked much at this code except to make it 64 bit compatible.

I tend to not overengineer regex, especially with uneeded bits.
See the commit message of https://codereview.chromium.org/278763005 for a fun
recent story.

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

https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/explai...
tools/binary_size/explain_binary_size_delta.py:110: def test_compare():
On 2014/05/21 08:42:13, Daniel Bratell wrote:
> On 2014/05/20 15:22:57, Primiano Tucci wrote:
> > Many thanks for the test, yay! :)
> > Just, we typically put the tests in a side-file name after the actual
source.
> > In more concrete words, please move this to
> > explain_binary_size_delta_unittest.py
> 
> Let me make a case for keeping it in the same file here. As this is a rather
> small file, keeping the tests close to the source increases the likelihood
they
> are both maintained and executed as part of changes to this files
functionality.


Oh, actually, good point.
This should have a PRESUBMIT.py which runs the test every time you make a
change. An optional test is just a broken test. Realistically, ~ nobody will run
a test if is not mandatory, and nobody will care about fixing it.
Either remove the test or make it mandatory, otherwise this is just dead code.
My suggestion is to move to explain..._unittest.py and copy over
tools/deps2git/PRESUBMIT.py (which simple and effective).
This will enforce your test to be run every time somebody tries to make a change
to this file.

https://codereview.chromium.org/258633003/diff/100001/tools/binary_size/run_b...
File tools/binary_size/run_binary_size_analysis.py (right):

https://codereview.chromium.org/258633003/diff/100001/tools/binary_size/run_b...
tools/binary_size/run_binary_size_analysis.py:29: # we've figured out a proper
way to organize our library of python
Typo in "chanege."

P.S: In general 1st person comments ("our library", I hope) are misleading.
How is us? is it us "binary size"? is it "us chromium"?
which python tools are we going to reorganize?

I know the answer to these questions, but IMHO they don't convey any actionable
value to a random developer reading this code.

If you want to keep the comment just be concise and explain the point. You can
put intentions / project suggestions in a bug and link it. Spreading hopes in
the code doesn't help understanding it. :)

# elf_symbolizer lives in build/android (even though is not android specific).
crbug.com/XXXX

https://codereview.chromium.org/258633003/diff/100001/tools/binary_size/run_b...
tools/binary_size/run_binary_size_analysis.py:332: for _sym, _symbol_type, size,
path in symbols:
Hmm this naming is confusing.
_name should be for intenral global/class variables.
Why did you change that?

[Primiano Tucci (use gerrit), 2014-05-21 10:21:17]

Also a note on the commit message (sorry for the double mail)

Please note that the commit message is what will end up in the continuous
integration waterfall and in the git log.
Its purpose is to describe what the change is doing and which files is
affecting. It is the thing that:
- sheriffs will look at when something goes wrong
- developers will look at when searching for regressions
- future contributors will look at for understanding the reasons of a particular
change.

As such should describe the code / functionality.
If you have comments / plans like "should make it faster", "it would be nice
if", they should go into a bug (new.crbug.com) which should be linked using the
BUG= marker.
In this regard, note that you might want to have BUG=339059 to this commit
message (you might use that for suggesting improvements).

More comments inline below:

> The new binary_size is a tool written by andrewhayden for Android and ported
by me to generic Linux.

We have git blame for this, no need to keep this in the commit message.


> With the graphical output it's easier to locate segments that are unexpectedly
large. 
> Currently the tool is rather slow since the operation to map binary symbols to
source files is slow but the output is worth it. 
This doesn't look to describe the functional changes brought by this CL. If you
have concerns / plans for perf improvements put it into a crbug and link it.

>There are many further improvements possible to to do on this tool.
Contributions most welcome. 
Ditto here, especially the "Contributions most welcome. " part

[Daniel Bratell, 2014-05-21 13:52:58]

On 2014/05/21 10:05:59, Primiano Tucci wrote:
> Note that you have lint wargnings.

There are some lint warnings in the old code that Andrew wants to remove. Seems
like a waste of time to clean up code that will soon be removed. That is all
I've seen with the version of pylint in depot_tools but I guess other tools or
versions of tools or configurations of tools might have other warnings.

> I tend to not overengineer regex, especially with uneeded bits.

I tend to not touch regexps that seem to work. :-) (I've seen more bugs
introduced than fixed in my recent code reviews. :p) If I were to start changing
this code I would probably move away from regexps completely. The format is
dynamic and trivial enough to just parse from left to right.
 
>
https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/explai...
> File tools/binary_size/explain_binary_size_delta.py (right):
> 
>
https://codereview.chromium.org/258633003/diff/60001/tools/binary_size/explai...
> tools/binary_size/explain_binary_size_delta.py:110: def test_compare():
> On 2014/05/21 08:42:13, Daniel Bratell wrote:
> > On 2014/05/20 15:22:57, Primiano Tucci wrote:
> > > Many thanks for the test, yay! :)
> > > Just, we typically put the tests in a side-file name after the actual
> source.
> > > In more concrete words, please move this to
> > > explain_binary_size_delta_unittest.py
> > 
> > Let me make a case for keeping it in the same file here. As this is a rather
> > small file, keeping the tests close to the source increases the likelihood
> they
> > are both maintained and executed as part of changes to this files
> functionality.
> 
> 
> Oh, actually, good point.
> This should have a PRESUBMIT.py which runs the test every time you make a
> change. An optional test is just a broken test.

Moved to a different CL.

> Typo in "chanege."
> 
> P.S: In general 1st person comments ("our library", I hope) are misleading.
> How is us? is it us "binary size"? is it "us chromium"?
> which python tools are we going to reorganize?

Fixed.

> # elf_symbolizer lives in build/android (even though is not android specific).
> crbug.com/XXXX

Done.

>
https://codereview.chromium.org/258633003/diff/100001/tools/binary_size/run_b...
> tools/binary_size/run_binary_size_analysis.py:332: for _sym, _symbol_type,
size,
> path in symbols:
> Hmm this naming is confusing.
> _name should be for intenral global/class variables.
> Why did you change that?

"_" prefix is also common practice for unused variables. It silences pylint for
instance. (the prefix "dummy" does the same I think).

On 2014/05/21 10:21:17, Primiano Tucci wrote:
> Also a note on the commit message (sorry for the double mail)
> 
> Please note that the commit message is what will end up in the continuous
> integration waterfall and in the git log.

Oh, I had no idea. :-)

(Sorry, but if you had known me better you would have realized that it was
pretty funny. :-) )

> Its purpose is to describe what the change is doing and which files is
> affecting. It is the thing that:
> - sheriffs will look at when something goes wrong
> - developers will look at when searching for regressions
> - future contributors will look at for understanding the reasons of a
particular
> change.

You forgot one case: It will form the basis of changelogs for people that wonder
what happens in the source. Release managers, curious people and so on. The last
line, which seems to be what you are protesting against, are directed toward
those people.
> As such should describe the code / functionality.
> If you have comments / plans like "should make it faster", "it would be nice
> if", they should go into a bug (http://new.crbug.com) which should be linked
using the
> BUG= marker.

Eh, no? There are plenty of loose ideas for this code and Andrew has filed quite
a few bugs. Surely a commit that improves something shouldn't link to the bugs
listing what it didn't fix. Consider that CQ will also spam those bugs with
irrelevant information if done as that. I have included a descriptive text that
tells people how to find related bugs though.> > The new binary_size is a tool
written by andrewhayden for Android and ported
> by me to generic Linux.
> 
> We have git blame for this, no need to keep this in the commit message.

Except that if I commit it then all blame will land on me, despite almost all
code (like 80-90%) was written by andrewhayden and I only bug fixed it, ported
some code away from Java and now cleaned it up. That is why I think it is
reasonable to mention his name. He deserves the credit (and blame :-) ).

[Andrew Hayden (chromium.org), 2014-05-21 14:47:37]

> > by me to generic Linux.
> > 
> > We have git blame for this, no need to keep this in the commit message.
> 
> Except that if I commit it then all blame will land on me, despite almost all
> code (like 80-90%) was written by andrewhayden and I only bug fixed it, ported
> some code away from Java and now cleaned it up. That is why I think it is
> reasonable to mention his name. He deserves the credit (and blame :-) ).

Also something Primiano might not have realized: the explain_binary_size_delta
doesn't even exist. It's not a thing. It lived in a code review that I've just
deleted (since this one obviates the need for it). When this lands, it will be
the first time that the bytes hit the repo, so there's another good reason.

Yes, let the blaming begin! I'll deny all knowledge :)

[Andrew Hayden (chromium.org), 2014-05-21 14:48:43]

Oh!

One more thing!

You should delete the binary_size_tool entry from the gyp file:

https://code.google.com/p/chromium/codesearch#chromium/src/build/all.gyp&q=bi...

[Daniel Bratell, 2014-05-21 16:48:23]

On 2014/05/21 14:48:43, Andrew Hayden wrote:
> Oh!
> 
> One more thing!
> 
> You should delete the binary_size_tool entry from the gyp file:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/all.gyp&q=bi...

Ah yes.

Done, please take another look.

[Andrew Hayden (chromium.org), 2014-05-21 17:39:56]

Ah, good, you got the known_bugs as well. I had forgotten there was still one
entry lurking there. Great! Thanks. Still LGTM, now moreso.

[Primiano Tucci (use gerrit), 2014-05-21 18:08:39]

> Moved to a different CL.
Thanks.
> > Hmm this naming is confusing.
> > _name should be for intenral global/class variables.
> > Why did you change that?
> 
> "_" prefix is also common practice for unused variables. It silences pylint
for
> instance. (the prefix "dummy" does the same I think).
Ah sorry I didn't realize.
Ah, in this case I think our code style suggests just _ 
e.g. for (_, _, foo, bar) in x


> > Please note that the commit message is what will end up in the continuous
> > integration waterfall and in the git log.
> 
> Oh, I had no idea. :-)
> 
> (Sorry, but if you had known me better you would have realized that it was
> pretty funny. :-) )

In general, when it comes to technical discussions (like a codereview), my
goal is just making actionable comments and explain my reasons according to what
I
see on the CL, without doing background check on people (actually I'd love if
codereviews were double-blind).

I do apologize if that made you feel offended in any way, but this is not about
people or social ladders, a code review is just about code (and commit logs) ;-)

From the more concrete viewpoint, please note that the your CL title is
still wrong ("binary_size TNG") and it should match the first line of the 
description.

> You forgot one case: It will form the basis of changelogs for people that
wonder
> what happens in the source. Release managers, curious people and so on.

Release managers... in the changelog of src/tools?

> The last
> line, which seems to be what you are protesting against, are directed toward
> those people.

Just to be clear, I am not protesting at all (which sentence that I wrote made
you feel I'm protesting?).
If I was protesting or uncomfortable with the change I'd just have wrote "NOT
L..."
I am just spending time striving to provide actionable and useful feedback.
The former CL description looked in my view just unusual and redundant for the
chromium style, and I just pointed that out in a very extremely polite way.


> > As such should describe the code / functionality.
> > If you have comments / plans like "should make it faster", "it would be nice
> > if", they should go into a bug (http://new.crbug.com) which should be linked
> using the
> > BUG= marker.
> 
> Eh, no? There are plenty of loose ideas for this code and Andrew has filed
quite
> a few bugs. Surely a commit that improves something shouldn't link to the bugs
> listing what it didn't fix.

I don't think I ever suggested to link random unrelated bugs.
I just suggested "339059: Binary size tool shouldn't have any Java portions"
which seems a perfect match for this CL.


> > The new binary_size is a tool
> written by andrewhayden for Android and ported
> > by me to generic Linux.
> > 
> > We have git blame for this, no need to keep this in the commit message.
> 
> Except that if I commit it then all blame will land on me, despite almost all
> code (like 80-90%) was written by andrewhayden and I only bug fixed it, ported
> some code away from Java and now cleaned it up. That is why I think it is
> reasonable to mention his name. He deserves the credit (and blame :-) ).

git blame is not a tool for recognizing contributions.
We have other ways to evaluate people performances and that tool is definitely
not running "wc" on the git history. I am sure that he'll get all the credit for
the awesome work he's making regardless of the lines of code under his name.

git blame is a tool for developers. If somebody in future will find a bug or
want
to improve something, he'll use git blame to figure out who wrote the code and
who is the person likely to be most familiar with that for a review.
If you wrote the code and are familiar, you are eligible to be that person.
If you'll not be available for whatever reason, they will pick the reviewers
of this CL. easy :)

Either ways is fine, this is definitely not the kind of detail worth spending
more than two emails, and the CL description looks definitely in better shape
now.


Just to be clear,
I don't want to prevent / block anybody. I think this is work is useful.
The python code LGTM.

I'm just used to a slightly different culture which is more simplistic and based
on pure technical and quantitative comments, where people don't get offended
for lese majeste if they receive a comment saying that the commit message
contains redundant information.

I personally never got offended if an intern replies to a CR explaining
something
which is already clear to me. But this discussion is non technical, and is far
beyond the scope of a CR.

[Daniel Bratell, 2014-05-21 22:10:16]

On 2014/05/21 18:08:39, Primiano Tucci wrote:
> In general, when it comes to technical discussions (like a codereview), my
> goal is just making actionable comments and explain my reasons according to
what
> I
> see on the CL, without doing background check on people (actually I'd love if
> codereviews were double-blind).
> 
> I do apologize if that made you feel offended in any way, but this is not
about
> people or social ladders, a code review is just about code (and commit logs)
;-)

Not at all. It amused me. It didn't offend me. I've spent a lot of time over
many years teaching people to write good commit messages. I have written and
maintain changelog generators and parsers Opera use to keep track of what flows
in from the chromium project. I have written tools that analyze commit messages
and reject integrations that don't fulfill certain minimum criteria to avoid the
worst mistakes. I've written summary reports of changes for blog posts based on
nothing but commit messages. We also have the commit messages as a reviewable
part in our code review system (critic). So it was amusing to end up at the
other side of it. :-) No ill feelings at all!

I'm grateful that you have taken the time to read through this code, though I'm
also a little worried that I make you spend time on things that are less
important than other things you do.

> From the more concrete viewpoint, please note that the your CL title is
> still wrong ("binary_size TNG") and it should match the first line of the 
> description.

I wasn't even aware it was used anywhere. I used to think it was the first line
of the commit messages but I learned the hard way that was not true.

> Release managers... in the changelog of src/tools?

You never know who reads your commit messages. It's a rare case when there is
too much information in a commit message IMO. If you look through the commit
messages of chromium and blink (and other projects both inside Google and
outside) there are many that are not even close to what I would like them to be.
In Chromium I like the v8 ones the best. They are something like "Improvements
to stability and performance to all platforms". And they are always the same.

On the other hand, when I used to dig through the history of Presto to find the
cause of some strange code line and ended up back in the 1990s I could find
commit messages for multi-thousand line changes that said just "ok" and nothing
more so I've seen worse as well. :-)

[Daniel Bratell, 2014-05-22 14:26:31]

The CQ bit was checked by bratell@opera.com

[commit-bot: I haz the power, 2014-05-22 14:27:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bratell@opera.com/258633003/180001

[commit-bot: I haz the power, 2014-05-22 18:49:42]

Change committed as 272255