Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9169074:
Add '-d' flag to dump-static-initializers.py (Closed)
|
Created:
8 years, 11 months ago by Tyler Breisacher (Chromium) Modified:
8 years, 10 months ago CC:
chromium-reviews, pam+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptiondump-static-initializers.py: Add '-d' flag for listing SI's in an easily "diff-able" way, for use in the 'sizes' script.
BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=120131
Patch Set 1 #Patch Set 2 : cleaning up a bit #
Total comments: 1
Patch Set 3 : "" #
Total comments: 11
Patch Set 4 : addressing review comments #
Total comments: 7
Patch Set 5 : s/yields/returns #Patch Set 6 : remove count flags #
Total comments: 4
Patch Set 7 : fixes #Messages
Total messages: 18 (0 generated)
I read http://codereview.chromium.org/8207001 and I didn't really understand the discussion there; do you have a link to the chromium-dev thread? I'm also not sure exactly what sizes.py should look like because I think we want both the number of initializers (for the sizes dashboard) and the full list of initializers (so it can be diff'ed with the previous build). https://chromiumcodereview.appspot.com/9169074/diff/1002/tools/linux/dump-sta... File tools/linux/dump-static-initializers.py (left): https://chromiumcodereview.appspot.com/9169074/diff/1002/tools/linux/dump-sta... tools/linux/dump-static-initializers.py:135: continue I'm pretty sure this doesn't actually do anything. https://chromiumcodereview.appspot.com/9169074/diff/1003/tools/linux/dump-sta... File tools/linux/dump-static-initializers.py (left): https://chromiumcodereview.appspot.com/9169074/diff/1003/tools/linux/dump-sta... tools/linux/dump-static-initializers.py:155: if size == 2: I discussed this with fischman@ and he said that we *do* want to count these little two-byte ctors, because they still get called at runtime, even though they don't do anything. If I'm understanding http://neugierig.org/software/chromium/notes/2011/08/static-initializers.html correctly, what's important is reducing the *number* of SIs, not just getting rid of the biggest ones. https://chromiumcodereview.appspot.com/9169074/diff/1003/tools/linux/dump-sta... File tools/linux/dump-static-initializers.py (right): https://chromiumcodereview.appspot.com/9169074/diff/1003/tools/linux/dump-sta... tools/linux/dump-static-initializers.py:99: """Given a binary, yield static initializers as (file, start, size) tuples.""" I switched the order so that it would always be sorted by filename. The start is likely to change from one build to the next, but the filename is not.
> I read http://codereview.chromium.org/8207001 and I didn't really understand the discussion there; do you have a link to the chromium-dev thread? There was no chromium-dev thread. There was a separate thread with the compiler folks, but I summarized that already. http://codereview.chromium.org/9169074/diff/1003/tools/linux/dump-static-init... File tools/linux/dump-static-initializers.py (left): http://codereview.chromium.org/9169074/diff/1003/tools/linux/dump-static-init... tools/linux/dump-static-initializers.py:155: if size == 2: On 2012/01/26 22:36:33, tbreisacher wrote: > I discussed this with fischman@ and he said that we *do* want to count these > little two-byte ctors, because they still get called at runtime, even though > they don't do anything. If I'm understanding > http://neugierig.org/software/chromium/notes/2011/08/static-initializers.html > correctly, what's important is reducing the *number* of SIs, not just getting > rid of the biggest ones. From the comment, it sounds like many of these are accidentally generated in the current version of gcc and there's nothing we can do to fix them. Counting them won't do us any good. http://codereview.chromium.org/9169074/diff/1003/tools/linux/dump-static-init... File tools/linux/dump-static-initializers.py (right): http://codereview.chromium.org/9169074/diff/1003/tools/linux/dump-static-init... tools/linux/dump-static-initializers.py:102: files = [] This defeats the purpose of having the yield(). http://codereview.chromium.org/9169074/diff/1003/tools/linux/dump-static-init... tools/linux/dump-static-initializers.py:175: file_count += 1 Isn't this going to count a given file multiple times?
Can you make the review description describe the change? It is hard to review otherwise.
http://codereview.chromium.org/9169074/diff/1003/tools/linux/dump-static-init... File tools/linux/dump-static-initializers.py (left): http://codereview.chromium.org/9169074/diff/1003/tools/linux/dump-static-init... tools/linux/dump-static-initializers.py:155: if size == 2: But we can make sure they're not called at load time. On the other hand, if we're going to switch to GCC 4.6+ at some point, then maybe it doesn't matter. http://codereview.chromium.org/9169074/diff/1003/tools/linux/dump-static-init... File tools/linux/dump-static-initializers.py (right): http://codereview.chromium.org/9169074/diff/1003/tools/linux/dump-static-init... tools/linux/dump-static-initializers.py:175: file_count += 1 I don't think so, because of the |continue| after it.
http://codereview.chromium.org/9169074/diff/1003/tools/linux/dump-static-init... File tools/linux/dump-static-initializers.py (left): http://codereview.chromium.org/9169074/diff/1003/tools/linux/dump-static-init... tools/linux/dump-static-initializers.py:155: if size == 2: On 2012/01/27 19:44:19, tbreisacher wrote: > But we can make sure they're not called at load time. On the other hand, if > we're going to switch to GCC 4.6+ at some point, then maybe it doesn't matter. Yes, that is my belief. Removing them will make the code needlessly cluttered and it's really a compiler bug. http://codereview.chromium.org/9169074/diff/1003/tools/linux/dump-static-init... File tools/linux/dump-static-initializers.py (right): http://codereview.chromium.org/9169074/diff/1003/tools/linux/dump-static-init... tools/linux/dump-static-initializers.py:109: for f in sorted(files): I'd weakly prefer for this function to remain unsorted and have the caller sort if it needs it. That would allow you to keep the yield as well.
http://codereview.chromium.org/9169074/diff/1003/tools/linux/dump-static-init... File tools/linux/dump-static-initializers.py (right): http://codereview.chromium.org/9169074/diff/1003/tools/linux/dump-static-init... tools/linux/dump-static-initializers.py:102: files = [] On 2012/01/27 19:18:38, Lei Zhang wrote: > This defeats the purpose of having the yield(). Done. Also at line 135 in patchset 4. http://codereview.chromium.org/9169074/diff/1003/tools/linux/dump-static-init... tools/linux/dump-static-initializers.py:109: for f in sorted(files): On 2012/01/27 19:52:09, Evan Martin wrote: > I'd weakly prefer for this function to remain unsorted and have the caller sort > if it needs it. That would allow you to keep the yield as well. Done. http://codereview.chromium.org/9169074/diff/9002/tools/linux/dump-static-init... File tools/linux/dump-static-initializers.py (right): http://codereview.chromium.org/9169074/diff/9002/tools/linux/dump-static-init... tools/linux/dump-static-initializers.py:170: files = sorted(files) The only thing that's weird about this, is we're sorting by the original filename, not the qualified filename. So the output doesn't actually look sorted. I don't think this is really a problem, the goal was just to make sure the order won't change from one build to the next, not necessarily to list every SI alphabetically.
http://codereview.chromium.org/9169074/diff/9002/tools/linux/dump-static-init... File tools/linux/dump-static-initializers.py (left): http://codereview.chromium.org/9169074/diff/9002/tools/linux/dump-static-init... tools/linux/dump-static-initializers.py:138: yield ref I'm confused, is this code in the current checked-in version?
http://codereview.chromium.org/9169074/diff/9002/tools/linux/dump-static-init... File tools/linux/dump-static-initializers.py (left): http://codereview.chromium.org/9169074/diff/9002/tools/linux/dump-static-init... tools/linux/dump-static-initializers.py:138: yield ref Yes. There are typically only a few refs per file, I think, so it doesn't matter much either way, but (as above) using |yield| implies that we're yielding values continuously, instead of waiting to get all of them and returning the whole list all at once. If we're sorting, then we can't do that so there's no point in using yield. Unless there's something about this case that's different than the one above?
http://codereview.chromium.org/9169074/diff/9002/tools/linux/dump-static-init... File tools/linux/dump-static-initializers.py (right): http://codereview.chromium.org/9169074/diff/9002/tools/linux/dump-static-init... tools/linux/dump-static-initializers.py:135: return sorted(refs) Can you fix the function-level comment to not say "yields"? I think that is what got me confused. http://codereview.chromium.org/9169074/diff/9002/tools/linux/dump-static-init... tools/linux/dump-static-initializers.py:145: help='Print out the number of static initializers') Why not just always print these things?
http://codereview.chromium.org/9169074/diff/9002/tools/linux/dump-static-init... File tools/linux/dump-static-initializers.py (right): http://codereview.chromium.org/9169074/diff/9002/tools/linux/dump-static-init... tools/linux/dump-static-initializers.py:135: return sorted(refs) On 2012/01/30 23:42:25, Evan Martin wrote: > Can you fix the function-level comment to not say "yields"? > I think that is what got me confused. Done. http://codereview.chromium.org/9169074/diff/9002/tools/linux/dump-static-init... tools/linux/dump-static-initializers.py:145: help='Print out the number of static initializers') On 2012/01/30 23:42:25, Evan Martin wrote: > Why not just always print these things? I'm not opposed to always printing them, but I think it is useful to have the option to ONLY print them. When I was fixing some SIs last week, I fell into this pattern to make sure they were actually fixed: * checkout master * build * run this with -f (output is, say, 115) * checkout the branch with my fix on it * build * run this with -f (output is, say, 114) -f is faster than the other options because it doesn't call |objdump|. So maybe we should keep -f and -i as they are, and also print the numbers at the end of the output for the other options. On the other hand, I also could have just piped the output of this script into |wc -l| or something, and maybe it's simpler to just advise users to do that. Whatever you think is better.
On 2012/01/30 23:53:45, tbreisacher wrote: > -f is faster than the other options because it doesn't call |objdump|. So maybe > we should keep -f and -i as they are, and also print the numbers at the end of > the output for the other options. My priorities are: 1) speed, if it is enough to be noticeable 2) less code, as more code is more bugs and more maintenance If you think the speed difference of these flags matters, I'll defer to you. But otherwise I think 99% of people who run this script don't understand the issues to know what flags they ought to be using and will just use the defaults.
Here's what I got on my machine:
no flags, or -d: 31s
-f: 0.6s
-i: 17s
30 seconds is really not that much time, and of course it should get quicker the
more SIs we fix. So I vote for simplifying the code.
What do we need to do to get sizes.py to use this script? Is it just a matter of
changing sizes.py to call this script, and then making sure this script outputs
the right format ("RESULT chrome-si: initializers= %d files")?
LGTM with one minor fix http://codereview.chromium.org/9169074/diff/14001/tools/linux/dump-static-ini... File tools/linux/dump-static-initializers.py (right): http://codereview.chromium.org/9169074/diff/14001/tools/linux/dump-static-ini... tools/linux/dump-static-initializers.py:154: if (opts.diffable): No parens around this
oh, and please update the review description to describe the new behavior
LGTM http://codereview.chromium.org/9169074/diff/14001/tools/linux/dump-static-ini... File tools/linux/dump-static-initializers.py (right): http://codereview.chromium.org/9169074/diff/14001/tools/linux/dump-static-ini... tools/linux/dump-static-initializers.py:96: # 0000000001919920 0000000000000008 b _ZN12_GLOBAL__N_119g_nine_box_prelightE can you fix this incorrect comment while you're at it? It should be: 0000000001919920 0000000000000008 t _ZN12_GLOBAL__I_safe_browsing_service.cc
http://codereview.chromium.org/9169074/diff/14001/tools/linux/dump-static-ini... File tools/linux/dump-static-initializers.py (right): http://codereview.chromium.org/9169074/diff/14001/tools/linux/dump-static-ini... tools/linux/dump-static-initializers.py:96: # 0000000001919920 0000000000000008 b _ZN12_GLOBAL__N_119g_nine_box_prelightE On 2012/01/31 20:57:46, Lei Zhang wrote: > can you fix this incorrect comment while you're at it? It should be: > 0000000001919920 0000000000000008 t _ZN12_GLOBAL__I_safe_browsing_service.cc Done. http://codereview.chromium.org/9169074/diff/14001/tools/linux/dump-static-ini... tools/linux/dump-static-initializers.py:154: if (opts.diffable): On 2012/01/31 19:49:19, Evan Martin wrote: > No parens around this Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tbreisacher@chromium.org/9169074/18001
Change committed as 120131 |
