Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2214013002:
[Cronet] add base proguard flags (Closed)
Description[Cronet] add base proguard flags
This CL pulls in base's proguard flags when compiling
cronet_sample_apk and cronet_package.
Cronet will not need to duplicate proguard flags and can
benefit from base proguard file changes.
This saves about 1882 - 1471 = 411 methods from our dex
count for CronetSample.apk and 10kB(4.35%).
BUG=634998
Committed: https://crrev.com/eff981189bb608fb2125f3b514ab8a5634bc2c0a
Cr-Commit-Position: refs/heads/master@{#417060}
Patch Set 1 #Patch Set 2 : remove todo #Patch Set 3 : fix tests #Patch Set 4 : self review #
Total comments: 6
Patch Set 5 : address comments #Patch Set 6 : Rebased #
Total comments: 6
Patch Set 7 : Synced to 0855f910f2561da7beb87578f4af99faa6db75ec #
Messages
Total messages: 42 (25 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== [Cronet] add base proguard flags BUG= ========== to ========== [Cronet] add base proguard flags This CL pulls in base's proguard flags when compiling cronet_sample_apk and cronet_package. Cronet will not need to duplicate proguard flags and can benefit from base proguard file changes. This saves about 97 methods from our dex count. BUG= ==========
Description was changed from ========== [Cronet] add base proguard flags This CL pulls in base's proguard flags when compiling cronet_sample_apk and cronet_package. Cronet will not need to duplicate proguard flags and can benefit from base proguard file changes. This saves about 97 methods from our dex count. BUG= ========== to ========== [Cronet] add base proguard flags This CL pulls in base's proguard flags when compiling cronet_sample_apk and cronet_package. Cronet will not need to duplicate proguard flags and can benefit from base proguard file changes. This saves about 97 methods from our dex count and 10kB from CronetSample.apk. BUG=634998 ==========
Description was changed from ========== [Cronet] add base proguard flags This CL pulls in base's proguard flags when compiling cronet_sample_apk and cronet_package. Cronet will not need to duplicate proguard flags and can benefit from base proguard file changes. This saves about 97 methods from our dex count and 10kB from CronetSample.apk. BUG=634998 ========== to ========== [Cronet] add base proguard flags This CL pulls in base's proguard flags when compiling cronet_sample_apk and cronet_package. Cronet will not need to duplicate proguard flags and can benefit from base proguard file changes. This saves about 478 methods from our dex count and 10kB from CronetSample.apk. BUG=634998 ==========
Description was changed from ========== [Cronet] add base proguard flags This CL pulls in base's proguard flags when compiling cronet_sample_apk and cronet_package. Cronet will not need to duplicate proguard flags and can benefit from base proguard file changes. This saves about 478 methods from our dex count and 10kB from CronetSample.apk. BUG=634998 ========== to ========== [Cronet] add base proguard flags This CL pulls in base's proguard flags when compiling cronet_sample_apk and cronet_package. Cronet will not need to duplicate proguard flags and can benefit from base proguard file changes. This saves about 478 methods from our dex count and 10kB(4.35%) from CronetSample.apk. BUG=634998 ==========
xunjieli@chromium.org changed reviewers: + mef@chromium.org
This is very cool! It would be interesting to try to use generated proguard file on other apps that use Cronet to ensure that they are still functional. https://codereview.chromium.org/2214013002/diff/80001/components/cronet/tools... File components/cronet/tools/generate_proguard_file.py (right): https://codereview.chromium.org/2214013002/diff/80001/components/cronet/tools... components/cronet/tools/generate_proguard_file.py:17: target.write(current.read()) what would happen if there is no newline at the end of one of the input files?
On 2016/08/10 01:25:35, mef wrote: > This is very cool! It would be interesting to try to use generated proguard file > on other apps that use Cronet to ensure that they are still functional. Thanks for the review! I tried it with a consumer app. It looks like it doesn't like -optimizationpasses 3 -optimizations !class/merging/* -allowaccessmodification Otherwise everything works. I've sent out an email to ask whether these three flags can be moved out of base. I will put this on hold in the meanwhile. Thanks!
smaier@chromium.org changed reviewers: + smaier@chromium.org
This is a great change! It's always nice to see ProGuard cleanup. https://codereview.chromium.org/2214013002/diff/80001/components/cronet/andro... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2214013002/diff/80001/components/cronet/andro... components/cronet/android/BUILD.gn:395: "//testing/android/proguard_for_test.flags", Perhaps I don't quite understand what the sample_apk's purpose is, but why are you including the testing ProGuard flags here? These flags are meant to be applied to instrumentation tests. https://codereview.chromium.org/2214013002/diff/80001/components/cronet/andro... File components/cronet/android/proguard.cfg (right): https://codereview.chromium.org/2214013002/diff/80001/components/cronet/andro... components/cronet/android/proguard.cfg:5: -keepattributes *Annotation* I'd wager you'll be able to remove this line. base_proguard_config already contains the following lines: -keepattributes RuntimeVisible*Annotations -keepattributes AnnotationDefault Effectively, what this means is that your config will additionally keep RuntimeInvisibleParameterAnnotations, RuntimeInvisibleAnnotations, and RuntimeInvisibleTypeAnnotations on top of what's already kept in base. From my experience, none of these will affect multiple optimization passes. What they will affect, are any tools looking for these compile-time-only annotations in the ProGuarded .class files. Perhaps you still want to keep around compile-time annotations after ProGuard, and if so, ignore this comment.
Thanks! PTAL. https://codereview.chromium.org/2214013002/diff/80001/components/cronet/andro... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2214013002/diff/80001/components/cronet/andro... components/cronet/android/BUILD.gn:395: "//testing/android/proguard_for_test.flags", On 2016/08/11 16:08:28, smaier wrote: > Perhaps I don't quite understand what the sample_apk's purpose is, but why are > you including the testing ProGuard flags here? These flags are meant to be > applied to instrumentation tests. Done. You are right. This shouldn't be here. The "sample/javatests/proguard.cfg" tripped me up. I will move that to the instrumentation test target in a follow-up. https://codereview.chromium.org/2214013002/diff/80001/components/cronet/andro... File components/cronet/android/proguard.cfg (right): https://codereview.chromium.org/2214013002/diff/80001/components/cronet/andro... components/cronet/android/proguard.cfg:5: -keepattributes *Annotation* On 2016/08/11 16:08:29, smaier wrote: > I'd wager you'll be able to remove this line. base_proguard_config already > contains the following lines: > > -keepattributes RuntimeVisible*Annotations > -keepattributes AnnotationDefault > > Effectively, what this means is that your config will additionally keep > RuntimeInvisibleParameterAnnotations, RuntimeInvisibleAnnotations, and > RuntimeInvisibleTypeAnnotations on top of what's already kept in base. From my > experience, none of these will affect multiple optimization passes. > > What they will affect, are any tools looking for these compile-time-only > annotations in the ProGuarded .class files. Perhaps you still want to keep > around compile-time annotations after ProGuard, and if so, ignore this comment. Done. Thanks for pointing that out and explaining the significance of these annotations! That makes sense. We don't have these annotations, so I think the original line was added to deal with base. Since it is already handled by base_proguard_config, I think we can do without this line. https://codereview.chromium.org/2214013002/diff/80001/components/cronet/tools... File components/cronet/tools/generate_proguard_file.py (right): https://codereview.chromium.org/2214013002/diff/80001/components/cronet/tools... components/cronet/tools/generate_proguard_file.py:17: target.write(current.read()) On 2016/08/10 01:25:35, mef wrote: > what would happen if there is no newline at the end of one of the input files? Done. You are right. That will be bad. I changed it so we append line by line.
On 2016/08/11 21:05:53, xunjieli wrote: > Thanks! PTAL. > > https://codereview.chromium.org/2214013002/diff/80001/components/cronet/andro... > File components/cronet/android/BUILD.gn (right): > > https://codereview.chromium.org/2214013002/diff/80001/components/cronet/andro... > components/cronet/android/BUILD.gn:395: > "//testing/android/proguard_for_test.flags", > On 2016/08/11 16:08:28, smaier wrote: > > Perhaps I don't quite understand what the sample_apk's purpose is, but why are > > you including the testing ProGuard flags here? These flags are meant to be > > applied to instrumentation tests. > > Done. You are right. This shouldn't be here. The "sample/javatests/proguard.cfg" > tripped me up. I will move that to the instrumentation test target in a > follow-up. > > https://codereview.chromium.org/2214013002/diff/80001/components/cronet/andro... > File components/cronet/android/proguard.cfg (right): > > https://codereview.chromium.org/2214013002/diff/80001/components/cronet/andro... > components/cronet/android/proguard.cfg:5: -keepattributes *Annotation* > On 2016/08/11 16:08:29, smaier wrote: > > I'd wager you'll be able to remove this line. base_proguard_config already > > contains the following lines: > > > > -keepattributes RuntimeVisible*Annotations > > -keepattributes AnnotationDefault > > > > Effectively, what this means is that your config will additionally keep > > RuntimeInvisibleParameterAnnotations, RuntimeInvisibleAnnotations, and > > RuntimeInvisibleTypeAnnotations on top of what's already kept in base. From my > > experience, none of these will affect multiple optimization passes. > > > > What they will affect, are any tools looking for these compile-time-only > > annotations in the ProGuarded .class files. Perhaps you still want to keep > > around compile-time annotations after ProGuard, and if so, ignore this > comment. > > Done. Thanks for pointing that out and explaining the significance of these > annotations! That makes sense. We don't have these annotations, so I think the > original line was added to deal with base. Since it is already handled by > base_proguard_config, I think we can do without this line. > > https://codereview.chromium.org/2214013002/diff/80001/components/cronet/tools... > File components/cronet/tools/generate_proguard_file.py (right): > > https://codereview.chromium.org/2214013002/diff/80001/components/cronet/tools... > components/cronet/tools/generate_proguard_file.py:17: > target.write(current.read()) > On 2016/08/10 01:25:35, mef wrote: > > what would happen if there is no newline at the end of one of the input files? > > Done. You are right. That will be bad. I changed it so we append line by line. lgtm
lgtm, thanks for doing this!
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
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 ========== [Cronet] add base proguard flags This CL pulls in base's proguard flags when compiling cronet_sample_apk and cronet_package. Cronet will not need to duplicate proguard flags and can benefit from base proguard file changes. This saves about 478 methods from our dex count and 10kB(4.35%) from CronetSample.apk. BUG=634998 ========== to ========== [Cronet] add base proguard flags This CL pulls in base's proguard flags when compiling cronet_sample_apk and cronet_package. Cronet will not need to duplicate proguard flags and can benefit from base proguard file changes. This saves about 1882 - 1471 = 411 methods from our dex count and 10kB(4.35%) from CronetSample.apk. BUG=634998 ==========
Description was changed from ========== [Cronet] add base proguard flags This CL pulls in base's proguard flags when compiling cronet_sample_apk and cronet_package. Cronet will not need to duplicate proguard flags and can benefit from base proguard file changes. This saves about 1882 - 1471 = 411 methods from our dex count and 10kB(4.35%) from CronetSample.apk. BUG=634998 ========== to ========== [Cronet] add base proguard flags This CL pulls in base's proguard flags when compiling cronet_sample_apk and cronet_package. Cronet will not need to duplicate proguard flags and can benefit from base proguard file changes. This saves about 1882 - 1471 = 411 methods from our dex count and 10kB(4.35%) from CronetSample.apk. BUG=634998 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
pauljensen@chromium.org changed reviewers: + pauljensen@chromium.org
https://codereview.chromium.org/2214013002/diff/120001/components/cronet/tool... File components/cronet/tools/generate_proguard_file.py (right): https://codereview.chromium.org/2214013002/diff/120001/components/cronet/tool... components/cronet/tools/generate_proguard_file.py:18: target.write(line) Any reason we don't use "cat" instead of this python program?
https://codereview.chromium.org/2214013002/diff/120001/components/cronet/tool... File components/cronet/tools/generate_proguard_file.py (right): https://codereview.chromium.org/2214013002/diff/120001/components/cronet/tool... components/cronet/tools/generate_proguard_file.py:18: target.write(line) On 2016/08/17 11:04:46, pauljensen wrote: > Any reason we don't use "cat" instead of this python program? Python scripts work across platforms. We can certainly use "cat." In case that we need to do more file processing in the future, it's easier to add in a Python file?
Description was changed from ========== [Cronet] add base proguard flags This CL pulls in base's proguard flags when compiling cronet_sample_apk and cronet_package. Cronet will not need to duplicate proguard flags and can benefit from base proguard file changes. This saves about 1882 - 1471 = 411 methods from our dex count and 10kB(4.35%) from CronetSample.apk. BUG=634998 ========== to ========== [Cronet] add base proguard flags This CL pulls in base's proguard flags when compiling cronet_sample_apk and cronet_package. Cronet will not need to duplicate proguard flags and can benefit from base proguard file changes. This saves about 1882 - 1471 = 411 methods from our dex count for CronetSample.apk and 10kB(4.35%). BUG=634998 ==========
https://codereview.chromium.org/2214013002/diff/120001/components/cronet/tool... File components/cronet/tools/generate_proguard_file.py (right): https://codereview.chromium.org/2214013002/diff/120001/components/cronet/tool... components/cronet/tools/generate_proguard_file.py:18: target.write(line) On 2016/08/17 12:50:31, xunjieli wrote: > On 2016/08/17 11:04:46, pauljensen wrote: > > Any reason we don't use "cat" instead of this python program? > > Python scripts work across platforms. We can certainly use "cat." In case that > we need to do more file processing in the future, it's easier to add in a Python > file? "cat" should work on any platform we build for Android. Chromium already uses "cat" in build files. I don't think we should be reinventing basic shell commands and it's nice to save having to maintain 33 lines of code. If we need more processing in the future I'm not opposed to using python.
https://codereview.chromium.org/2214013002/diff/120001/components/cronet/tool... File components/cronet/tools/generate_proguard_file.py (right): https://codereview.chromium.org/2214013002/diff/120001/components/cronet/tool... components/cronet/tools/generate_proguard_file.py:18: target.write(line) On 2016/08/17 13:04:07, pauljensen wrote: > On 2016/08/17 12:50:31, xunjieli wrote: > > On 2016/08/17 11:04:46, pauljensen wrote: > > > Any reason we don't use "cat" instead of this python program? > > > > Python scripts work across platforms. We can certainly use "cat." In case that > > we need to do more file processing in the future, it's easier to add in a > Python > > file? > > "cat" should work on any platform we build for Android. Chromium already uses > "cat" in build files. I don't think we should be reinventing basic shell > commands and it's nice to save having to maintain 33 lines of code. If we need > more processing in the future I'm not opposed to using python. Thanks for the comments! I asked on Chromium-dev (https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/HYpcKxUzKYg). It looks like we should keep it as it is (using Python rather than shell script, and having the file in cronet/tools rather than reusing a script from a different directory) Let me know if you have any concern. Otherwise I will keep it as it is. I will wait for a day or two in case anyone has different opinions in the forums :)
https://codereview.chromium.org/2214013002/diff/120001/components/cronet/tool... File components/cronet/tools/generate_proguard_file.py (right): https://codereview.chromium.org/2214013002/diff/120001/components/cronet/tool... components/cronet/tools/generate_proguard_file.py:18: target.write(line) On 2016/08/17 17:11:44, xunjieli wrote: > On 2016/08/17 13:04:07, pauljensen wrote: > > On 2016/08/17 12:50:31, xunjieli wrote: > > > On 2016/08/17 11:04:46, pauljensen wrote: > > > > Any reason we don't use "cat" instead of this python program? > > > > > > Python scripts work across platforms. We can certainly use "cat." In case > that > > > we need to do more file processing in the future, it's easier to add in a > > Python > > > file? > > > > "cat" should work on any platform we build for Android. Chromium already uses > > "cat" in build files. I don't think we should be reinventing basic shell > > commands and it's nice to save having to maintain 33 lines of code. If we > need > > more processing in the future I'm not opposed to using python. > > Thanks for the comments! I asked on Chromium-dev > (https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/HYpcKxUzKYg). > It looks like we should keep it as it is (using Python rather than shell script, > and having the file in cronet/tools rather than reusing a script from a > different directory) > > Let me know if you have any concern. Otherwise I will keep it as it is. I will > wait for a day or two in case anyone has different opinions in the forums :) I understand the GN folks' motivations for not using a shell script, but I still feel this is duplicated code but it appears the division between other repositories interferes with deduplication, so I'm fine with leaving it as is. I should note that there are several python scripts that appear to do concatenation but AFAIK none are in the Chromium repo itself: https://cs.chromium.org/search/?q=concat+file:py$+file:cat%5B%5E/%5D*$
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org, smaier@chromium.org Link to the patchset: https://codereview.chromium.org/2214013002/#ps140001 (title: "Synced to 0855f910f2561da7beb87578f4af99faa6db75ec")
https://codereview.chromium.org/2214013002/diff/120001/components/cronet/tool... File components/cronet/tools/generate_proguard_file.py (right): https://codereview.chromium.org/2214013002/diff/120001/components/cronet/tool... components/cronet/tools/generate_proguard_file.py:18: target.write(line) On 2016/08/18 15:46:09, pauljensen wrote: > On 2016/08/17 17:11:44, xunjieli wrote: > > On 2016/08/17 13:04:07, pauljensen wrote: > > > On 2016/08/17 12:50:31, xunjieli wrote: > > > > On 2016/08/17 11:04:46, pauljensen wrote: > > > > > Any reason we don't use "cat" instead of this python program? > > > > > > > > Python scripts work across platforms. We can certainly use "cat." In case > > that > > > > we need to do more file processing in the future, it's easier to add in a > > > Python > > > > file? > > > > > > "cat" should work on any platform we build for Android. Chromium already > uses > > > "cat" in build files. I don't think we should be reinventing basic shell > > > commands and it's nice to save having to maintain 33 lines of code. If we > > need > > > more processing in the future I'm not opposed to using python. > > > > Thanks for the comments! I asked on Chromium-dev > > > (https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/HYpcKxUzKYg). > > It looks like we should keep it as it is (using Python rather than shell > script, > > and having the file in cronet/tools rather than reusing a script from a > > different directory) > > > > Let me know if you have any concern. Otherwise I will keep it as it is. I will > > wait for a day or two in case anyone has different opinions in the forums :) > > I understand the GN folks' motivations for not using a shell script, but I still > feel this is duplicated code but it appears the division between other > repositories interferes with deduplication, so I'm fine with leaving it as is. > > I should note that there are several python scripts that appear to do > concatenation but AFAIK none are in the Chromium repo itself: > https://cs.chromium.org/search/?q=concat+file:py$+file:cat%5B%5E/%5D*$ Acknowledged.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by xunjieli@chromium.org
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xunjieli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Cronet] add base proguard flags This CL pulls in base's proguard flags when compiling cronet_sample_apk and cronet_package. Cronet will not need to duplicate proguard flags and can benefit from base proguard file changes. This saves about 1882 - 1471 = 411 methods from our dex count for CronetSample.apk and 10kB(4.35%). BUG=634998 ========== to ========== [Cronet] add base proguard flags This CL pulls in base's proguard flags when compiling cronet_sample_apk and cronet_package. Cronet will not need to duplicate proguard flags and can benefit from base proguard file changes. This saves about 1882 - 1471 = 411 methods from our dex count for CronetSample.apk and 10kB(4.35%). BUG=634998 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [Cronet] add base proguard flags This CL pulls in base's proguard flags when compiling cronet_sample_apk and cronet_package. Cronet will not need to duplicate proguard flags and can benefit from base proguard file changes. This saves about 1882 - 1471 = 411 methods from our dex count for CronetSample.apk and 10kB(4.35%). BUG=634998 ========== to ========== [Cronet] add base proguard flags This CL pulls in base's proguard flags when compiling cronet_sample_apk and cronet_package. Cronet will not need to duplicate proguard flags and can benefit from base proguard file changes. This saves about 1882 - 1471 = 411 methods from our dex count for CronetSample.apk and 10kB(4.35%). BUG=634998 Committed: https://crrev.com/eff981189bb608fb2125f3b514ab8a5634bc2c0a Cr-Commit-Position: refs/heads/master@{#417060} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/eff981189bb608fb2125f3b514ab8a5634bc2c0a Cr-Commit-Position: refs/heads/master@{#417060} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
