Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
303453003:
binary_size: Easier-to-read output (Closed)
|
Created:
6 years, 7 months ago by Daniel Bratell Modified:
6 years, 6 months ago CC:
chromium-reviews, Primiano Tucci (use gerrit) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptionbinary_size: Easier-to-read output
It's easy to have to have to click a few levels down (in my case down
into / -> home -> bratell -> src -> chromium -> src). Remove the part
that seems to be an unnecessary prefix by assuming that everything
above cwd is not interesting.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=279448
Patch Set 1 #
Total comments: 3
Patch Set 2 : Output relative to CWD fixes #Patch Set 3 : Rebased to newer master #Messages
Total messages: 20 (0 generated)
Given the size of the change, you probably want to merge this into another of your CLs :) https://codereview.chromium.org/303453003/diff/1/tools/binary_size/run_binary... File tools/binary_size/run_binary_size_analysis.py (right): https://codereview.chromium.org/303453003/diff/1/tools/binary_size/run_binary... tools/binary_size/run_binary_size_analysis.py:97: if file_path and file_path != "??": Please be consistent with quotes (we tend to use single quotes pretty much everywhere in chromium code) https://codereview.chromium.org/303453003/diff/1/tools/binary_size/run_binary... tools/binary_size/run_binary_size_analysis.py:98: file_path = os.path.abspath(file_path) Lines 98-100 look like re-implementing python os.path.relpath ;) * https://docs.python.org/2/library/os.path.html https://codereview.chromium.org/303453003/diff/1/tools/binary_size/run_binary... tools/binary_size/run_binary_size_analysis.py:442: def progress_output(): Hmm what is the rationale of defining and calling a nested function in the middle of another function (note that you have still code referring to the outer RunElfSymbolizer) after the inner function? It doesn't look easier to read, it is the exact same code as before, with two extra line. It doesn't either look being reused anywhere.
This seems to me like more of a problem with the design of the tool: we don't specify a "base" directory. We can't, really, because nm/addr2line don't necessarily have that data. IMO, treating any path in the tree as special might lead to later errors, either in the data itself or in assumptions about it. I'd prefer that we not do this, at least not this way. What I'd find more palatable - and what could be a good general solution - is to allow as many arg pairs of the following as one wishes: --alias /path/you/want/to/shorten=/shortpath It would accomplish the same thing and force the user to make a concious choice to compress path segments in a way that they wish, explicitly. Shouldn't add too much work. WDYT?
On 2014/05/27 10:15:41, Andrew Hayden wrote: > This seems to me like more of a problem with the design of the tool: we don't > specify a "base" directory. We can't, really, because nm/addr2line don't > necessarily have that data. > > IMO, treating any path in the tree as special might lead to later errors, either > in the data itself or in assumptions about it. I'd prefer that we not do this, > at least not this way. > > What I'd find more palatable - and what could be a good general solution - is to > allow as many arg pairs of the following as one wishes: > > --alias /path/you/want/to/shorten=/shortpath > > It would accomplish the same thing and force the user to make a concious choice > to compress path segments in a way that they wish, explicitly. Shouldn't add too > much work. WDYT? I am not in love with this patch but I think something has to be done because it isn't very practical the way it looks right now. Your demo output had the data in / but there it looked like you had put the source in /, something that is not practical in general. The --alias proposal would work but I don't think it's practical enough. It would be a mouthful (penful?) to write and since the output would be "working" without it I don't think people would go looking for the setting and instead just be annoyed by the output. The idea is to select a good start point for the data browsing. Currently it's /, and with this patch it becomes some kind of hybrid between / and cwd. cwd for everything below cwd, otherwise /. Maybe it's better to always use cwd, though it would possibly lead to a few clicks on ".." instead of the current few clicks on "home", "bratell", "src", "chromium", "src". Not sure what would give the best results. Almost all source is inside cwd, but the templates in the standard libraries are not.
I agree that there's not a generally awesome solution. I guess let's go with cwd.
Please take another look. One of the problems with paths turned out to be that addr2line returned relative paths but at the time they were resolved we did not know what they were relative against. This changes it so that we resolve paths immediately in the nm step. os.path.relname (which I had forgotten about) turns out produce a lot of ".." for the /usr/include/... files. So I skipped that part here.
LGTM
The CQ bit was checked by bratell@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bratell@opera.com/303453003/20001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/14438) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/17554)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was checked by bratell@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bratell@opera.com/303453003/20001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/20327) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/24158)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/20328)
The CQ bit was checked by bratell@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bratell@opera.com/303453003/40001
Message was sent while issue was closed.
Change committed as 279448 |
