Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2930313002:
resource_sizes.py: Better pakfile and dex normalization. (Closed)
|
Created:
3 years, 6 months ago by estevenson Modified:
3 years, 6 months ago Reviewers:
agrieve CC:
chromium-reviews, mikecase+watch_chromium.org, jbudorick+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionresource_sizes.py: Better pakfile and dex normalization.
This CL adds special casing for Monochrome and improves dex size
tracking:
* Detect whether or not the native lib is extracted for install size
* Add a decompression factor for dex files
* Account for stored-locales in Monochrome in pakfile normalization
BUG=726420
Review-Url: https://codereview.chromium.org/2930313002
Cr-Commit-Position: refs/heads/master@{#480534}
Committed: https://chromium.googlesource.com/chromium/src/+/3311c02bc4c282ef05b2dc24fbe6b8f4745f5f35
Patch Set 1 #
Total comments: 21
Patch Set 2 : Review comments #
Total comments: 4
Patch Set 3 : Fix extracted_multiplier for ZipInfo calls. Update dex multiplier comment. #Patch Set 4 : comment typo #Messages
Total messages: 17 (10 generated)
The CQ bit was checked by estevenson@chromium.org to run a CQ dry run
estevenson@chromium.org changed reviewers: + agrieve@chromium.org
ptal Andrew. With this patch, for Felix's pakfile change diagnose_bloat reports a decrease in APK size decrease of 2.6 MB and the normalized_apk_size decrease of 1.8MB. It makes sense to me that the normalized size decrease won't be as high (since we were over-compensating previously by multiplying all 53 locale paks by the english pak size), but 800 kb seems pretty crazy.. However, this change brings the normalized_apk_size closer to the actual APK size (by about 700 kb) so I'm inclined to say the change is.. more correct. https://codereview.chromium.org/2930313002/diff/1/build/android/resource_size... File build/android/resource_sizes.py (right): https://codereview.chromium.org/2930313002/diff/1/build/android/resource_size... build/android/resource_sizes.py:190: dex_multiplier = 3 Should this be 2?
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== resource_sizes.py: Better pakfile and dex normalization. This CL adds special casing for Monochrome and improves dex size tracking: * Detect whether or not the native lib is extracted for install size * Add a decompression factor for dex files * Account for stored-locales in Monochrome in pakfile normalization BUG=726420 ========== to ========== resource_sizes.py: Better pakfile and dex normalization. This CL adds special casing for Monochrome and improves dex size tracking: * Detect whether or not the native lib is extracted for install size * Add a decompression factor for dex files * Account for stored-locales in Monochrome in pakfile normalization BUG=726420 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2930313002/diff/1/build/android/resource_size... File build/android/resource_sizes.py (left): https://codereview.chromium.org/2930313002/diff/1/build/android/resource_size... build/android/resource_sizes.py:445: # 1.17 found by looking at Chrome.apk and seeing how much smaller en-US.pak can we keep this comment? https://codereview.chromium.org/2930313002/diff/1/build/android/resource_size... File build/android/resource_sizes.py (right): https://codereview.chromium.org/2930313002/diff/1/build/android/resource_size... build/android/resource_sizes.py:115: _RE_LANGUAGE_PAK = re.compile( nit: RE_LANGUAGE_PAK -> RE_COMPRESSED_LANGUAGE_PAK https://codereview.chromium.org/2930313002/diff/1/build/android/resource_size... build/android/resource_sizes.py:118: r'\.lpak$|^assets/stored-locales/.*(?!resources|percent)\.pak$') nit: can remove .lpak from this list. .lpak is historical, but was only ever used to allow compression. https://codereview.chromium.org/2930313002/diff/1/build/android/resource_size... build/android/resource_sizes.py:177: 'aapt', 'd', 'xmltree', apk_path, 'AndroidManifest.xml']) I don't think we can assume aapt will be in the PATH. I think to get this, the best way is to read build_vars.txt. https://codereview.chromium.org/2930313002/diff/1/build/android/resource_size... build/android/resource_sizes.py:183: output = cmd_helper.GetCmdOutput(['aapt', 'd', 'badging', apk_path]) nit: no need to run both badging and xmltree. The minSdkVersion is in the output of xmltree already: E: uses-sdk (line=19) A: android:minSdkVersion(0x0101020c)=(type 0x10)0x18 A: android:targetSdkVersion(0x01010270)=(type 0x10)0x1a https://codereview.chromium.org/2930313002/diff/1/build/android/resource_size... build/android/resource_sizes.py:189: elif 'monochrome' in apk_path.lower(): # Extracted for WebView and Chrome. Worth a comment saying why this is the case. https://codereview.chromium.org/2930313002/diff/1/build/android/resource_size... build/android/resource_sizes.py:326: def AddZipInfo(self, zip_info, extracted_multiplier=None): nit: maybe make the default 0, so that you don't have to add the "if multiplier" below? https://codereview.chromium.org/2930313002/diff/1/build/android/resource_size... build/android/resource_sizes.py:400: native_code.AddZipInfo(member, should_extract_lib) nit: change to int(should_extract_lib) https://codereview.chromium.org/2930313002/diff/1/build/android/resource_size... build/android/resource_sizes.py:406: translations.AddZipInfo(member, 'en_' in filename or 'en-' in filename) nit: add int() to 2nd param https://codereview.chromium.org/2930313002/diff/1/build/android/resource_size... build/android/resource_sizes.py:409: member, 'en_' in filename or 'en-' in filename) I think 2nd param here should always be 0 (never extracted)
https://codereview.chromium.org/2930313002/diff/1/build/android/resource_size... File build/android/resource_sizes.py (left): https://codereview.chromium.org/2930313002/diff/1/build/android/resource_size... build/android/resource_sizes.py:445: # 1.17 found by looking at Chrome.apk and seeing how much smaller en-US.pak On 2017/06/14 14:11:13, agrieve wrote: > can we keep this comment? Done. https://codereview.chromium.org/2930313002/diff/1/build/android/resource_size... File build/android/resource_sizes.py (right): https://codereview.chromium.org/2930313002/diff/1/build/android/resource_size... build/android/resource_sizes.py:115: _RE_LANGUAGE_PAK = re.compile( On 2017/06/14 14:11:12, agrieve wrote: > nit: RE_LANGUAGE_PAK -> RE_COMPRESSED_LANGUAGE_PAK Done. https://codereview.chromium.org/2930313002/diff/1/build/android/resource_size... build/android/resource_sizes.py:118: r'\.lpak$|^assets/stored-locales/.*(?!resources|percent)\.pak$') On 2017/06/14 14:11:13, agrieve wrote: > nit: can remove .lpak from this list. .lpak is historical, but was only ever > used to allow compression. Done. https://codereview.chromium.org/2930313002/diff/1/build/android/resource_size... build/android/resource_sizes.py:177: 'aapt', 'd', 'xmltree', apk_path, 'AndroidManifest.xml']) On 2017/06/14 14:11:12, agrieve wrote: > I don't think we can assume aapt will be in the PATH. I think to get this, the > best way is to read build_vars.txt. Forgot that we already have AAPT_PATH via devil.android.sdk.build_tools. Done. https://codereview.chromium.org/2930313002/diff/1/build/android/resource_size... build/android/resource_sizes.py:183: output = cmd_helper.GetCmdOutput(['aapt', 'd', 'badging', apk_path]) On 2017/06/14 14:11:12, agrieve wrote: > nit: no need to run both badging and xmltree. The minSdkVersion is in the output > of xmltree already: > > E: uses-sdk (line=19) > A: android:minSdkVersion(0x0101020c)=(type 0x10)0x18 > A: android:targetSdkVersion(0x01010270)=(type 0x10)0x1a Ahh missed that. Done! https://codereview.chromium.org/2930313002/diff/1/build/android/resource_size... build/android/resource_sizes.py:189: elif 'monochrome' in apk_path.lower(): # Extracted for WebView and Chrome. On 2017/06/14 14:11:13, agrieve wrote: > Worth a comment saying why this is the case. Done. Can you confirm that it should be a multiplier of 2 here not 3? https://codereview.chromium.org/2930313002/diff/1/build/android/resource_size... build/android/resource_sizes.py:326: def AddZipInfo(self, zip_info, extracted_multiplier=None): On 2017/06/14 14:11:13, agrieve wrote: > nit: maybe make the default 0, so that you don't have to add the "if multiplier" > below? Done. https://codereview.chromium.org/2930313002/diff/1/build/android/resource_size... build/android/resource_sizes.py:400: native_code.AddZipInfo(member, should_extract_lib) On 2017/06/14 14:11:12, agrieve wrote: > nit: change to int(should_extract_lib) Done. https://codereview.chromium.org/2930313002/diff/1/build/android/resource_size... build/android/resource_sizes.py:406: translations.AddZipInfo(member, 'en_' in filename or 'en-' in filename) On 2017/06/14 14:11:13, agrieve wrote: > nit: add int() to 2nd param Done. https://codereview.chromium.org/2930313002/diff/1/build/android/resource_size... build/android/resource_sizes.py:409: member, 'en_' in filename or 'en-' in filename) On 2017/06/14 14:11:13, agrieve wrote: > I think 2nd param here should always be 0 (never extracted) Definitely. Done.
sorry for forgetting about this today. lgtm /w a couple more comments. https://codereview.chromium.org/2930313002/diff/20001/build/android/resource_... File build/android/resource_sizes.py (right): https://codereview.chromium.org/2930313002/diff/20001/build/android/resource_... build/android/resource_sizes.py:189: # Monochrome consists of WebView and Chrome packaged into a single APK, so It's not that both apps extract the dex, it's that on Android N+, there is profile-guided dexopt, which ends up being 1x multiplied, but as another app loads your dex, it switches to non-profile-guided dexopt (3x multiplier). Because webview loads the dex, and webview lives in random other apps, Monochrome falls into the bucket of having full dexopt, making it the same as pre-N (so, 3x). Would be great if you added this explanation as a comment :P (perhaps cleaned up a bit) https://codereview.chromium.org/2930313002/diff/20001/build/android/resource_... build/android/resource_sizes.py:409: member, 'en_' in filename or 'en-' in filename) add int()
https://codereview.chromium.org/2930313002/diff/20001/build/android/resource_... File build/android/resource_sizes.py (right): https://codereview.chromium.org/2930313002/diff/20001/build/android/resource_... build/android/resource_sizes.py:189: # Monochrome consists of WebView and Chrome packaged into a single APK, so On 2017/06/16 01:37:16, agrieve wrote: > It's not that both apps extract the dex, it's that on Android N+, there is > profile-guided dexopt, which ends up being 1x multiplied, but as another app > loads your dex, it switches to non-profile-guided dexopt (3x multiplier). > > Because webview loads the dex, and webview lives in random other apps, > Monochrome falls into the bucket of having full dexopt, making it the same as > pre-N (so, 3x). > > Would be great if you added this explanation as a comment :P (perhaps cleaned up > a bit) Ahhh yes that's what it is, couldn't remember. Fixed up these comments a bit. https://codereview.chromium.org/2930313002/diff/20001/build/android/resource_... build/android/resource_sizes.py:409: member, 'en_' in filename or 'en-' in filename) On 2017/06/16 01:37:16, agrieve wrote: > add int() Oops, must have mixed these up. Added int() to the compressed pak above, left out the extracted_multiplier for stored_translations. Also updated calls to AddZipInfo() to use "extracted_multiplier=".
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/2930313002/#ps60001 (title: "comment typo")
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": 60001, "attempt_start_ts": 1497891659553100,
"parent_rev": "56cf4cd3572f99eaee5d669e9596d4cfd3ff2ad2", "commit_rev":
"3311c02bc4c282ef05b2dc24fbe6b8f4745f5f35"}
Message was sent while issue was closed.
Description was changed from ========== resource_sizes.py: Better pakfile and dex normalization. This CL adds special casing for Monochrome and improves dex size tracking: * Detect whether or not the native lib is extracted for install size * Add a decompression factor for dex files * Account for stored-locales in Monochrome in pakfile normalization BUG=726420 ========== to ========== resource_sizes.py: Better pakfile and dex normalization. This CL adds special casing for Monochrome and improves dex size tracking: * Detect whether or not the native lib is extracted for install size * Add a decompression factor for dex files * Account for stored-locales in Monochrome in pakfile normalization BUG=726420 Review-Url: https://codereview.chromium.org/2930313002 Cr-Commit-Position: refs/heads/master@{#480534} Committed: https://chromium.googlesource.com/chromium/src/+/3311c02bc4c282ef05b2dc24fbe6... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/3311c02bc4c282ef05b2dc24fbe6... |
