Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1397553002:
[Android] Apk size tool. (Closed)
|
Created:
5 years, 2 months ago by rnephew (Reviews Here) Modified:
5 years, 1 month ago Reviewers:
perezju, jbudorick, Andrew Hayden (chromium.org), rnephew (Wrong account), andrewhayden, mikecase (-- gone --) CC:
chromium-reviews, jbudorick+watch_chromium.org, klundberg+watch_chromium.org, yfriedman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Android] Apk size tool.
Gives information about files and sizes in an apk using zipfile.
BUG=546012
Committed: https://crrev.com/c37b1f6447cf974870dc27d6cbb29335200241fa
Cr-Commit-Position: refs/heads/master@{#357098}
Patch Set 1 #
Total comments: 24
Patch Set 2 : address comments #
Total comments: 26
Patch Set 3 : #
Total comments: 6
Patch Set 4 : header and get rid of patch estimates #
Total comments: 4
Patch Set 5 : #Patch Set 6 : chartjson conversion step one #
Total comments: 3
Patch Set 7 : #
Total comments: 2
Patch Set 8 : #Messages
Total messages: 31 (8 generated)
rnephew@chromium.org changed reviewers: + andrewhayden@chromium.org, jbudorick@chromium.org
On 2015/10/07 18:24:39, rnephew1 wrote: Example outputs: rnephew@rnephew0:~/chromium/clank/src/build/android$ python apksize.py $HOME/apksize/android-C4MPAR1-47.0.2516.0-arm-ChromeDev.apk CRITICAL:root:Stats for files as they exist within the apk: CRITICAL:root: xml 236436 bytes in 457 files CRITICAL:root: bin 1003661 bytes in 2 files CRITICAL:root: MF 56578 bytes in 1 files CRITICAL:root: arsc 2653256 bytes in 1 files CRITICAL:root: RSA 1160 bytes in 1 files CRITICAL:root: dat 6266928 bytes in 1 files CRITICAL:root: dex 2315231 bytes in 1 files CRITICAL:root: so 20525357 bytes in 16 files CRITICAL:root: lpak 1471625 bytes in 44 files CRITICAL:root: pak 5462684 bytes in 2 files CRITICAL:root: SF 56953 bytes in 1 files CRITICAL:root: png 1538421 bytes in 1313 files CRITICAL:root:-------------------------------------- CRITICAL:root:All Files: 41588290 bytes in 1840 files CRITICAL:root:APK Size: 41897171 CRITICAL:root:APK overhead: 308881 CRITICAL:root:-------------------------------------- CRITICAL:root:Stats for files when extracted from the apk: CRITICAL:root: xml 636572 bytes in 457 files CRITICAL:root: bin 1003661 bytes in 2 files CRITICAL:root: MF 176823 bytes in 1 files CRITICAL:root: arsc 2653256 bytes in 1 files CRITICAL:root: RSA 1720 bytes in 1 files CRITICAL:root: dat 6266928 bytes in 1 files CRITICAL:root: dex 5542480 bytes in 1 files CRITICAL:root: so 34615120 bytes in 16 files CRITICAL:root: lpak 4573376 bytes in 44 files CRITICAL:root: pak 5462684 bytes in 2 files CRITICAL:root: SF 176944 bytes in 1 files CRITICAL:root: png 1538421 bytes in 1313 files CRITICAL:root:-------------------------------------- CRITICAL:root:All Files: 62647985 bytes in 1840 files rnephew@rnephew0:~/chromium/clank/src/build/android$ python apksize.py -b $HOME/apksize/android-C4MPAR1-47.0.2516.0-arm-ChromeDev.apk *RESULT apk_size: total_files= 1840 files *RESULT apk_size: total_size_compressed= 41588290 bytes *RESULT apk_size: total_size_uncompressed= 62647985 bytes *RESULT apk_size: apk_overhead= 308881 bytes *RESULT apk_size: xml_files= 457 files *RESULT apk_size: xml_compressed_size= 236436 bytes *RESULT apk_size: xml_uncompressed_size= 636572 bytes *RESULT apk_size: bin_files= 2 files *RESULT apk_size: bin_compressed_size= 1003661 bytes *RESULT apk_size: bin_uncompressed_size= 1003661 bytes *RESULT apk_size: MF_files= 1 files *RESULT apk_size: MF_compressed_size= 56578 bytes *RESULT apk_size: MF_uncompressed_size= 176823 bytes *RESULT apk_size: arsc_files= 1 files *RESULT apk_size: arsc_compressed_size= 2653256 bytes *RESULT apk_size: arsc_uncompressed_size= 2653256 bytes *RESULT apk_size: RSA_files= 1 files *RESULT apk_size: RSA_compressed_size= 1160 bytes *RESULT apk_size: RSA_uncompressed_size= 1720 bytes *RESULT apk_size: dat_files= 1 files *RESULT apk_size: dat_compressed_size= 6266928 bytes *RESULT apk_size: dat_uncompressed_size= 6266928 bytes *RESULT apk_size: dex_files= 1 files *RESULT apk_size: dex_compressed_size= 2315231 bytes *RESULT apk_size: dex_uncompressed_size= 5542480 bytes *RESULT apk_size: so_files= 16 files *RESULT apk_size: so_compressed_size= 20525357 bytes *RESULT apk_size: so_uncompressed_size= 34615120 bytes *RESULT apk_size: lpak_files= 44 files *RESULT apk_size: lpak_compressed_size= 1471625 bytes *RESULT apk_size: lpak_uncompressed_size= 4573376 bytes *RESULT apk_size: pak_files= 2 files *RESULT apk_size: pak_compressed_size= 5462684 bytes *RESULT apk_size: pak_uncompressed_size= 5462684 bytes *RESULT apk_size: SF_files= 1 files *RESULT apk_size: SF_compressed_size= 56953 bytes *RESULT apk_size: SF_uncompressed_size= 176944 bytes *RESULT apk_size: png_files= 1313 files *RESULT apk_size: png_compressed_size= 1538421 bytes *RESULT apk_size: png_uncompressed_size= 1538421 bytes rnephew@rnephew0:~/chromium/clank/src/build/android$ python apksize.py -c $HOME/apksize/android-C4MPAR1-47.0.2516.0-arm-ChromeDev.apk $HOME/apksize/android-C4MPAR1-47.0.2517.0-arm-ChromeDev.apk CRITICAL:root: APK_size_reduction -18775 bytes CRITICAL:root: ARM32_Legacy_install_or_upgrade_reduction -37465 bytes CRITICAL:root: ARM32_Legacy_system_image_reduction -12857 bytes CRITICAL:root: ARM32_Legacy_patch_size_reduction -12857 bytes CRITICAL:root: ARM32_Modern_ARM64_install_or_upgrade_reduction -24608 bytes CRITICAL:root: ARM32_Modern_ARM64_system_image_reduction -24608 bytes CRITICAL:root: ARM32_Modern_ARM64_patch_size_reduction -12857 bytes rnephew@rnephew0:~/chromium/clank/src/build/android$ python apksize.py -b -c $HOME/apksize/android-C4MPAR1-47.0.2516.0-arm-ChromeDev.apk $HOME/apksize/android-C4MPAR1-47.0.2517.0-arm-ChromeDev.apk *RESULT apk_size_compare: APK_size_reduction= -18775 bytes *RESULT apk_size_compare: ARM32_Legacy_install_or_upgrade_reduction= -37465 bytes *RESULT apk_size_compare: ARM32_Legacy_system_image_reduction= -12857 bytes *RESULT apk_size_compare: ARM32_Legacy_patch_size_reduction= -12857 bytes *RESULT apk_size_compare: ARM32_Modern_ARM64_install_or_upgrade_reduction= -24608 bytes *RESULT apk_size_compare: ARM32_Modern_ARM64_system_image_reduction= -24608 bytes *RESULT apk_size_compare: ARM32_Modern_ARM64_patch_size_reduction= -12857 bytes
rnephew@chromium.org changed reviewers: + mikecase@chromium.org
https://codereview.chromium.org/1397553002/diff/1/build/android/apksize.py File build/android/apksize.py (right): https://codereview.chromium.org/1397553002/diff/1/build/android/apksize.py#ne... build/android/apksize.py:28: logging.info('APK: %s', path) nit: Slightly strangely placed logging statement. Maybe just move to start of function or move to be after all three "if" statements. Would help organize things a little bit. https://codereview.chromium.org/1397553002/diff/1/build/android/apksize.py#ne... build/android/apksize.py:51: self._processed_files[file_ext]['compressed'] += f.compress_size Maybe add units into the key name, like 'compressed_bytes'. https://codereview.chromium.org/1397553002/diff/1/build/android/apksize.py#ne... build/android/apksize.py:65: def Compare(self, old_apk): maybe rename old_apk to other_apk. Doesn't appear to have to be an older apk. https://codereview.chromium.org/1397553002/diff/1/build/android/apksize.py#ne... build/android/apksize.py:81: reduction_uncompressed_files = old_lib_uncompressed - new_lib_uncompressed nit: ^ for the above 10 or so lines, maybe just fill in a dict as you go along so you don't have to make a ton of new variables. https://codereview.chromium.org/1397553002/diff/1/build/android/apksize.py#ne... build/android/apksize.py:86: ret_dict = collections.OrderedDict([ nit: Rename ret_dict to something more descriptive like comparison_results https://codereview.chromium.org/1397553002/diff/1/build/android/apksize.py#ne... build/android/apksize.py:88: ('ARM32_Legacy_install_or_upgrade_reduction', reduction_install_legacy), Does this size comparison only work for ARM32? https://codereview.chromium.org/1397553002/diff/1/build/android/apksize.py#ne... build/android/apksize.py:175: apk: ApkSizeInfo object Fix this Args doc. https://codereview.chromium.org/1397553002/diff/1/build/android/apksize.py#ne... build/android/apksize.py:180: def dashboard_readable_compare(results): nit: I would rename these functions to have "output" or "print" in there names. like output_compare_results output_dashboard_compare_results or print_compare_results print_dashboard_compare_results https://codereview.chromium.org/1397553002/diff/1/build/android/apksize.py#ne... build/android/apksize.py:183: Args: Fix this Args doc. https://codereview.chromium.org/1397553002/diff/1/build/android/apksize.py#ne... build/android/apksize.py:192: parser.add_argument("file_path") nit: single quotes probably https://codereview.chromium.org/1397553002/diff/1/build/android/apksize.py#ne... build/android/apksize.py:193: parser.add_argument('-b', '--bot-mode', action='store_true', maybe rename to --perf-dashboard-formatting or --perf-dashboard-output https://codereview.chromium.org/1397553002/diff/1/build/android/apksize.py#ne... build/android/apksize.py:207: human_readable_size_info(apk) nit: I think this would look better as... if args.compare: if args.bot_mode: dashboard_readable_compare(apk.Compare(ApkSizeInfo(args.compare))) else: human_readable_compare(apk.Compare(ApkSizeInfo(args.compare))) else: if args.bot_mode: dashboard_readable_size_info(apk) else: human_readable_size_info(apk) because its just more symmetric.
https://codereview.chromium.org/1397553002/diff/1/build/android/apksize.py File build/android/apksize.py (right): https://codereview.chromium.org/1397553002/diff/1/build/android/apksize.py#ne... build/android/apksize.py:28: logging.info('APK: %s', path) On 2015/10/07 19:33:01, mikecase wrote: > nit: Slightly strangely placed logging statement. Maybe just move to start of > function or move to be after all three "if" statements. Would help organize > things a little bit. Done. https://codereview.chromium.org/1397553002/diff/1/build/android/apksize.py#ne... build/android/apksize.py:51: self._processed_files[file_ext]['compressed'] += f.compress_size On 2015/10/07 19:33:01, mikecase wrote: > Maybe add units into the key name, like 'compressed_bytes'. Done. https://codereview.chromium.org/1397553002/diff/1/build/android/apksize.py#ne... build/android/apksize.py:65: def Compare(self, old_apk): On 2015/10/07 19:33:01, mikecase wrote: > maybe rename old_apk to other_apk. Doesn't appear to have to be an older apk. Done. https://codereview.chromium.org/1397553002/diff/1/build/android/apksize.py#ne... build/android/apksize.py:81: reduction_uncompressed_files = old_lib_uncompressed - new_lib_uncompressed On 2015/10/07 19:33:01, mikecase wrote: > nit: ^ for the above 10 or so lines, maybe just fill in a dict as you go along > so you don't have to make a ton of new variables. I did that with some of them, but not all; it made readability bad. https://codereview.chromium.org/1397553002/diff/1/build/android/apksize.py#ne... build/android/apksize.py:86: ret_dict = collections.OrderedDict([ On 2015/10/07 19:33:01, mikecase wrote: > nit: Rename ret_dict to something more descriptive like comparison_results Just got rid of it, no need to make a variable when all its going to do is return it next line. https://codereview.chromium.org/1397553002/diff/1/build/android/apksize.py#ne... build/android/apksize.py:88: ('ARM32_Legacy_install_or_upgrade_reduction', reduction_install_legacy), On 2015/10/07 19:33:01, mikecase wrote: > Does this size comparison only work for ARM32? I picked the names based off the document here: https://docs.google.com/document/d/1b-99ysCyQxhaTCFdBjRhrtft7-NXXAwlwG0K951e9... I added Andrew as a reviewer to this CL so maybe he can comment more on it. https://codereview.chromium.org/1397553002/diff/1/build/android/apksize.py#ne... build/android/apksize.py:175: apk: ApkSizeInfo object On 2015/10/07 19:33:01, mikecase wrote: > Fix this Args doc. Done. https://codereview.chromium.org/1397553002/diff/1/build/android/apksize.py#ne... build/android/apksize.py:180: def dashboard_readable_compare(results): On 2015/10/07 19:33:01, mikecase wrote: > nit: I would rename these functions to have "output" or "print" in there names. > > like > > output_compare_results > output_dashboard_compare_results > > or > > print_compare_results > print_dashboard_compare_results Done. https://codereview.chromium.org/1397553002/diff/1/build/android/apksize.py#ne... build/android/apksize.py:183: Args: On 2015/10/07 19:33:01, mikecase wrote: > Fix this Args doc. Done. https://codereview.chromium.org/1397553002/diff/1/build/android/apksize.py#ne... build/android/apksize.py:192: parser.add_argument("file_path") On 2015/10/07 19:33:01, mikecase wrote: > nit: single quotes probably Done. https://codereview.chromium.org/1397553002/diff/1/build/android/apksize.py#ne... build/android/apksize.py:193: parser.add_argument('-b', '--bot-mode', action='store_true', On 2015/10/07 19:33:01, mikecase wrote: > maybe rename to --perf-dashboard-formatting or --perf-dashboard-output Done. https://codereview.chromium.org/1397553002/diff/1/build/android/apksize.py#ne... build/android/apksize.py:207: human_readable_size_info(apk) On 2015/10/07 19:33:01, mikecase wrote: > nit: I think this would look better as... > > if args.compare: > if args.bot_mode: > dashboard_readable_compare(apk.Compare(ApkSizeInfo(args.compare))) > else: > human_readable_compare(apk.Compare(ApkSizeInfo(args.compare))) > else: > if args.bot_mode: > dashboard_readable_size_info(apk) > else: > human_readable_size_info(apk) > > because its just more symmetric. Done.
https://codereview.chromium.org/1397553002/diff/20001/build/android/apksize.py File build/android/apksize.py (right): https://codereview.chromium.org/1397553002/diff/20001/build/android/apksize.p... build/android/apksize.py:19: """ ApkSizeInfo constructor. nit: no space after """ https://codereview.chromium.org/1397553002/diff/20001/build/android/apksize.p... build/android/apksize.py:38: def ProcessFiles(self): What's the reasoning behind doing this lazily rather than at construction time? https://codereview.chromium.org/1397553002/diff/20001/build/android/apksize.p... build/android/apksize.py:39: """ Uses zipinfo to process apk file information.""" nit: same https://codereview.chromium.org/1397553002/diff/20001/build/android/apksize.p... build/android/apksize.py:42: self._processed_files = {} You could do INITIAL_FILE_EXTENSION_INFO = { 'number': 0, 'compressed_bytes': 0, 'uncompressed_bytes': 0 } self._processed_files = collections.defaultdict(lambda: dict(FILE_EXTENSION_INFO)) and ditch the else block below entirely https://codereview.chromium.org/1397553002/diff/20001/build/android/apksize.p... build/android/apksize.py:54: nit: remove blank line or move it before the else https://codereview.chromium.org/1397553002/diff/20001/build/android/apksize.p... build/android/apksize.py:69: other_pak: Apk to compare size against. other_pak -> other_apk? https://codereview.chromium.org/1397553002/diff/20001/build/android/apksize.p... build/android/apksize.py:70: """ Returns: https://codereview.chromium.org/1397553002/diff/20001/build/android/apksize.p... build/android/apksize.py:73: apk_one = other_pak.ProcessFiles() nit: "one" and "two" should be named to reflect which one comes from self and which one comes from other_(apk|pak) https://codereview.chromium.org/1397553002/diff/20001/build/android/apksize.p... build/android/apksize.py:80: # seperate, a new method to compare will be required eventually. nit: separate https://codereview.chromium.org/1397553002/diff/20001/build/android/apksize.p... build/android/apksize.py:175: apk: ApkSizeInfo object "apk" isn't an ApkSizeInfo object, and it probably shouldn't be named "apk" https://codereview.chromium.org/1397553002/diff/20001/build/android/apksize.p... build/android/apksize.py:184: apk: ApkSizeInfo object same https://codereview.chromium.org/1397553002/diff/20001/build/android/apksize.p... build/android/apksize.py:201: print_dashboard_readable_compare(apk.Compare(ApkSizeInfo(args.compare))) The Compare call is the same in both cases, so extract it outside of the perf_dashboard_output switch.
https://codereview.chromium.org/1397553002/diff/20001/build/android/apksize.py File build/android/apksize.py (right): https://codereview.chromium.org/1397553002/diff/20001/build/android/apksize.p... build/android/apksize.py:19: """ ApkSizeInfo constructor. On 2015/10/08 14:23:29, jbudorick wrote: > nit: no space after """ Done. https://codereview.chromium.org/1397553002/diff/20001/build/android/apksize.p... build/android/apksize.py:38: def ProcessFiles(self): On 2015/10/08 14:23:29, jbudorick wrote: > What's the reasoning behind doing this lazily rather than at construction time? Made this a private function called by the constructor and added a property to retrieve it. https://codereview.chromium.org/1397553002/diff/20001/build/android/apksize.p... build/android/apksize.py:39: """ Uses zipinfo to process apk file information.""" On 2015/10/08 14:23:29, jbudorick wrote: > nit: same Done. https://codereview.chromium.org/1397553002/diff/20001/build/android/apksize.p... build/android/apksize.py:42: self._processed_files = {} On 2015/10/08 14:23:29, jbudorick wrote: > You could do > > INITIAL_FILE_EXTENSION_INFO = { > 'number': 0, > 'compressed_bytes': 0, > 'uncompressed_bytes': 0 > } > self._processed_files = collections.defaultdict(lambda: > dict(FILE_EXTENSION_INFO)) > > and ditch the else block below entirely Done. https://codereview.chromium.org/1397553002/diff/20001/build/android/apksize.p... build/android/apksize.py:54: On 2015/10/08 14:23:29, jbudorick wrote: > nit: remove blank line or move it before the else Done. https://codereview.chromium.org/1397553002/diff/20001/build/android/apksize.p... build/android/apksize.py:69: other_pak: Apk to compare size against. On 2015/10/08 14:23:29, jbudorick wrote: > other_pak -> other_apk? Done. https://codereview.chromium.org/1397553002/diff/20001/build/android/apksize.p... build/android/apksize.py:70: """ On 2015/10/08 14:23:29, jbudorick wrote: > Returns: Done. https://codereview.chromium.org/1397553002/diff/20001/build/android/apksize.p... build/android/apksize.py:73: apk_one = other_pak.ProcessFiles() On 2015/10/08 14:23:29, jbudorick wrote: > nit: "one" and "two" should be named to reflect which one comes from self and > which one comes from other_(apk|pak) Done. https://codereview.chromium.org/1397553002/diff/20001/build/android/apksize.p... build/android/apksize.py:80: # seperate, a new method to compare will be required eventually. On 2015/10/08 14:23:29, jbudorick wrote: > nit: separate Done. https://codereview.chromium.org/1397553002/diff/20001/build/android/apksize.p... build/android/apksize.py:175: apk: ApkSizeInfo object On 2015/10/08 14:23:29, jbudorick wrote: > "apk" isn't an ApkSizeInfo object, and it probably shouldn't be named "apk" Done. https://codereview.chromium.org/1397553002/diff/20001/build/android/apksize.p... build/android/apksize.py:184: apk: ApkSizeInfo object On 2015/10/08 14:23:29, jbudorick wrote: > same Done. https://codereview.chromium.org/1397553002/diff/20001/build/android/apksize.p... build/android/apksize.py:201: print_dashboard_readable_compare(apk.Compare(ApkSizeInfo(args.compare))) On 2015/10/08 14:23:29, jbudorick wrote: > The Compare call is the same in both cases, so extract it outside of the > perf_dashboard_output switch. Done.
Friendly ping.
non-owner lgtm w/ 1 comment https://codereview.chromium.org/1397553002/diff/40001/build/android/apksize.py File build/android/apksize.py (right): https://codereview.chromium.org/1397553002/diff/40001/build/android/apksize.p... build/android/apksize.py:1: import argparse I think you need to add the Chromium copyright notice at the top here. # Copyright 2015 The Chromium Authors. All rights reserved. # Use of this source code is governed by a BSD-style license that can be # found in the LICENSE file.
andrewhayden@google.com changed reviewers: + andrewhayden@google.com
I agree with mikecase@'s comment, and I think we should strike the patch size reduction stuff completely (see comment). Also a big thank you for taking this on. https://codereview.chromium.org/1397553002/diff/40001/build/android/apksize.py File build/android/apksize.py (right): https://codereview.chromium.org/1397553002/diff/40001/build/android/apksize.p... build/android/apksize.py:89: ('ARM32_Legacy_patch_size_reduction', The details of Google's application update technology - or that of any app store that is providing Chrome updates to users - is opaque here. Although I used to think this would be a great number to have, I've come to think otherwise. You could reduce the size of the binary but use a different set of compiler flags that resulted in a wildly patching-hostile diff that *increased* the patch size dramatically. Given this, I think we should drop patch size reduction from here. The only accurate way to get patch size numbers is to run them through a real patch generation pipeline. It's not feasible to do that here, at least not right now. https://codereview.chromium.org/1397553002/diff/40001/build/android/apksize.p... build/android/apksize.py:95: ('ARM32_Modern_ARM64_patch_size_reduction', Same here, not feasible to get this number in any useful way right now.
rnephew@google.com changed reviewers: + rnephew@google.com
https://codereview.chromium.org/1397553002/diff/20001/build/android/apksize.py File build/android/apksize.py (right): https://codereview.chromium.org/1397553002/diff/20001/build/android/apksize.p... build/android/apksize.py:38: def ProcessFiles(self): On 2015/10/08 14:23:29, jbudorick wrote: > What's the reasoning behind doing this lazily rather than at construction time? Made it a private function called by the constructor and made a property to get the results. https://codereview.chromium.org/1397553002/diff/20001/build/android/apksize.p... build/android/apksize.py:39: """ Uses zipinfo to process apk file information.""" On 2015/10/08 14:23:29, jbudorick wrote: > nit: same Done. https://codereview.chromium.org/1397553002/diff/40001/build/android/apksize.py File build/android/apksize.py (right): https://codereview.chromium.org/1397553002/diff/40001/build/android/apksize.p... build/android/apksize.py:1: import argparse On 2015/10/14 20:16:44, mikecase wrote: > I think you need to add the Chromium copyright notice at the top here. > > # Copyright 2015 The Chromium Authors. All rights reserved. > # Use of this source code is governed by a BSD-style license that can be > # found in the LICENSE file. Done. https://codereview.chromium.org/1397553002/diff/40001/build/android/apksize.p... build/android/apksize.py:89: ('ARM32_Legacy_patch_size_reduction', On 2015/10/14 23:38:58, andrewhayden wrote: > The details of Google's application update technology - or that of any app store > that is providing Chrome updates to users - is opaque here. Although I used to > think this would be a great number to have, I've come to think otherwise. You > could reduce the size of the binary but use a different set of compiler flags > that resulted in a wildly patching-hostile diff that *increased* the patch size > dramatically. Given this, I think we should drop patch size reduction from here. > The only accurate way to get patch size numbers is to run them through a real > patch generation pipeline. It's not feasible to do that here, at least not right > now. Done. https://codereview.chromium.org/1397553002/diff/40001/build/android/apksize.p... build/android/apksize.py:95: ('ARM32_Modern_ARM64_patch_size_reduction', On 2015/10/14 23:38:58, andrewhayden wrote: > Same here, not feasible to get this number in any useful way right now. Done.
Description was changed from ========== [Android] Apk size tool. Gives information about files and sizes in an apk using zipfile. BUG= ========== to ========== [Android] Apk size tool. Gives information about files and sizes in an apk using zipfile. BUG=546012 ==========
gave it another look through. Still lgtm (w/ 1 question). https://codereview.chromium.org/1397553002/diff/60001/build/android/apksize.py File build/android/apksize.py (right): https://codereview.chromium.org/1397553002/diff/60001/build/android/apksize.p... build/android/apksize.py:91: (other_lib_uncompressed - this_lib_uncompressed)), Is this correct? Why is this measuring (other_compressed + other_uncompressed) - (this_compressed + this_uncompressed). Are both compressed and uncompressed libs stored on the device or something? I'm not too familiar with this.
https://codereview.chromium.org/1397553002/diff/60001/build/android/apksize.py File build/android/apksize.py (right): https://codereview.chromium.org/1397553002/diff/60001/build/android/apksize.p... build/android/apksize.py:91: (other_lib_uncompressed - this_lib_uncompressed)), On 2015/10/22 01:20:53, mikecase wrote: > Is this correct? > > Why is this measuring (other_compressed + other_uncompressed) - (this_compressed > + this_uncompressed). > > Are both compressed and uncompressed libs stored on the device or something? I'm > not too familiar with this. For 32b legacy apks, yes; both the compressed and uncompressed versions are there. For more information on apk sizes you can look at: https://docs.google.com/document/d/1b-99ysCyQxhaTCFdBjRhrtft7-NXXAwlwG0K951e9...
lgtm w/ nit https://codereview.chromium.org/1397553002/diff/60001/build/android/apksize.py File build/android/apksize.py (right): https://codereview.chromium.org/1397553002/diff/60001/build/android/apksize.p... build/android/apksize.py:70: other_apk: Apk to compare size against. nit: make it clear that this is another instance of ApkSizeInfo
https://codereview.chromium.org/1397553002/diff/60001/build/android/apksize.py File build/android/apksize.py (right): https://codereview.chromium.org/1397553002/diff/60001/build/android/apksize.p... build/android/apksize.py:70: other_apk: Apk to compare size against. On 2015/10/27 23:40:03, jbudorick wrote: > nit: make it clear that this is another instance of ApkSizeInfo Done.
perezju@chromium.org changed reviewers: + perezju@chromium.org
https://codereview.chromium.org/1397553002/diff/100001/build/android/apksize.py File build/android/apksize.py (right): https://codereview.chromium.org/1397553002/diff/100001/build/android/apksize.... build/android/apksize.py:208: parser.add_argument('-d', '--perf-dashboard-output', action='store_true', You've got nearly everything ready. The last bit to get this working for uploading dashboard results is that your script should accept an --output-dir argument (this will be supplied by the perf-test runner) where you should serialize the chatjson dict to a file named 'results-chart.json'. So I would replace your '-d' option with the '--output-dir' one, and explain in the help message that when the option is used, it will write a chartjson there instead of (or in addition to?) printing the human readable results to stdout. The perf test runner will also try to send you a --device option, which doesn't make that much sense here. Not sure what would be the best option: 1. Just add a dummy --device option to your script, whose value is simply ignored. 2. Make some changes on the perf test runner so that you can configure yours as a "non-device" test. 3. Do not run the test with the perf test runner, but craft instead a custom infra recipe to run your script and upload the chartjson file. John, any thoughts on this?
https://codereview.chromium.org/1397553002/diff/100001/build/android/apksize.py File build/android/apksize.py (right): https://codereview.chromium.org/1397553002/diff/100001/build/android/apksize.... build/android/apksize.py:208: parser.add_argument('-d', '--perf-dashboard-output', action='store_true', On 2015/10/29 09:56:23, perezju wrote: > You've got nearly everything ready. The last bit to get this working for > uploading dashboard results is that your script should accept an --output-dir > argument (this will be supplied by the perf-test runner) where you should > serialize the chatjson dict to a file named 'results-chart.json'. So I would > replace your '-d' option with the '--output-dir' one, and explain in the help > message that when the option is used, it will write a chartjson there instead of > (or in addition to?) printing the human readable results to stdout. > > The perf test runner will also try to send you a --device option, which doesn't > make that much sense here. Not sure what would be the best option: > 1. Just add a dummy --device option to your script, whose value is simply > ignored. > 2. Make some changes on the perf test runner so that you can configure yours as > a "non-device" test. > 3. Do not run the test with the perf test runner, but craft instead a custom > infra recipe to run your script and upload the chartjson file. > > John, any thoughts on this? I would lean toward the dummy --device argument.
https://codereview.chromium.org/1397553002/diff/100001/build/android/apksize.py File build/android/apksize.py (right): https://codereview.chromium.org/1397553002/diff/100001/build/android/apksize.... build/android/apksize.py:208: parser.add_argument('-d', '--perf-dashboard-output', action='store_true', On 2015/10/29 09:56:23, perezju wrote: > You've got nearly everything ready. The last bit to get this working for > uploading dashboard results is that your script should accept an --output-dir > argument (this will be supplied by the perf-test runner) where you should > serialize the chatjson dict to a file named 'results-chart.json'. So I would > replace your '-d' option with the '--output-dir' one, and explain in the help > message that when the option is used, it will write a chartjson there instead of > (or in addition to?) printing the human readable results to stdout. > > The perf test runner will also try to send you a --device option, which doesn't > make that much sense here. Not sure what would be the best option: > 1. Just add a dummy --device option to your script, whose value is simply > ignored. > 2. Make some changes on the perf test runner so that you can configure yours as > a "non-device" test. > 3. Do not run the test with the perf test runner, but craft instead a custom > infra recipe to run your script and upload the chartjson file. > > John, any thoughts on this? Done.
lgtm w/nit https://codereview.chromium.org/1397553002/diff/120001/build/android/apksize.py File build/android/apksize.py (right): https://codereview.chromium.org/1397553002/diff/120001/build/android/apksize.... build/android/apksize.py:210: parser.add_argument('-c', '--compare', help=('APK to compare against.')) nit: remove the parenthesis around the help strings?
https://codereview.chromium.org/1397553002/diff/120001/build/android/apksize.py File build/android/apksize.py (right): https://codereview.chromium.org/1397553002/diff/120001/build/android/apksize.... build/android/apksize.py:210: parser.add_argument('-c', '--compare', help=('APK to compare against.')) On 2015/10/30 09:17:10, perezju wrote: > nit: remove the parenthesis around the help strings? Done.
The CQ bit was checked by rnephew@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org, mikecase@chromium.org, perezju@chromium.org Link to the patchset: https://codereview.chromium.org/1397553002/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1397553002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1397553002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/c37b1f6447cf974870dc27d6cbb29335200241fa Cr-Commit-Position: refs/heads/master@{#357098} |
