Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2706243013:
Android: improve static initializer counting in resource_sizes.py. (Closed)
|
Created:
3 years, 10 months ago by estevenson Modified:
3 years, 9 months ago Reviewers:
agrieve CC:
chromium-reviews, mikecase+watch_chromium.org, jbudorick+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAndroid: improve static initializer counting in resource_sizes.py.
This CL also includes some other cleanup items:
* Add a helper function for reading a single file from a zip archive
* Ensure stale output dirs aren't used by checking the build id of
.so files
* Use the APK version of .so files instead of the output-dir version
when possible
* Remove obsolete args
* Use argparse instead of optparse
* Use toolchain specific binary utilities
BUG=695177
Review-Url: https://codereview.chromium.org/2706243013
Cr-Commit-Position: refs/heads/master@{#453698}
Committed: https://chromium.googlesource.com/chromium/src/+/636c61da146a7294fa65857a6d5c0ed04f99c586
Patch Set 1 #
Total comments: 4
Patch Set 2 : Remove obsolete args + use correct toolchain #
Total comments: 6
Patch Set 3 : Addressed agrieve comments #
Messages
Total messages: 16 (6 generated)
estevenson@chromium.org changed reviewers: + agrieve@chromium.org
ptal Andrew!
FYI since you're looking at SIs anyways, I think the best way to get them to use the correct tools is to have GN output the toolchain directory via build_vars.txt, and then read that in and pass it to dump-static-initializers.py https://codereview.chromium.org/2706243013/diff/1/build/android/resource_size... File build/android/resource_sizes.py (right): https://codereview.chromium.org/2706243013/diff/1/build/android/resource_size... build/android/resource_sizes.py:153: match = re.search(r'Build ID: (\w{40})', stdout) nit: BuildIDs don't need to be 40 characters (although they should be when using sha1, like we do). It would be better to just take the entire line (since we're just comparing, there's no need to even strip the prefix, although if you want to keep that in that's fine) https://codereview.chromium.org/2706243013/diff/1/build/android/resource_size... build/android/resource_sizes.py:749: so_path = options.so_with_symbols_path This actually makes less sense inside the loop. However, this flag doesn't even look to be used anymore (with one exception), so we should just make it a no-op. Exception is here: https://codereview.chromium.org/2718743002/
On 2017/02/24 20:04:21, agrieve wrote: > FYI since you're looking at SIs anyways, I think the best way to get them to use > the correct tools is to have GN output the toolchain directory via > build_vars.txt, and then read that in and pass it to dump-static-initializers.py > > https://codereview.chromium.org/2706243013/diff/1/build/android/resource_size... > File build/android/resource_sizes.py (right): > > https://codereview.chromium.org/2706243013/diff/1/build/android/resource_size... > build/android/resource_sizes.py:153: match = re.search(r'Build ID: (\w{40})', > stdout) > nit: BuildIDs don't need to be 40 characters (although they should be when using > sha1, like we do). It would be better to just take the entire line (since we're > just comparing, there's no need to even strip the prefix, although if you want > to keep that in that's fine) > > https://codereview.chromium.org/2706243013/diff/1/build/android/resource_size... > build/android/resource_sizes.py:749: so_path = options.so_with_symbols_path > This actually makes less sense inside the loop. However, this flag doesn't even > look to be used anymore (with one exception), so we should just make it a no-op. > > Exception is here: > https://codereview.chromium.org/2718743002/ oh, lgtm!
ptal Andrew! I'll wait to see what happens in https://codereview.chromium.org/2718743002/ before landing this of course. dump-static-initializers.py works with the toolchain, but still gives different results than the other method (curiously always seems to find static initializers in 1 less file). I tried fixing this but didn't have any luck so I'll look into that separately from this CL. Also changed the script to use argparse. https://codereview.chromium.org/2706243013/diff/1/build/android/resource_size... File build/android/resource_sizes.py (right): https://codereview.chromium.org/2706243013/diff/1/build/android/resource_size... build/android/resource_sizes.py:153: match = re.search(r'Build ID: (\w{40})', stdout) On 2017/02/24 20:04:21, agrieve wrote: > nit: BuildIDs don't need to be 40 characters (although they should be when using > sha1, like we do). It would be better to just take the entire line (since we're > just comparing, there's no need to even strip the prefix, although if you want > to keep that in that's fine) Left it in. Done. https://codereview.chromium.org/2706243013/diff/1/build/android/resource_size... build/android/resource_sizes.py:749: so_path = options.so_with_symbols_path On 2017/02/24 20:04:21, agrieve wrote: > This actually makes less sense inside the loop. However, this flag doesn't even > look to be used anymore (with one exception), so we should just make it a no-op. > > Exception is here: > https://codereview.chromium.org/2718743002/ I removed obsolete args and changed the script to just take 1 apk as an argument, was there a reason to allow more?
lgtm https://codereview.chromium.org/2706243013/diff/20001/build/android/resource_... File build/android/resource_sizes.py (right): https://codereview.chromium.org/2706243013/diff/20001/build/android/resource_... build/android/resource_sizes.py:123: tools_prefix = tools_prefix if tools_prefix else '' nit: just make optional parameter be tools_prefix='' https://codereview.chromium.org/2706243013/diff/20001/build/android/resource_... build/android/resource_sizes.py:627: with Unzip(apk_path, pattern="*%s" % apk_so_name) as unzipped_so: is the * prefix necessary? apk_so_name is the complete path I believe. https://codereview.chromium.org/2706243013/diff/20001/build/android/resource_... build/android/resource_sizes.py:684: yield unzipped_files[0] if len(unzipped_files) == 1 else unzipped_files I don't think you use the len()>1 case, Probably better to remove it so there's no risk of wrong types being returned.
Description was changed from ========== Android: use correct lib version in resource_sizes.py. This CL modifies static initializer counting in resource_sizes.py: * Add a helper function for reading a set of files from a zip archive * Ensure stale output dirs aren't used by checking the build id of .so files * Use the APK version of .so files instead of the output-dir version when possible BUG=695177 ========== to ========== Android: improve static initializer counting in resource_sizes.py. This CL also includes some other cleanup items: * Add a helper function for reading a single file from a zip archive * Ensure stale output dirs aren't used by checking the build id of .so files * Use the APK version of .so files instead of the output-dir version when possible * Remove obsolete args * Use argparse instead of optparse * Use toolchain specific binary utilities BUG=695177 ==========
https://codereview.chromium.org/2706243013/diff/20001/build/android/resource_... File build/android/resource_sizes.py (right): https://codereview.chromium.org/2706243013/diff/20001/build/android/resource_... build/android/resource_sizes.py:123: tools_prefix = tools_prefix if tools_prefix else '' On 2017/02/28 15:20:59, agrieve wrote: > nit: just make optional parameter be tools_prefix='' Oops. Done. https://codereview.chromium.org/2706243013/diff/20001/build/android/resource_... build/android/resource_sizes.py:627: with Unzip(apk_path, pattern="*%s" % apk_so_name) as unzipped_so: On 2017/02/28 15:20:59, agrieve wrote: > is the * prefix necessary? apk_so_name is the complete path I believe. Done. https://codereview.chromium.org/2706243013/diff/20001/build/android/resource_... build/android/resource_sizes.py:684: yield unzipped_files[0] if len(unzipped_files) == 1 else unzipped_files On 2017/02/28 15:20:59, agrieve wrote: > I don't think you use the len()>1 case, Probably better to remove it so there's > no risk of wrong types being returned. Sure, also took out predicate since that parameter isn't used. We can add these back again once there's a use case for them. Done.
On 2017/02/28 16:49:57, estevenson wrote: > https://codereview.chromium.org/2706243013/diff/20001/build/android/resource_... > File build/android/resource_sizes.py (right): > > https://codereview.chromium.org/2706243013/diff/20001/build/android/resource_... > build/android/resource_sizes.py:123: tools_prefix = tools_prefix if tools_prefix > else '' > On 2017/02/28 15:20:59, agrieve wrote: > > nit: just make optional parameter be tools_prefix='' > > Oops. Done. > > https://codereview.chromium.org/2706243013/diff/20001/build/android/resource_... > build/android/resource_sizes.py:627: with Unzip(apk_path, pattern="*%s" % > apk_so_name) as unzipped_so: > On 2017/02/28 15:20:59, agrieve wrote: > > is the * prefix necessary? apk_so_name is the complete path I believe. > > Done. > > https://codereview.chromium.org/2706243013/diff/20001/build/android/resource_... > build/android/resource_sizes.py:684: yield unzipped_files[0] if > len(unzipped_files) == 1 else unzipped_files > On 2017/02/28 15:20:59, agrieve wrote: > > I don't think you use the len()>1 case, Probably better to remove it so > there's > > no risk of wrong types being returned. > > Sure, also took out predicate since that parameter isn't used. We can add these > back again once there's a use case for them. Done. Actually, I don't need to wait for https://codereview.chromium.org/2718743002/ to be resolved since it isn't working right now anyways right?
On 2017/02/28 19:33:18, estevenson wrote: > On 2017/02/28 16:49:57, estevenson wrote: > > > https://codereview.chromium.org/2706243013/diff/20001/build/android/resource_... > > File build/android/resource_sizes.py (right): > > > > > https://codereview.chromium.org/2706243013/diff/20001/build/android/resource_... > > build/android/resource_sizes.py:123: tools_prefix = tools_prefix if > tools_prefix > > else '' > > On 2017/02/28 15:20:59, agrieve wrote: > > > nit: just make optional parameter be tools_prefix='' > > > > Oops. Done. > > > > > https://codereview.chromium.org/2706243013/diff/20001/build/android/resource_... > > build/android/resource_sizes.py:627: with Unzip(apk_path, pattern="*%s" % > > apk_so_name) as unzipped_so: > > On 2017/02/28 15:20:59, agrieve wrote: > > > is the * prefix necessary? apk_so_name is the complete path I believe. > > > > Done. > > > > > https://codereview.chromium.org/2706243013/diff/20001/build/android/resource_... > > build/android/resource_sizes.py:684: yield unzipped_files[0] if > > len(unzipped_files) == 1 else unzipped_files > > On 2017/02/28 15:20:59, agrieve wrote: > > > I don't think you use the len()>1 case, Probably better to remove it so > > there's > > > no risk of wrong types being returned. > > > > Sure, also took out predicate since that parameter isn't used. We can add > these > > back again once there's a use case for them. Done. > > Actually, I don't need to wait for https://codereview.chromium.org/2718743002/ > to be resolved since it isn't working right now anyways right? Correct :)
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/2706243013/#ps40001 (title: "Addressed agrieve comments")
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": 40001, "attempt_start_ts": 1488311049209770,
"parent_rev": "285830a522a50523027f3f4bbd0aa8368d2bdc82", "commit_rev":
"636c61da146a7294fa65857a6d5c0ed04f99c586"}
Message was sent while issue was closed.
Description was changed from ========== Android: improve static initializer counting in resource_sizes.py. This CL also includes some other cleanup items: * Add a helper function for reading a single file from a zip archive * Ensure stale output dirs aren't used by checking the build id of .so files * Use the APK version of .so files instead of the output-dir version when possible * Remove obsolete args * Use argparse instead of optparse * Use toolchain specific binary utilities BUG=695177 ========== to ========== Android: improve static initializer counting in resource_sizes.py. This CL also includes some other cleanup items: * Add a helper function for reading a single file from a zip archive * Ensure stale output dirs aren't used by checking the build id of .so files * Use the APK version of .so files instead of the output-dir version when possible * Remove obsolete args * Use argparse instead of optparse * Use toolchain specific binary utilities BUG=695177 Review-Url: https://codereview.chromium.org/2706243013 Cr-Commit-Position: refs/heads/master@{#453698} Committed: https://chromium.googlesource.com/chromium/src/+/636c61da146a7294fa65857a6d5c... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/636c61da146a7294fa65857a6d5c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
