Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2810813003:
Add resource_sizes diffs to diagnose_apk_bloat.py. (Closed)
|
Created:
3 years, 8 months ago by estevenson Modified:
3 years, 8 months ago Reviewers:
agrieve CC:
chromium-reviews, wnwen+watch_chromium.org, estevenson+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd resource_sizes diffs to diagnose_apk_bloat.py.
This CL adds the ability to show the differences in resource_sizes.py
metrics using dianose_apk_bloat.py. It also includes a few other
updates:
* Simplfy output and write diff output to file
* Generate and save .size files (and save .so files for Disassemble())
* Be nicer by restoring user branches after script runs
BUG=710076
Review-Url: https://codereview.chromium.org/2810813003
Cr-Commit-Position: refs/heads/master@{#464023}
Committed: https://chromium.googlesource.com/chromium/src/+/08f973975155004f8a9bc6887f6e9551cedbac1d
Patch Set 1 #
Total comments: 12
Patch Set 2 : Address comments + rebase onto supersize #Patch Set 3 : Put ProduceDiff back into BaseDiff #
Total comments: 4
Patch Set 4 : Change output file naming convention + append to it #
Total comments: 1
Patch Set 5 : AppendResults #Patch Set 6 : Comment AppendResults #Messages
Total messages: 16 (6 generated)
estevenson@chromium.org changed reviewers: + agrieve@chromium.org
ptal Andrew.
Output looks like:
******************************Resource Sizes Diff******************************
Summary:
Normalized APK size: +2 bytes
Details:
+2,412 bytes ChromePublic.apk_InstallSize APK size
+2,412 bytes ChromePublic.apk_InstallSize Estimated installed size
+2,410 bytes ChromePublic.apk_InstallBreakdown Native code size
+2,410 bytes ChromePublic.apk_Breakdown Native code size
+1,790 bytes ChromePublic.apk_TransferSize Transfer size (deflate)
+1,068 bytes ChromePublic.apk_MainLibInfo text
+2 bytes ChromePublic.apk_Specifics normalized apk size
+0 entries ChromePublic.apk_Dex fields
+0 entries ChromePublic.apk_Dex methods
+0 entries ChromePublic.apk_Dex types
+0 entries ChromePublic.apk_Dex strings
+0 bytes ChromePublic.apk_DexCache DexCache
Looks great! Just a few small comments. https://codereview.chromium.org/2810813003/diff/1/tools/binary_size/diagnose_... File tools/binary_size/diagnose_apk_bloat.py (right): https://codereview.chromium.org/2810813003/diff/1/tools/binary_size/diagnose_... tools/binary_size/diagnose_apk_bloat.py:95: v['units'])) nit: Let's add logic to have it fail gracefully if the same unit is not present in both. E.g. we might change what's measured over time and still want the ability to diff. https://codereview.chromium.org/2810813003/diff/1/tools/binary_size/diagnose_... tools/binary_size/diagnose_apk_bloat.py:232: map2size.main(['tools/binary_size/map2size.py', '--output-directory', Probably better to use subprocess for this since it's so RAM hungry and uses atexit https://codereview.chromium.org/2810813003/diff/1/tools/binary_size/diagnose_... tools/binary_size/diagnose_apk_bloat.py:251: def _NormalizeRevs(*revs): nit: This interface seems a bit awkward to me. How about calling it "_NormalizeRev" and call it twice. https://codereview.chromium.org/2810813003/diff/1/tools/binary_size/diagnose_... tools/binary_size/diagnose_apk_bloat.py:311: parser.add_argument('--save-results', Rather than adding another option, I think it would be better to always save the diff to a file, and then just cat the file to print it in the logs. https://codereview.chromium.org/2810813003/diff/1/tools/binary_size/diagnose_... tools/binary_size/diagnose_apk_bloat.py:315: help='Run some extra steps that take longer to complete.') nit: Should say exactly which options this enables here so that devs can reason about whether it's worth it for them. https://codereview.chromium.org/2810813003/diff/1/tools/binary_size/diagnose_... tools/binary_size/diagnose_apk_bloat.py:350: archive_dirs = [os.path.join(args.archive_dir, '%d-%s' % (i + 1, rev)) nit: might be better as len(revs) - i, so that the larger number means latter in time.
Description was changed from ========== Add resource_sizes diffs to diagnose_apk_bloat.py. This CL adds the ability to show the differences in resource_sizes.py metrics using dianose_apk_bloat.py. It also includes a few other updates: * Simplfy output and add option to write diffs to file * Generate and save .size files (and save .so files for Disassemble()) * Be nicer by restoring user branches after script runs BUG=710076 ========== to ========== Add resource_sizes diffs to diagnose_apk_bloat.py. This CL adds the ability to show the differences in resource_sizes.py metrics using dianose_apk_bloat.py. It also includes a few other updates: * Simplfy output and write diff output to file * Generate and save .size files (and save .so files for Disassemble()) * Be nicer by restoring user branches after script runs BUG=710076 ==========
https://codereview.chromium.org/2810813003/diff/1/tools/binary_size/diagnose_... File tools/binary_size/diagnose_apk_bloat.py (right): https://codereview.chromium.org/2810813003/diff/1/tools/binary_size/diagnose_... tools/binary_size/diagnose_apk_bloat.py:95: v['units'])) On 2017/04/11 14:10:32, agrieve wrote: > nit: Let's add logic to have it fail gracefully if the same unit is not present > in both. E.g. we might change what's measured over time and still want the > ability to diff. Done. https://codereview.chromium.org/2810813003/diff/1/tools/binary_size/diagnose_... tools/binary_size/diagnose_apk_bloat.py:232: map2size.main(['tools/binary_size/map2size.py', '--output-directory', On 2017/04/11 14:10:32, agrieve wrote: > Probably better to use subprocess for this since it's so RAM hungry and uses > atexit Done. https://codereview.chromium.org/2810813003/diff/1/tools/binary_size/diagnose_... tools/binary_size/diagnose_apk_bloat.py:251: def _NormalizeRevs(*revs): On 2017/04/11 14:10:32, agrieve wrote: > nit: This interface seems a bit awkward to me. How about calling it > "_NormalizeRev" and call it twice. Done. Still left revs as a list (instead of a rev_without and rev_with) since this will be required for https://crbug.com/710076 anyways. https://codereview.chromium.org/2810813003/diff/1/tools/binary_size/diagnose_... tools/binary_size/diagnose_apk_bloat.py:311: parser.add_argument('--save-results', On 2017/04/11 14:10:32, agrieve wrote: > Rather than adding another option, I think it would be better to always save the > diff to a file, and then just cat the file to print it in the logs. Done. https://codereview.chromium.org/2810813003/diff/1/tools/binary_size/diagnose_... tools/binary_size/diagnose_apk_bloat.py:315: help='Run some extra steps that take longer to complete.') On 2017/04/11 14:10:32, agrieve wrote: > nit: Should say exactly which options this enables here so that devs can reason > about whether it's worth it for them. Done. https://codereview.chromium.org/2810813003/diff/1/tools/binary_size/diagnose_... tools/binary_size/diagnose_apk_bloat.py:350: archive_dirs = [os.path.join(args.archive_dir, '%d-%s' % (i + 1, rev)) On 2017/04/11 14:10:32, agrieve wrote: > nit: might be better as len(revs) - i, so that the larger number means latter in > time. Done.
On 2017/04/11 21:53:16, estevenson wrote: > https://codereview.chromium.org/2810813003/diff/1/tools/binary_size/diagnose_... > File tools/binary_size/diagnose_apk_bloat.py (right): > > https://codereview.chromium.org/2810813003/diff/1/tools/binary_size/diagnose_... > tools/binary_size/diagnose_apk_bloat.py:95: v['units'])) > On 2017/04/11 14:10:32, agrieve wrote: > > nit: Let's add logic to have it fail gracefully if the same unit is not > present > > in both. E.g. we might change what's measured over time and still want the > > ability to diff. > > Done. > > https://codereview.chromium.org/2810813003/diff/1/tools/binary_size/diagnose_... > tools/binary_size/diagnose_apk_bloat.py:232: > map2size.main(['tools/binary_size/map2size.py', '--output-directory', > On 2017/04/11 14:10:32, agrieve wrote: > > Probably better to use subprocess for this since it's so RAM hungry and uses > > atexit > > Done. > > https://codereview.chromium.org/2810813003/diff/1/tools/binary_size/diagnose_... > tools/binary_size/diagnose_apk_bloat.py:251: def _NormalizeRevs(*revs): > On 2017/04/11 14:10:32, agrieve wrote: > > nit: This interface seems a bit awkward to me. How about calling it > > "_NormalizeRev" and call it twice. > > Done. Still left revs as a list (instead of a rev_without and rev_with) since > this will be required for https://crbug.com/710076 anyways. > > https://codereview.chromium.org/2810813003/diff/1/tools/binary_size/diagnose_... > tools/binary_size/diagnose_apk_bloat.py:311: > parser.add_argument('--save-results', > On 2017/04/11 14:10:32, agrieve wrote: > > Rather than adding another option, I think it would be better to always save > the > > diff to a file, and then just cat the file to print it in the logs. > > Done. > > https://codereview.chromium.org/2810813003/diff/1/tools/binary_size/diagnose_... > tools/binary_size/diagnose_apk_bloat.py:315: help='Run some extra steps that > take longer to complete.') > On 2017/04/11 14:10:32, agrieve wrote: > > nit: Should say exactly which options this enables here so that devs can > reason > > about whether it's worth it for them. > > Done. > > https://codereview.chromium.org/2810813003/diff/1/tools/binary_size/diagnose_... > tools/binary_size/diagnose_apk_bloat.py:350: archive_dirs = > [os.path.join(args.archive_dir, '%d-%s' % (i + 1, rev)) > On 2017/04/11 14:10:32, agrieve wrote: > > nit: might be better as len(revs) - i, so that the larger number means latter > in > > time. > > Done. Also annoying, the script will fail if you pass a rev before your recent supersize change, is it worth copying these files temporarily so that this doesn't happen?
just nits lgtm https://codereview.chromium.org/2810813003/diff/40001/tools/binary_size/diagn... File tools/binary_size/diagnose_apk_bloat.py (right): https://codereview.chromium.org/2810813003/diff/40001/tools/binary_size/diagn... tools/binary_size/diagnose_apk_bloat.py:245: _RunCmd(['tools/binary_size/supersize', 'archive', '--output-directory', I think you need to os.path.join this with _SRC_ROOT Also - I know the parser doesn't care, but it's much more visually appealing to put size_path before the --hyphen-flags https://codereview.chromium.org/2810813003/diff/40001/tools/binary_size/diagn... tools/binary_size/diagnose_apk_bloat.py:362: if os.path.exists(output_file): nit: probably unnecessary to explicitly delete this (instead of just letting it be overwritten). Unless the thinking is that it might be harmful to have an old one lying around? In which case you should add a comment. Actually, would it be better to just error out if the file already exists? I know when I've been using this script, I "mv" the output directory after running it so that I start fresh each time.
On 2017/04/12 00:09:49, estevenson wrote: > On 2017/04/11 21:53:16, estevenson wrote: > > > https://codereview.chromium.org/2810813003/diff/1/tools/binary_size/diagnose_... > > File tools/binary_size/diagnose_apk_bloat.py (right): > > > > > https://codereview.chromium.org/2810813003/diff/1/tools/binary_size/diagnose_... > > tools/binary_size/diagnose_apk_bloat.py:95: v['units'])) > > On 2017/04/11 14:10:32, agrieve wrote: > > > nit: Let's add logic to have it fail gracefully if the same unit is not > > present > > > in both. E.g. we might change what's measured over time and still want the > > > ability to diff. > > > > Done. > > > > > https://codereview.chromium.org/2810813003/diff/1/tools/binary_size/diagnose_... > > tools/binary_size/diagnose_apk_bloat.py:232: > > map2size.main(['tools/binary_size/map2size.py', '--output-directory', > > On 2017/04/11 14:10:32, agrieve wrote: > > > Probably better to use subprocess for this since it's so RAM hungry and uses > > > atexit > > > > Done. > > > > > https://codereview.chromium.org/2810813003/diff/1/tools/binary_size/diagnose_... > > tools/binary_size/diagnose_apk_bloat.py:251: def _NormalizeRevs(*revs): > > On 2017/04/11 14:10:32, agrieve wrote: > > > nit: This interface seems a bit awkward to me. How about calling it > > > "_NormalizeRev" and call it twice. > > > > Done. Still left revs as a list (instead of a rev_without and rev_with) since > > this will be required for https://crbug.com/710076 anyways. > > > > > https://codereview.chromium.org/2810813003/diff/1/tools/binary_size/diagnose_... > > tools/binary_size/diagnose_apk_bloat.py:311: > > parser.add_argument('--save-results', > > On 2017/04/11 14:10:32, agrieve wrote: > > > Rather than adding another option, I think it would be better to always save > > the > > > diff to a file, and then just cat the file to print it in the logs. > > > > Done. > > > > > https://codereview.chromium.org/2810813003/diff/1/tools/binary_size/diagnose_... > > tools/binary_size/diagnose_apk_bloat.py:315: help='Run some extra steps that > > take longer to complete.') > > On 2017/04/11 14:10:32, agrieve wrote: > > > nit: Should say exactly which options this enables here so that devs can > > reason > > > about whether it's worth it for them. > > > > Done. > > > > > https://codereview.chromium.org/2810813003/diff/1/tools/binary_size/diagnose_... > > tools/binary_size/diagnose_apk_bloat.py:350: archive_dirs = > > [os.path.join(args.archive_dir, '%d-%s' % (i + 1, rev)) > > On 2017/04/11 14:10:32, agrieve wrote: > > > nit: might be better as len(revs) - i, so that the larger number means > latter > > in > > > time. > > > > Done. > > Also annoying, the script will fail if you pass a rev before your recent > supersize change, is it worth copying these files temporarily so that this > doesn't happen? Hmm, interesting problem. I'd hold off on it for now since this change is big already. Another bit simpler alternative would be to use git to restore it before running: git checkout WHATEVER_HEAD_WAS tools/binary_size tools/binary_size/supersize ... git checkout HEAD tools/binary_size
Will handle commits that sync to pre supersize revs in a follow-up (https://crbug.com/710717) https://codereview.chromium.org/2810813003/diff/40001/tools/binary_size/diagn... File tools/binary_size/diagnose_apk_bloat.py (right): https://codereview.chromium.org/2810813003/diff/40001/tools/binary_size/diagn... tools/binary_size/diagnose_apk_bloat.py:245: _RunCmd(['tools/binary_size/supersize', 'archive', '--output-directory', On 2017/04/12 00:20:50, agrieve wrote: > I think you need to os.path.join this with _SRC_ROOT > > Also - I know the parser doesn't care, but it's much more visually appealing to > put size_path before the --hyphen-flags Done. https://codereview.chromium.org/2810813003/diff/40001/tools/binary_size/diagn... tools/binary_size/diagnose_apk_bloat.py:362: if os.path.exists(output_file): On 2017/04/12 00:20:50, agrieve wrote: > nit: probably unnecessary to explicitly delete this (instead of just letting it > be overwritten). Unless the thinking is that it might be harmful to have an old > one lying around? In which case you should add a comment. > > Actually, would it be better to just error out if the file already exists? I > know when I've been using this script, I "mv" the output directory after running > it so that I start fresh each time. Oops, what I meant to do here was delete it if it exists and use output_file in append mode to make it easy to use the same file for each type of diff. I changed it do this and changed the name to be diff_result_revwithpatch_revwithoutpatch so that you can keep multiple diffs and actually know which one is which. Another thing we could do (as a follow up) would be to record the GN args used and avoid doing unnecessary builds. Let me know if you still think the error out is better and I'll change it.
sg, lgtm! (one more nit) https://codereview.chromium.org/2810813003/diff/60001/tools/binary_size/diagn... File tools/binary_size/diagnose_apk_bloat.py (right): https://codereview.chromium.org/2810813003/diff/60001/tools/binary_size/diagn... tools/binary_size/diagnose_apk_bloat.py:39: def PrintResults(self, output_file): nit: Maybe just rename this to "AppendResults", and pass in the already-open file so that we're not re-opening it inside of a loop.
The CQ bit was checked by estevenson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/2810813003/#ps100001 (title: "Comment AppendResults")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492008761523110,
"parent_rev": "490d8cdd48f1813962f5549217b737642ccbfa3e", "commit_rev":
"08f973975155004f8a9bc6887f6e9551cedbac1d"}
Message was sent while issue was closed.
Description was changed from ========== Add resource_sizes diffs to diagnose_apk_bloat.py. This CL adds the ability to show the differences in resource_sizes.py metrics using dianose_apk_bloat.py. It also includes a few other updates: * Simplfy output and write diff output to file * Generate and save .size files (and save .so files for Disassemble()) * Be nicer by restoring user branches after script runs BUG=710076 ========== to ========== Add resource_sizes diffs to diagnose_apk_bloat.py. This CL adds the ability to show the differences in resource_sizes.py metrics using dianose_apk_bloat.py. It also includes a few other updates: * Simplfy output and write diff output to file * Generate and save .size files (and save .so files for Disassemble()) * Be nicer by restoring user branches after script runs BUG=710076 Review-Url: https://codereview.chromium.org/2810813003 Cr-Commit-Position: refs/heads/master@{#464023} Committed: https://chromium.googlesource.com/chromium/src/+/08f973975155004f8a9bc6887f6e... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/08f973975155004f8a9bc6887f6e... |
