Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
119083006:
Add tool to help analyze binary size (Closed)
|
Created:
6 years, 11 months ago by Andrew Hayden (chromium.org) Modified:
6 years, 10 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd tool to help analyze binary size
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245405
Patch Set 1 #Patch Set 2 : Add gyp file, fix missing "static" keyword #Patch Set 3 : Break apart monolithic class into smaller units #Patch Set 4 : Add necessary gypii/py files for running tool #Patch Set 5 : remove unused run_cwd arg #Patch Set 6 : use a python script instead of gyp #Patch Set 7 : make sure to copy license file from webtreemap #Patch Set 8 : Add android-x86 support and fix regexes for modern addr2line #Patch Set 9 : Fix --arch bug, add target to all_android.gyp #Patch Set 10 : remove extraneous comment from buildfile #
Total comments: 45
Patch Set 11 : Marcus's comments #Patch Set 12 : Added top-100 largest symbols report #Patch Set 13 : Allow choosing what to see in the report index.html #Patch Set 14 : Add largest-sources report #Patch Set 15 : Don't hyperlink sources from the out directories #Patch Set 16 : Remove unnecessary threadsafety from Record.java #
Total comments: 93
Patch Set 17 : comments from beverloo@ and bulach@ #Patch Set 18 : Formatting ONLY. #Patch Set 19 : Fix minor bug with zip download and dir creation #Patch Set 20 : Fix weird gyp/gn/ninja problem #Patch Set 21 : Improve jsonification and spatial map, add vtables report #Patch Set 22 : shrink text inputs #Patch Set 23 : avoid indenting tree dump, saving 5 megs in contentshell report #Patch Set 24 : Add chromium notice to index.html #
Total comments: 28
Patch Set 25 : Remove problematic OSS, add README #Patch Set 26 : Output usage message in Java, fix oversight in python script #Patch Set 27 : Address comments from Marcus #Patch Set 28 : Switch to webtreemap in third_party #
Total comments: 1
Patch Set 29 : Trivial text update to README.txt #
Total comments: 7
Patch Set 30 : <ol> -> <ul> for README.txt shortcomings list #
Total comments: 1
Patch Set 31 : ORDERED LISTS SHOULD BE ILLEGAL #Patch Set 32 : Marcus' comments #
Total comments: 3
Patch Set 33 : Rebase onto master for a clean commit of gyp change #
Total comments: 1
Messages
Total messages: 31 (0 generated)
So, this is now at a point where I'd consider it sufficient for an initial checkin. Ideally we'd just have python, and no Java - but that can be the next version, in my opinion. Yeah, we don't have a lot of Java tools... but there's nothing magic here, and it is pretty straightforward. WDYT? It's been suggested that I look into using d3 instead of webtreemap, but that can also be an upgrade in a later commit.
nice!! I have a few general comments below, and yeah, I suppose in python this would've been simpler... :( anyways, let's get this going so we can actually analyze and reduce the binary size sooner rather than later. https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... File tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java (right): https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:38: private final ConcurrentHashMap<Integer, Record> addressesSeen = nit: similar to 31, looks like this can be declared as just a "HashMap"? also, to make it more generic, perhaps "Long" ? https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:69: .getParentFile().getParentFile().getParentFile() nit: subtle family tree.. :) can we split the components and then rejoin up to src/ ? https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:100: * @param percent absolute percentage to jitter by (in the range [0,1]) nit: for the purpose here, could do all in integer space, i.e., [0,100], then (r.nextInt() * percent) / 100 and avoid the cast.. https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:126: * approximately 2000 appears, empirically, to be a good tradeoff nit: perhaps replace "approximately 2000" with: ... 2000+/-10% (so the process aren't recycled all at once) ... https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:130: private final int processRecycleThreshold = jitter(2000,.10f); nit: space after , also, I think 10 would be simpler https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:134: workerThread = new Thread(new Addr2LineTask(), "read thread"); nit: Addr2Line reader thread https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:344: } nit: perhaps skip everything starting with . as well, like .git, or .svn? https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:358: if (dupes.size() > 0) { nit: could remove this check https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:365: // TODO: Could integrate with build system to know EXACTLY what is out there nit: TODO(andrewhayden) https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:370: if (file.isDirectory() && file.canRead()) { ditto, exclude anything starting with a . https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:388: } nit: for consistency, add a \n here and 391 below https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... File tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java (right): https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java:25: // is a vanilla installation of the Java Runtime Environment, 1.5 or later. nit: move this comment inside the following block? https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java:63: private static final String NM_REGEX = nit: could have this as private "static final Pattern NM_REGEX" and store it compiled? https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java:157: pool.await(5, TimeUnit.MINUTES); nit: send this as a command line param https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java:160: + queued + " queued for processing (" nit: the first "+" operator goes in the previous line https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java:225: // Line looks like this: nit: I guess it'd be more readable to move 225-250 into a "Record getICUDT46Record()", so here it'd be: Record record = getICUDT46Record(); if (record) { numSpooled++; pool.submit(record); continue; } https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java:308: completionLatch.await(); // wait for output to be done not quite sure what this sink is bringing? why not just execute it inline here? https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java:330: + successCount.get() + " ok, " + unsuccessful + " failed)" nit: move first + to the previous line https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java:409: Record record = pool.poll(); does this poll take a timeout? can it replace the sleep block in 418-426? https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java:505: // Source: https://github.com/andrewhayden/uopt4j I understand it's yours and license compatible, but I suppose this needs to be cleared by third-party... and this would be another big plus for python too :) https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... File tools/binary_size/java/src/org/chromium/tools/binary_size/Record.java (right): https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Record.java:9: * addr2line was successful. All fields are volatile because this structure hmm... is it ever used concurrently? just the fact that it's passed across threads doesn't justify volatile, no? https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/run_b... File tools/binary_size/run_binary_size_analysis.py (right): https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:35: def parse_nm(input): not sure, is this needed?
Addressing all comments... https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... File tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java (right): https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:38: private final ConcurrentHashMap<Integer, Record> addressesSeen = On 2014/01/07 19:38:30, bulach wrote: > nit: similar to 31, looks like this can be declared as just a "HashMap"? > > also, to make it more generic, perhaps "Long" ? It needs to be concurrent because all the worker threads are able to modify it in parallel, so I'm declaring as ConcurrentMap. I also fixed a race condition that could have resulted in multiple workers inserting into the cache in parallel. https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:69: .getParentFile().getParentFile().getParentFile() On 2014/01/07 19:38:30, bulach wrote: > nit: subtle family tree.. :) can we split the components and then rejoin up to > src/ ? How about we take everything up to and including the final instance of "/src/"? I'll try that. https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:100: * @param percent absolute percentage to jitter by (in the range [0,1]) On 2014/01/07 19:38:30, bulach wrote: > nit: for the purpose here, could do all in integer space, i.e., [0,100], then > (r.nextInt() * percent) / 100 and avoid the cast.. I changed the code to accept a value between 0 and 100 instead of 0 and 1, but there's still a cast necessary. https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:126: * approximately 2000 appears, empirically, to be a good tradeoff On 2014/01/07 19:38:30, bulach wrote: > nit: perhaps replace "approximately 2000" with: > ... 2000+/-10% (so the process aren't recycled all at once) ... Done. https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:130: private final int processRecycleThreshold = jitter(2000,.10f); On 2014/01/07 19:38:30, bulach wrote: > nit: space after , > also, I think 10 would be simpler Done. https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:134: workerThread = new Thread(new Addr2LineTask(), "read thread"); On 2014/01/07 19:38:30, bulach wrote: > nit: Addr2Line reader thread Done. https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:344: } On 2014/01/07 19:38:30, bulach wrote: > nit: perhaps skip everything starting with . as well, like .git, or .svn? Done. https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:358: if (dupes.size() > 0) { On 2014/01/07 19:38:30, bulach wrote: > nit: could remove this check Ha. Yeah I thought dupe removal was going to be fairly evil, then realized it was just remove. Done :) https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:365: // TODO: Could integrate with build system to know EXACTLY what is out there On 2014/01/07 19:38:30, bulach wrote: > nit: TODO(andrewhayden) Done. https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:370: if (file.isDirectory() && file.canRead()) { On 2014/01/07 19:38:30, bulach wrote: > ditto, exclude anything starting with a . Done. https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:388: } On 2014/01/07 19:38:30, bulach wrote: > nit: for consistency, add a \n here and 391 below Done. https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... File tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java (right): https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java:25: // is a vanilla installation of the Java Runtime Environment, 1.5 or later. On 2014/01/07 19:38:30, bulach wrote: > nit: move this comment inside the following block? Done. https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java:63: private static final String NM_REGEX = On 2014/01/07 19:38:30, bulach wrote: > nit: could have this as private "static final Pattern NM_REGEX" and store it > compiled? Done. https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java:157: pool.await(5, TimeUnit.MINUTES); On 2014/01/07 19:38:30, bulach wrote: > nit: send this as a command line param This is just a safety valve to make sure we eventually terminate; since the input queue is a fixed size (1000 entries), there should be very little left to process on the queue by the time we enter the call to wait. If it takes more than 5 minutes to process 1000 symbols, you are in trouble :) I'll add a comment here explaining this and add an exception if it times out. https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java:160: + queued + " queued for processing (" On 2014/01/07 19:38:30, bulach wrote: > nit: the first "+" operator goes in the previous line Done here and elsewhere. https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java:225: // Line looks like this: On 2014/01/07 19:38:30, bulach wrote: > nit: I guess it'd be more readable to move 225-250 into a "Record > getICUDT46Record()", so here it'd be: > > Record record = getICUDT46Record(); > if (record) { > numSpooled++; > pool.submit(record); > continue; > } Done. https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java:308: completionLatch.await(); // wait for output to be done On 2014/01/07 19:38:30, bulach wrote: > not quite sure what this sink is bringing? why not just execute it inline here? If the process is trying to write to stderr and it is not drained, the process may block indefinitely. The stream must be managed as soon as the process is launched to avoid this possibility. https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java:330: + successCount.get() + " ok, " + unsuccessful + " failed)" On 2014/01/07 19:38:30, bulach wrote: > nit: move first + to the previous line Done. https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java:409: Record record = pool.poll(); On 2014/01/07 19:38:30, bulach wrote: > does this poll take a timeout? can it replace the sleep block in 418-426? Sadly ConcurrentLinkedQueue does not have a poll() method that takes parameters, thus there is no easy way to do this. https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java:505: // Source: https://github.com/andrewhayden/uopt4j On 2014/01/07 19:38:30, bulach wrote: > I understand it's yours and license compatible, but I suppose this needs to be > cleared by third-party... > > and this would be another big plus for python too :) Yeah. I'll bug open source approvers. One of the long-standing pain points of Java. https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... File tools/binary_size/java/src/org/chromium/tools/binary_size/Record.java (right): https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Record.java:9: * addr2line was successful. All fields are volatile because this structure On 2014/01/07 19:38:30, bulach wrote: > hmm... is it ever used concurrently? just the fact that it's passed across > threads doesn't justify volatile, no? It is written in two separate threads and read in a third, so yes, it needs to be volatile. Some fields could probably be final, but the long pole here is addr2line so I'm not too worried about the performance implications of reading and writing these. https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/run_b... File tools/binary_size/run_binary_size_analysis.py (right): https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:35: def parse_nm(input): On 2014/01/07 19:38:30, bulach wrote: > not sure, is this needed? Yes, the bloat script parses nm to convert the strings to records for the treemap. In the end (not now), we should probably have a little pylib for parsing nm that we can share both with this code and the eventual pythonified version of ParallelAddr2Line.
I'm also very skeptical about the use of volatile here; my rule of thumb for both C++ and Java is "if you use volatile your code is wrong". However, I can't easily follow this code to tell you exactly whether this is the case or not. :) Generally uses of volatile in Java are, in decreasing order of probability: 1) completely unnecessary 2) a sign that the code isn't actually threadsafe 3) a sign that you are doing some complex lock-free trickery that relies on specifics of the JVM memory model for performance reasons Since you appear to be using a threadsafe queue to move the records around, you probably aren't doing 3), and so my assumption is that it's either 1) or 2) :) Having said that, this is just an analysis tool and we should probably get it submitted so we can start analysing things sooner rather than later :)
On 2014/01/08 11:51:39, Torne wrote: > I'm also very skeptical about the use of volatile here; my rule of thumb for > both C++ and Java is "if you use volatile your code is wrong". However, I can't > easily follow this code to tell you exactly whether this is the case or not. :) > > Generally uses of volatile in Java are, in decreasing order of probability: > > 1) completely unnecessary > 2) a sign that the code isn't actually threadsafe > 3) a sign that you are doing some complex lock-free trickery that relies on > specifics of the JVM memory model for performance reasons > > Since you appear to be using a threadsafe queue to move the records around, you > probably aren't doing 3), and so my assumption is that it's either 1) or 2) :) > > Having said that, this is just an analysis tool and we should probably get it > submitted so we can start analysing things sooner rather than later :) Your point about the queues is valid and, I believe, does obviate the need for the volatile. As you said, let's focus on the rest of it, though, and not worry about overly-safe threading constructs too much :) I'll take out the volatiles since you have pointed out the reason why it should be safe.
Volatiles removed, and I re-ran the tool. With 310,000 records processed without error on my insanely multi-core desktop, I'm confident the threadsafety was unnecessary.
getting there, thanks!! I now focused on the python file which I haven't looked before: https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/run_b... File tools/binary_size/run_binary_size_analysis.py (right): https://codereview.chromium.org/119083006/diff/230001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:35: def parse_nm(input): On 2014/01/08 00:56:45, Andrew Hayden wrote: > On 2014/01/07 19:38:30, bulach wrote: > > not sure, is this needed? > > Yes, the bloat script parses nm to convert the strings to records for the > treemap. In the end (not now), we should probably have a little pylib for > parsing nm that we can share both with this code and the eventual pythonified > version of ParallelAddr2Line. got it... there's some hidden irony in that whilst trying to reduce binary size, there's this duplication of both python and java ;) I'm looking forward to have all of this in one single language. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... File tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java (right): https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:66: // We want to convert all output paths to be relative to src, nit: I had reviewers asking to not use pronouns.. their point is that it's simpler as: // Convert all output paths to be relative to src, // so strip everything up to and including "src/". https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:73: + ". Library is expected to be within a build directory."); nit: + operator in the previous line https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:104: private static int jitter(final int value, final float percent) { nit: s/float/int/ https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:106: int delta = (r.nextBoolean() ? 1 : -1) * r.nextInt((int) (percent / 100f * value)); nit: isn't the cast equivalent to just do it in int space? ((value * percent) / 100) https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:129: * throughput can be achieved. We jitter this value by +/- 10% so that nit: ditto about "we". // Jitter this value by +/-10% so that processes are not restarted all at once. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:191: } else { nit: the previous "if" will always continue, so can remove this else and unindent the following block. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... File tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java (right): https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java:287: + "/obj/third_party/icu"); nit: + previous line https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java:301: } nit: add a \n here https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java:312: final CountDownLatch completionLatch = sink( sure, but do we care about the stderr being a separate stream at this level? wouldn't it be simpler to just redirectErrorStream() ? I suppose the parser is robust enough, and checking for the exit code would be enough, so there'd be less code here.. the full command line is already being printed at createNmProcess() should anyone really have to inspect the stderr... https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java:372: private static final CountDownLatch sink(final InputStream in, as above, I think we can sink this sink :) https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... File tools/binary_size/java/src/org/chromium/tools/binary_size/Record.java (right): https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Record.java:12: String address; yeah, so just to close the loop re: volatile... beyond torne's explanation, it'd be used when the values are actually being accessed concurrently, so that the compiler would make all the reads (and writes) go through memory rather than cached in a CPU register or something... the simplest example I can remember is something like "while (!stopped) do()" in thread X, and another thread Y do "stopped = true"... compilers (and VMs) are free to optimize reading the "stopped" value just once, since it knows nothing else will change its value... by adding the "volatile", it'll force to always access the memory to get its value.... it's obviously far more subtle, but this is the "take your grain of salt" explanation.. ;) (another use for volatile is for shared memory... say, device drivers with DMA can manipulate areas of memory that are also manipulated directly by the device... in which case, it's never safe to "cache" said value in a CPU register, all access have to through the actual memory..) anyways... in this case, Records are being created (i.e., written) in one thread, then put in the hashmap, and then much later on are being read and dumped in another thread, so there's no need to mark as volatile... https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... File tools/binary_size/run_binary_size_analysis.py (right): https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:1: #!/usr/bin/python make sure this file has a chmod +x :) I got bitten by this many, many times... https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:22: import json nit: sort order https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:24: def format_bytes(bytes): chromium's python style guide is a bit different... :( http://www.chromium.org/chromium-os/python-style-guidelines specifically: Indentation: use 2, not 4, spaces Naming: Functions and Method names use CapWords() style, not lower_with_under() The only exception to this rule is the main() function -- we do not use Main() https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:101: # TODO: make segmenting by namespace work. nit: TODO(andrewhayden) https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:110: elif path and '/' in path: nit: is this test needed? I think split will work regardless, so could just test for "path" and remove 112-113 https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:185: nit: need another \n here. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:203: '\'location\': \'' + path + '\'},\n') I think it'd be more readable as: entry = { 'size': format_bytes(size), 'symbol': symbol_type_to_human, 'location': path } out.write(json.dumps(entry)) https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:210: if outfile is not None: if it was None, the previous two lines would've failed :) https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:212: nit: another \n here (two between top levels), so there'll be another one in 227 too https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:238: out.write(' {\'size\': \'' + format_bytes(record['size']) + '\',' ditto, using json would avoid the "quoting 'hell'" https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:251: def run_pa2l(outfile, library, arch, threads, verbose=False): nit: only called on one place, can remove the default value for verbose https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:255: classpath = out_dir + '/' + buildtype + '/lib.java/binary_size_java.jar' nit: classpath = os.path.join(out_dir, build_type, 'lib.java', 'binary_size_java.jar') https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:267: '--nm', 'third_party/android_tools/ndk/toolchains/arm-linux-androideabi-4.7/prebuilt/linux-x86_64/bin/arm-linux-androideabi-nm', nit: it has to be <80cols. since it's already under parenthesis, simple string continuation would work: 'third_party/android_tools/ndk/' 'toolchains/arm-linux-androideabi-4.7/' ... https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:289: usage="""%prog [options] this whole block has to be under def main(): ... ... and then at the end: if __name__ == '__main__': sys.exit(main()) https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:369: nm_in = opts.nm_in 369-393 would be better as: symbols = GetNMSymbols(opts.nm_out, opts.library, opts.arch, opts.threads, opts.verbose) https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:391: nmfile = open(nm_in, 'r') nit: with file(nm_in, 'r') as nm_file: symbols = list(parse_nm(nmfile)) (the "with" clause will automatically call close()) https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:396: dump_largest_symbols(symbols, opts.destdir + '/largest-symbols.js', 100) os.path.join in these three places.. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:400: url = 'https://github.com/martine/webtreemap/archive/gh-pages.zip' please, get third-party reviewers approval.. also, we should mirror this on our side when we add to the bots... we don't want to keep hitting external servers on our continuous build...
First two files done (also w/ Marcus' comments). Time for a meeting :-). https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... File tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java (right): https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:30: private final Addr2LineWorker[] workers; nit: Java files in Chromium follow the Android coding style guidelines, which state that private fields need to start with an m (mWorkers). Elsewhere too. http://source.android.com/source/code-style.html#follow-field-naming-conventions https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:44: private final AtomicInteger numDeduped = new AtomicInteger(0); s/numDeduped/dedupeCount/ for consistency (most notably with the getter). https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:146: private final Process createAddr2LineProcess() The "final" here declares that subclasses of Addr2LineWorker can't override this method, instead of indicating that it returns a "final Process" which I think you may have meant? Since there are no subclasses, it doesn't serve any purpose. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:179: // Check cache nit: inconsistent "cache" vs. "dedupe" terminology. Also formulate this as a sentence please :-). https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:192: if (addressesSeen.putIfAbsent(addressLong, record) != null) { Am I missing some subtlety of putIfAbsent here, or can we avoid the lookup by just using put? We check that |record| isn't null on line 169, and the if clause on line 183 would return true of |addressLong| already had a record. It should therefore always be absent here. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:199: // If the addr2line process is new (or recycled), nit: Why the odd character offset for line breaks in this comment? Also modify this given Marcus' pronouns comment. "Create a new reader if the addr2line process is new or has been recycled. A single reader will be used for the entirety of the addr2line process' lifetime." https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:228: } This method is very long, we should try to split it up. Can we perhaps extract lines 210-228 (perhaps even 199-228) to a separate method which deals with communicating with the process? https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:296: System.err.println("warning: zombie process"); s/warning/WARNING/ to match line 185. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:303: e.printStackTrace(); Why do we write the Exception above (line 297) to STDERR, but the ones on lines 303 and 310 to STDOUT? https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:316: // ignore nit: Clarify why it's ok to ignore this. An empty catch {} statement already implies that we're ignoring it. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:327: .getParentFile().getParentFile().getCanonicalFile(); This is rather hard-coded for usage on Android. Are the libraries we're interested in *always* in src/out/{Debug,Release}/lib/? https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:334: // ignore all the 'out' directories. nit: Sentences start with capitals. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:348: skip = true; Rather than using |skip| here, you can just use the continue statement. That would also allow you to get rid of one level of nesting (the if-clause on line 338). https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:351: // Skip things like .git, .svn, ... micro nit: "Skip directories such as .git, .svn, etcetera." feels a bit less informal. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:360: // Include magic directories. nit: "Include directories which are likely to contain generated source files."? https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:384: || name.endsWith(".h") || name.endsWith(".cpp")) { Does it help to include assembly files here (.asm and .S), a few of which are scattered throughout the source code? https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... File tools/binary_size/java/src/org/chromium/tools/binary_size/NmDumper.java (right): https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/NmDumper.java:9: micro nit: double empty line. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/NmDumper.java:11: class NmDumper { This class could use a sentence or two of documentation about its purpose, and why we are simulating Nm output. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/NmDumper.java:88: private synchronized void println(PrintWriter writer, String string) { nit: Since all callers have already been declared as synchronized, do we need it here again? I'm not fully savvy about the workings of "synchronized" in the JVM.. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/NmDumper.java:111: if (writer != null) { We can avoid a level of nesting here by using the continue statement if writer==null. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/NmDumper.java:113: writer.flush(); } nit: position of the }. On line 118 as well.
https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... File tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java (right): https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:30: private final Addr2LineWorker[] workers; On 2014/01/08 16:06:23, Peter Beverloo wrote: > nit: Java files in Chromium follow the Android coding style guidelines, which > state that private fields need to start with an m (mWorkers). Elsewhere too. > > http://source.android.com/source/code-style.html#follow-field-naming-conventions I'll refactor at the same time that I fix the python style, in the commit after your other ctructural/functional comments are addressed. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:44: private final AtomicInteger numDeduped = new AtomicInteger(0); On 2014/01/08 16:06:23, Peter Beverloo wrote: > s/numDeduped/dedupeCount/ for consistency (most notably with the getter). Done. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:66: // We want to convert all output paths to be relative to src, On 2014/01/08 15:04:00, bulach wrote: > nit: I had reviewers asking to not use pronouns.. their point is that it's > simpler as: > // Convert all output paths to be relative to src, > // so strip everything up to and including "src/". Done. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:73: + ". Library is expected to be within a build directory."); On 2014/01/08 15:04:00, bulach wrote: > nit: + operator in the previous line Done. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:106: int delta = (r.nextBoolean() ? 1 : -1) * r.nextInt((int) (percent / 100f * value)); On 2014/01/08 15:04:00, bulach wrote: > nit: isn't the cast equivalent to just do it in int space? > ((value * percent) / 100) Done. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:129: * throughput can be achieved. We jitter this value by +/- 10% so that On 2014/01/08 15:04:00, bulach wrote: > nit: ditto about "we". > // Jitter this value by +/-10% so that processes are not restarted all at once. Done. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:146: private final Process createAddr2LineProcess() On 2014/01/08 16:06:23, Peter Beverloo wrote: > The "final" here declares that subclasses of Addr2LineWorker can't override this > method, instead of indicating that it returns a "final Process" which I think > you may have meant? Since there are no subclasses, it doesn't serve any > purpose. Old habit. I tend to make things final and open them up as necessary, since it's always safe to remove the final keyword. I think there used to be (may not be any longer, I don't know...) performance improvements that the compiler would make if things were final, since final methods were always safe to inline. But it doesn't matter here, and I'll remove it. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:179: // Check cache On 2014/01/08 16:06:23, Peter Beverloo wrote: > nit: inconsistent "cache" vs. "dedupe" terminology. Also formulate this as a > sentence please :-). Done. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:191: } else { On 2014/01/08 15:04:00, bulach wrote: > nit: the previous "if" will always continue, so can remove this else and > unindent the following block. Done. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:192: if (addressesSeen.putIfAbsent(addressLong, record) != null) { On 2014/01/08 16:06:23, Peter Beverloo wrote: > Am I missing some subtlety of putIfAbsent here, or can we avoid the lookup by > just using put? We check that |record| isn't null on line 169, and the if > clause on line 183 would return true of |addressLong| already had a record. It > should therefore always be absent here. We could but the dedupe count could then be incorrect. It's a race on the first time we insert into the deduplication cache: if two workers both add it with put, neither will count the dedupe when in reality one of them should (even though it may have done extraneous work). Since this is non-obvious I'll add a comment to this effect. This was a bug I fixed due to marcus' previous comments... https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:199: // If the addr2line process is new (or recycled), On 2014/01/08 16:06:23, Peter Beverloo wrote: > nit: Why the odd character offset for line breaks in this comment? Also modify > this given Marcus' pronouns comment. > > "Create a new reader if the addr2line process is new or has been recycled. A > single reader will be used for the entirety of the addr2line process' lifetime." I ocassionally start new sentences on their own lines. Anyhow, changed. I also nuked the pronoun "we" from this entire file. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:228: } On 2014/01/08 16:06:23, Peter Beverloo wrote: > This method is very long, we should try to split it up. Can we perhaps extract > lines 210-228 (perhaps even 199-228) to a separate method which deals with > communicating with the process? I have pulled out two chunks: deduplication and location processing. This cuts about half the method away. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:303: e.printStackTrace(); On 2014/01/08 16:06:23, Peter Beverloo wrote: > Why do we write the Exception above (line 297) to STDERR, but the ones on lines > 303 and 310 to STDOUT? Whoops. Actually, the default with no argument passed is stderr, so I removed to unnecessary passing of System.err. They all should go to System.err https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:316: // ignore On 2014/01/08 16:06:23, Peter Beverloo wrote: > nit: Clarify why it's ok to ignore this. An empty catch {} statement already > implies that we're ignoring it. Added comments. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:327: .getParentFile().getParentFile().getCanonicalFile(); On 2014/01/08 16:06:23, Peter Beverloo wrote: > This is rather hard-coded for usage on Android. Are the libraries we're > interested in *always* in src/out/{Debug,Release}/lib/? Added TODO. It's not clear to me how else to do this without a fair bit of work to add a command line parameter used specifically to set the directory to be processed as part of deduplication. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:334: // ignore all the 'out' directories. On 2014/01/08 16:06:23, Peter Beverloo wrote: > nit: Sentences start with capitals. Done. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:348: skip = true; On 2014/01/08 16:06:23, Peter Beverloo wrote: > Rather than using |skip| here, you can just use the continue statement. That > would also allow you to get rid of one level of nesting (the if-clause on line > 338). Done. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:351: // Skip things like .git, .svn, ... On 2014/01/08 16:06:23, Peter Beverloo wrote: > micro nit: "Skip directories such as .git, .svn, etcetera." feels a bit less > informal. Done. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:360: // Include magic directories. On 2014/01/08 16:06:23, Peter Beverloo wrote: > nit: "Include directories which are likely to contain generated source files."? Done. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Addr2LineWorkerPool.java:384: || name.endsWith(".h") || name.endsWith(".cpp")) { On 2014/01/08 16:06:23, Peter Beverloo wrote: > Does it help to include assembly files here (.asm and .S), a few of which are > scattered throughout the source code? I don't know, but I'm not opposed. A bit of grepping and googling has yielded a few more likely candidates that I've added: private static final String[] INTERESTING_FILE_ENDINGS = new String[]{ ".c", ".cc", ".h", ".cp", ".cpp", ".cxx", ".c++", ".asm", ".inc", ".s", ".hxx" }; Sadly this didn't make any difference as far as I can tell. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... File tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java (right): https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java:287: + "/obj/third_party/icu"); On 2014/01/08 15:04:00, bulach wrote: > nit: + previous line Done. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java:301: } On 2014/01/08 15:04:00, bulach wrote: > nit: add a \n here Done. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java:312: final CountDownLatch completionLatch = sink( On 2014/01/08 15:04:00, bulach wrote: > sure, but do we care about the stderr being a separate stream at this level? > wouldn't it be simpler to just redirectErrorStream() ? > > I suppose the parser is robust enough, and checking for the exit code would be > enough, so there'd be less code here.. the full command line is already being > printed at createNmProcess() should anyone really have to inspect the stderr... Since the sink code is already used to process stdout, I don't think it's a big deal to have stderr processed the same way. And I'd prefer to keep stderr on stderr rather than commingling with stdout unless there's a good reason to combine them. If you feel strongly about this I'm open to further discussion, but I am leaving this as-is for now. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/ParallelAddress2Line.java:372: private static final CountDownLatch sink(final InputStream in, On 2014/01/08 15:04:00, bulach wrote: > as above, I think we can sink this sink :) We are still using this for stdout processing and sadly we lack Java 7's redirectInput: http://docs.oracle.com/javase/7/docs/api/java/lang/ProcessBuilder.html#redire... https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... File tools/binary_size/java/src/org/chromium/tools/binary_size/Record.java (right): https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/Record.java:12: String address; Thanks. I am fairly familiar with the Java Memory Model and the concepts of memory fences and all that good jazz. I missed the point that torne@ had made about using queues to pass them around, which is totally correct; since all the modifications that I need to see happen-before any of the actual reads, it's accurate that there is no need for volatile here. Your explanation is, for the record, very well written :) https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... File tools/binary_size/run_binary_size_analysis.py (right): https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:1: #!/usr/bin/python On 2014/01/08 15:04:00, bulach wrote: > make sure this file has a chmod +x :) > I got bitten by this many, many times... Thank you for the reminder. I just ran: git update-index --chmod=+x tools/binary_size/run_binary_size_analysis.py Hopefully that will do the trick :p https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:22: import json On 2014/01/08 15:04:00, bulach wrote: > nit: sort order Done. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:24: def format_bytes(bytes): On 2014/01/08 15:04:00, bulach wrote: > chromium's python style guide is a bit different... :( > http://www.chromium.org/chromium-os/python-style-guidelines > specifically: > > Indentation: use 2, not 4, spaces > Naming: Functions and Method names use CapWords() style, not lower_with_under() > The only exception to this rule is the main() function -- we do not use Main() I cobbled this together from an older Chromium-authored script that was never checked in; I'll adapt it to this style in a separate commit to avoid conflating style with functional changes. Look for that in the commit after the one addressing your current comments. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:101: # TODO: make segmenting by namespace work. On 2014/01/08 15:04:00, bulach wrote: > nit: TODO(andrewhayden) Done. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:110: elif path and '/' in path: On 2014/01/08 15:04:00, bulach wrote: > nit: is this test needed? I think split will work regardless, so could just test > for "path" and remove 112-113 Done. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:185: On 2014/01/08 15:04:00, bulach wrote: > nit: need another \n here. Done. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:203: '\'location\': \'' + path + '\'},\n') On 2014/01/08 15:04:00, bulach wrote: > I think it'd be more readable as: > entry = { 'size': format_bytes(size), > 'symbol': symbol_type_to_human, > 'location': path > } > out.write(json.dumps(entry)) Sorry, still adapting to python. Makes sense, fixed. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:210: if outfile is not None: On 2014/01/08 15:04:00, bulach wrote: > if it was None, the previous two lines would've failed :) No, outfile versus out[stream]. The code always goes down the file path, so I think I'll just take out the stdout support now... the first version of the script was meant to be redirected to a file, but as the script now outputs several files this no longer makes sense. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:212: On 2014/01/08 15:04:00, bulach wrote: > nit: another \n here (two between top levels), so there'll be another one in 227 > too Done. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:238: out.write(' {\'size\': \'' + format_bytes(record['size']) + '\',' On 2014/01/08 15:04:00, bulach wrote: > ditto, using json would avoid the "quoting 'hell'" Done. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:251: def run_pa2l(outfile, library, arch, threads, verbose=False): On 2014/01/08 15:04:00, bulach wrote: > nit: only called on one place, can remove the default value for verbose Done. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:255: classpath = out_dir + '/' + buildtype + '/lib.java/binary_size_java.jar' On 2014/01/08 15:04:00, bulach wrote: > nit: > classpath = os.path.join(out_dir, build_type, 'lib.java', > 'binary_size_java.jar') Done. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:267: '--nm', 'third_party/android_tools/ndk/toolchains/arm-linux-androideabi-4.7/prebuilt/linux-x86_64/bin/arm-linux-androideabi-nm', On 2014/01/08 15:04:00, bulach wrote: > nit: it has to be <80cols. > since it's already under parenthesis, simple string continuation would work: > 'third_party/android_tools/ndk/' > 'toolchains/arm-linux-androideabi-4.7/' > ... I've cleaned this up a bit and now use os.path.join here as well. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:289: usage="""%prog [options] On 2014/01/08 15:04:00, bulach wrote: > this whole block has to be under > def main(): > ... > > ... and then at the end: > > > if __name__ == '__main__': > sys.exit(main()) Done. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:369: nm_in = opts.nm_in On 2014/01/08 15:04:00, bulach wrote: > 369-393 would be better as: > > symbols = GetNMSymbols(opts.nm_out, opts.library, opts.arch, opts.threads, > opts.verbose) Oh yes, I had been meaning to extract this! And more, actually. This is done, and I may clean it up a bit more in a later commit. I think main should be as minimal as possible. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:391: nmfile = open(nm_in, 'r') On 2014/01/08 15:04:00, bulach wrote: > nit: > > with file(nm_in, 'r') as nm_file: > symbols = list(parse_nm(nmfile)) > > > (the "with" clause will automatically call close()) Careful. You are in danger of making me into a half-way decent python programmer. Done. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:396: dump_largest_symbols(symbols, opts.destdir + '/largest-symbols.js', 100) On 2014/01/08 15:04:00, bulach wrote: > os.path.join in these three places.. Done. https://codereview.chromium.org/119083006/diff/410001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:400: url = 'https://github.com/martine/webtreemap/archive/gh-pages.zip' On 2014/01/08 15:04:00, bulach wrote: > please, get third-party reviewers approval.. > > also, we should mirror this on our side when we add to the bots... > we don't want to keep hitting external servers on our continuous build... Will add TODO for the latter part and will email third-party today. Long term I think we'll move to D3 as suggested by several other folks, which I think obviates the need for this.
For my next trick, formatting the python and java code to meet Chromium coding standards. Stay tuned, next patchset will have zero functional changes.
Ready for re-review. All presubmit warnings fixed, and Python/Java code adhere to Chromium standards.
I have sent mail to both security and open source reviewers about the use of the MicroOptions class in the Java code and the webtreemap library in the web output.
lgtm, the remaining comments are just nits, feel free to land once you have third party clearance.. thanks a ton, looking forward to see the results! https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... File tools/binary_size/run_binary_size_analysis.py (right): https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:23: nit: need another \n https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:144: if fileKey not in tree['children']: nit: file_key https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:145: tree['children'][fileKey] = {'sizes': {}, 'size': 0} nit: would be simpler with "import collections" above, then here instead of: 'sizes': {} use: 'sizes': collections.defaultdict(int) then it'll be possible to remove all places like: if not '[vtable]' in tree['sizes']: tree['sizes']['[vtable]'] = 0 and just keep: tree['sizes']['[vtable]'] += size even simpler, since this is essentially just translating / mapping types to an entry (except in two cases), would be: mapping = { 'R': '[rodata]', 'D': '[data]', ... ] then: if type.upper() in mapping: tree[sizes][mapping[type.upper()]] += size this should take care of everything but 'vtable for' and the '[other]' entry, but still, would make this more compact. https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:201: """ Convert the output of TreeifySymbols to a format suitable for a nit: (g)pylint would complain, the first line of the docstring should be a sentence on its own, followed by a \n with the longer explanation.. https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:207: for childName, child in tree['children'].iteritems(): nit: child_name https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:212: childJson = {'name': kind + ' (' + FormatBytes(size) + ')', nit: child_json https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:214: cssClass = None nit: css_class, but again, this would be simpler with a map: css_class = { '[vtable]': 'vtable', '[rodata]': 'read-only-data', ... }.get(kind) https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:334: def RunPA2L(outfile, library, arch, threads, verbose): nit: perhaps RunParallelAddress2Line would be clearer? https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:409: parser.add_option('--nm-in', dest='nm_in', metavar='PATH', nit: dest is automatically computed out of name, no need to specify it here.. https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:432: 'This argument is only valid when using --library.') nit: default='host-native' also, could do with choices=['list', 'of', 'options'] https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:433: parser.add_option('--pa2l-threads', dest='threads', nit: perhaps just --jobs ? I think that's more inline with ninja also, default=1 and remove 463 below.. https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:443: parser.add_option('--nm-out', dest='nm_out', nit: remove dest https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:466: opts.arch = 'host-native' nit: as above, remove the 463-466.. https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:471: '[host-native,android-arm,android-mips,android-x86]') as above, make it a choices
it's cool. it looks not-android-specific. Do you have plan to support other platforms, especially linux?
On 2014/01/10 14:04:30, dshwang wrote: > it's cool. it looks not-android-specific. Do you have plan to support other > platforms, especially linux? The only reason this is currently tied to Android is that it has Java components to run addr2line in parallel. This portion needs to be ported to Python using the Python multiprocessing library, and then we can ditch the Java code and move away from being tied to Android. We also need to add a few more config options to avoid hardcoding the expected relative paths from the library files, but that should be easy. I hope to give the Python port a try shortly, but it could take a little while.
For posterity, there is a demo of the current functionality posted here: http://andrewhayden.github.io/binary_size_example/
Here the the review for checking in webtreemap, which I expect to land later today: https://codereview.chromium.org/131463006/ After this I'll refactor the tool to use the webtreemap in third_party instead of downloading it from the net. In the meantime, I'm going to also remove the command line argument parser so we can unblock from the OSS side.
Responses to Marcus https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... File tools/binary_size/run_binary_size_analysis.py (right): https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:23: On 2014/01/10 11:23:24, bulach wrote: > nit: need another \n Done. https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:144: if fileKey not in tree['children']: On 2014/01/10 11:23:24, bulach wrote: > nit: file_key Done. https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:145: tree['children'][fileKey] = {'sizes': {}, 'size': 0} On 2014/01/10 11:23:24, bulach wrote: > nit: would be simpler with "import collections" above, then here instead of: > 'sizes': {} > > use: > 'sizes': collections.defaultdict(int) > > then it'll be possible to remove all places like: > if not '[vtable]' in tree['sizes']: > tree['sizes']['[vtable]'] = 0 > > and just keep: > tree['sizes']['[vtable]'] += size > > even simpler, since this is essentially just translating / mapping types to an > entry (except in two cases), would be: > mapping = { > 'R': '[rodata]', > 'D': '[data]', > ... > ] > > then: > if type.upper() in mapping: > tree[sizes][mapping[type.upper()]] += size > > this should take care of everything but 'vtable for' and the '[other]' entry, > but still, would make this more compact. I had never heard of defaultdict, but yes, this makes things simpler. I've ditched al the "if not exists, create" logic as you suggested. I think it's clearer to just leave the remaining if-statements rather than mix them with the vtable/other constructs, though, so I'm going to leave that be. I'll remember the mapping class thing, though :) https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:201: """ Convert the output of TreeifySymbols to a format suitable for a On 2014/01/10 11:23:24, bulach wrote: > nit: (g)pylint would complain, the first line of the docstring should be a > sentence on its own, followed by a \n with the longer explanation.. Done. https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:207: for childName, child in tree['children'].iteritems(): On 2014/01/10 11:23:24, bulach wrote: > nit: child_name Done. https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:212: childJson = {'name': kind + ' (' + FormatBytes(size) + ')', On 2014/01/10 11:23:24, bulach wrote: > nit: child_json Done. https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:214: cssClass = None On 2014/01/10 11:23:24, bulach wrote: > nit: css_class, but again, this would be simpler with a map: > > css_class = { > '[vtable]': 'vtable', > '[rodata]': 'read-only-data', > ... > }.get(kind) Done. https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:334: def RunPA2L(outfile, library, arch, threads, verbose): On 2014/01/10 11:23:24, bulach wrote: > nit: perhaps RunParallelAddress2Line would be clearer? Done. https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:409: parser.add_option('--nm-in', dest='nm_in', metavar='PATH', On 2014/01/10 11:23:24, bulach wrote: > nit: dest is automatically computed out of name, no need to specify it here.. Done. https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:432: 'This argument is only valid when using --library.') On 2014/01/10 11:23:24, bulach wrote: > nit: default='host-native' > > also, could do with choices=['list', 'of', 'options'] We don't just use the default because I emit warnings if you do things like specify the arch with a --nm-in, since this is a nonsensical parameter combination. AFAIK there's no easy and clear way to know whether the default string is there because it was parsed or explicitly passed. You can apparently suck all the values out of the option parser and have it re-parse them or something like that, but it seems like this is simpler. https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:433: parser.add_option('--pa2l-threads', dest='threads', On 2014/01/10 11:23:24, bulach wrote: > nit: perhaps just --jobs ? I think that's more inline with ninja > > also, default=1 and remove 463 below.. Same discussion about defaults in the face of --nm-in, but will call it jobs instead. https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:443: parser.add_option('--nm-out', dest='nm_out', On 2014/01/10 11:23:24, bulach wrote: > nit: remove dest Done. https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:466: opts.arch = 'host-native' On 2014/01/10 11:23:24, bulach wrote: > nit: as above, remove the 463-466.. I don't know how to do this easily while retaining the desired behavior, since I won't know if the values came from defaults or were passed on the command line. As noted above, it's apparently possible - but I'm not sure it will be cleaner. https://codereview.chromium.org/119083006/diff/710001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:471: '[host-native,android-arm,android-mips,android-x86]') On 2014/01/10 11:23:24, bulach wrote: > as above, make it a choices Done.
With the advent of patchset 28, I now consider v1.0 of this tool complete and ready to check in as soon as the webtreemap library makes it into third_party, which should be later today. PTAL. Thanks to all who have reviewed.
https://codereview.chromium.org/119083006/diff/940001/tools/binary_size/READM... File tools/binary_size/README.txt (right): https://codereview.chromium.org/119083006/diff/940001/tools/binary_size/READM... tools/binary_size/README.txt:105: 6. The Javascript code in the HTML report Assumes code lives in Chromium for Shortcoming number 8: README.txt has two shortcomings numbered as 6.
https://codereview.chromium.org/119083006/diff/1000001/tools/binary_size/READ... File tools/binary_size/README.txt (right): https://codereview.chromium.org/119083006/diff/1000001/tools/binary_size/READ... tools/binary_size/README.txt:110: 4. Feature Requests and Bug Reports Gah.
lgtm, just tiny nits, thanks! https://codereview.chromium.org/119083006/diff/920001/tools/binary_size/java/... File tools/binary_size/java/src/org/chromium/tools/binary_size/NmDumper.java (right): https://codereview.chromium.org/119083006/diff/920001/tools/binary_size/java/... tools/binary_size/java/src/org/chromium/tools/binary_size/NmDumper.java:148: writer.flush(); } nit: } in the next line (ditto 153 below) https://codereview.chromium.org/119083006/diff/940001/tools/binary_size/run_b... File tools/binary_size/run_binary_size_analysis.py (right): https://codereview.chromium.org/119083006/diff/940001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:155: elif 'r' == type or 'R' == type: ok, let's leave the map for a v2, but it'd be simpler to just have: type = type.lower() before the if chain and check for just one of them. https://codereview.chromium.org/119083006/diff/940001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:193: The format is very similar, with the notable exceptions being nit: needs to be aligned with the """, i.e., indented by 2 https://codereview.chromium.org/119083006/diff/940001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:205: css_class = { nit: may want to put this as a constant at the top rather than inside the loop
https://codereview.chromium.org/119083006/diff/940001/tools/binary_size/run_b... File tools/binary_size/run_binary_size_analysis.py (right): https://codereview.chromium.org/119083006/diff/940001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:155: elif 'r' == type or 'R' == type: On 2014/01/16 15:01:59, bulach wrote: > ok, let's leave the map for a v2, but it'd be simpler to just have: > type = type.lower() before the if chain and check for just one of them. Ha, uh duh, yes. Sorry :) https://codereview.chromium.org/119083006/diff/940001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:193: The format is very similar, with the notable exceptions being On 2014/01/16 15:01:59, bulach wrote: > nit: needs to be aligned with the """, i.e., indented by 2 Done. https://codereview.chromium.org/119083006/diff/940001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:205: css_class = { On 2014/01/16 15:01:59, bulach wrote: > nit: may want to put this as a constant at the top rather than inside the loop Done.
https://codereview.chromium.org/119083006/diff/1060001/tools/binary_size/READ... File tools/binary_size/README.txt (right): https://codereview.chromium.org/119083006/diff/1060001/tools/binary_size/READ... tools/binary_size/README.txt:54: producing an HTMl report in /tmp/report: The 'L' in HTML should not be lowercase. https://codereview.chromium.org/119083006/diff/1060001/tools/binary_size/READ... tools/binary_size/README.txt:98: within the Chromium source tree due to missing symbols in the ICU ASM It's not missing symbols, it's that we don't provide a size entry for the symbol. Torne has a one-line fix for this that isn't yet (AFAIK) checked in. https://codereview.chromium.org/119083006/diff/1060001/tools/binary_size/READ... tools/binary_size/README.txt:101: binaries exist in ../../third_party. This is true only when dealing with ... in the android_ndk directory.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andrewhayden@chromium.org/119083006/11...
Message was sent while issue was closed.
Change committed as 245405
Message was sent while issue was closed.
https://codereview.chromium.org/119083006/diff/1100001/tools/binary_size/run_... File tools/binary_size/run_binary_size_analysis.py (right): https://codereview.chromium.org/119083006/diff/1100001/tools/binary_size/run_... tools/binary_size/run_binary_size_analysis.py:480: 'webtreemap-gh-pages') Whoops, this is desynced from the final approach of checking in webtreemap. We need to just be copying from src, not src/webtreemap-gh-pages.
Message was sent while issue was closed.
Hi, why was this written in Java? We usually do tooling stuff in python.
Message was sent while issue was closed.
On 2014/01/29 03:33:17, Nico wrote: > Hi, > > why was this written in Java? We usually do tooling stuff in python. Indeed, the tool was written in the downstream repository for android only, which is why it used java initially. Andrew will be porting it to python so it can be used in other platforms as well but we thought it'd be a good idea to upstream it as is so opera and other folks can start using it on their binaries. Andrew is there a bug to port it to python?
Message was sent while issue was closed.
On 2014/01/29 09:08:02, Miguel Garcia wrote: > Indeed, the tool was written in the downstream repository for android only, > which is why it used java initially. Andrew will be porting it to python so it > can be used in other platforms as well but we thought it'd be a good idea to > upstream it as is so opera and other folks can start using it on their binaries. > > Andrew is there a bug to port it to python? There is now: https://code.google.com/p/chromium/issues/detail?id=339059
Cool, thanks for explaining! On Wed, Jan 29, 2014 at 2:42 AM, <andrewhayden@chromium.org> wrote: > On 2014/01/29 09:08:02, Miguel Garcia wrote: > >> Indeed, the tool was written in the downstream repository for android >> only, >> which is why it used java initially. Andrew will be porting it to python >> so it >> can be used in other platforms as well but we thought it'd be a good idea >> to >> upstream it as is so opera and other folks can start using it on their >> > binaries. > > Andrew is there a bug to port it to python? >> > > There is now: > https://code.google.com/p/chromium/issues/detail?id=339059 > > https://codereview.chromium.org/119083006/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
