Update sizes.py to print a list of all static initializers

Update sizes.py to print a list of all static initializers

BUG=102013
TEST='sizes' step prints list of initializers as well as the number of initializers

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

[Lei Zhang, 2012-02-03 22:54:57]

https://chromiumcodereview.appspot.com/9323047/diff/2001/build/scripts/slave/...
File build/scripts/slave/chromium/sizes.py (right):

https://chromiumcodereview.appspot.com/9323047/diff/2001/build/scripts/slave/...
build/scripts/slave/chromium/sizes.py:170: if 'Release' in target_dir:
Otherwise, maybe os.path.basename(target_dir) is more robust?

Alternatively, if we had access to |options|, we can just check
|options.target|, and use options.build_dir below.

https://chromiumcodereview.appspot.com/9323047/diff/2001/build/scripts/slave/...
build/scripts/slave/chromium/sizes.py:171: dump_static_initializers =
os.path.join(os.path.dirname(target_dir),
If |target_dir| is /path/to/src/out/Release, don't we need
'../../tools/linux/d-s-i.py' ?

[Tyler Breisacher (Chromium), 2012-02-04 03:03:19]

https://chromiumcodereview.appspot.com/9323047/diff/2001/build/scripts/slave/...
File build/scripts/slave/chromium/sizes.py (right):

https://chromiumcodereview.appspot.com/9323047/diff/2001/build/scripts/slave/...
build/scripts/slave/chromium/sizes.py:170: if 'Release' in target_dir:
You're right, that's probably cleaner.

https://chromiumcodereview.appspot.com/9323047/diff/2001/build/scripts/slave/...
build/scripts/slave/chromium/sizes.py:171: dump_static_initializers =
os.path.join(os.path.dirname(target_dir),
I think you're right.

[Lei Zhang, 2012-02-04 03:10:00]

https://chromiumcodereview.appspot.com/9323047/diff/2004/build/scripts/slave/...
File build/scripts/slave/chromium/sizes.py (right):

https://chromiumcodereview.appspot.com/9323047/diff/2004/build/scripts/slave/...
build/scripts/slave/chromium/sizes.py:171: dump_static_initializers =
os.path.join(os.path.dirname(target_dir),
So how about os.path.join(options.build_dir, 'tools', 'linux', d-s-i.py') ?

[Tyler Breisacher (Chromium), 2012-02-04 03:18:11]

Good idea. Thanks.

[Lei Zhang, 2012-02-04 03:20:33]

lgtm

[Tyler Breisacher (Chromium), 2012-02-04 03:50:33]

Adding thakis@ for OWNER approval. Also, would you happen to know if there are
any trybots that run sizes.py so we can test this?

[Nico, 2012-02-04 17:33:39]

I like the idea of printing the list of static initializers, that should make
life much easier :-)

I'm not sure if printing arbitrary output will confuse the perf waterfall
scripts -- cmp will know.

I'd like to keep the previous method of coming up with just the count, since
that's right from the binary, without any potentially buggy postprocessing.

All trybots run the sizes step as far as I know (but it never goes red on any
bot, because the trybots build without nacl and svg, so the results aren't
comparable to our baselines).

[cmp, 2012-02-06 19:02:09]

This shouldn't confuse the bots, as long as dump-static-intializers doesn't
output any lines matching the format listed in
tools/build/scripts/master/log_parser/process_log.py.  But note that if
dump-static-initializers is ever changed in the future, it could confuse the
bots then (since this script doesn't read that output and print it in a safe
format, it only reprints what dump-static-initializers outputs).

[Tyler Breisacher (Chromium), 2012-02-06 19:38:49]

On 2012/02/04 17:33:39, Nico wrote:
> I'd like to keep the previous method of coming up with just the count

One advantage of using dump-static-initializers.py is that it lists the number
of static initializers, rather than the number of files containing static
initializers. Currently, if someone adds another initializer to a file that
already has one, we won't catch it. Is there a way to keep the previous method,
but modify it so that it counts initializers rather than files?

On 2012/02/06 19:02:09, cmp wrote:
> But note that if
> dump-static-initializers is ever changed in the future, it could confuse the
bots then

Then let's add a note in dump-static-initializers.py, something like this:
https://chromiumcodereview.appspot.com/9325078

[Nico, 2012-02-07 04:26:03]

On 2012/02/06 19:38:49, Tyler Breisacher wrote:
> On 2012/02/04 17:33:39, Nico wrote:
> > I'd like to keep the previous method of coming up with just the count
> 
> One advantage of using dump-static-initializers.py is that it lists the number
> of static initializers, rather than the number of files containing static
> initializers. Currently, if someone adds another initializer to a file that
> already has one, we won't catch it. Is there a way to keep the previous
method,
> but modify it so that it counts initializers rather than files?

I don't think this is terribly important – it's usually fairly easy to remove
all static initializers from a file in one go (except if the file also contains
protobufs, but pliard@ is actively working on that problem, so it should go away
soon). Also, I could imagine that all code in a translation unit ends up in the
same general area of the executable, so the static initializers in a file
probably cache in the same set of pages, no matter how many there are in a file
(up to a degree).

(Also, this keeps the number comparable to the mac number.)

So if you don't hate that too much, I'd go with keeping the old method for the
number + printing a list of initializers.

> On 2012/02/06 19:02:09, cmp wrote:
> > But note that if
> > dump-static-initializers is ever changed in the future, it could confuse the
> bots then
> 
> Then let's add a note in dump-static-initializers.py, something like this:
> https://chromiumcodereview.appspot.com/9325078

Maybe print # characters in column 0? Then the other script could eventually
learn to strip comments, and it's clearer to the reader too.

[Tyler Breisacher (Chromium), 2012-02-07 05:47:51]

All good points. Reverting back to the old code for getting the number

> Maybe print # characters in column 0?

https://chromiumcodereview.appspot.com/9347013/

[Nico, 2012-02-07 06:21:41]

lgtm

[commit-bot: I haz the power, 2012-02-07 19:36:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbreisacher@chromium.org/9323047/11001

[commit-bot: I haz the power, 2012-02-07 19:36:09]

Can't process patch for file build/scripts/slave/chromium/sizes.py.
File's status is None, patchset upload is incomplete.