Stop -keeping all Java native methods |
|||||||||||
Issue descriptionIn https://cs.chromium.org/chromium/src/base/android/proguard/chromium_code.flags, we have: -keepclasseswithmembers,includedescriptorclasses class * { native <methods>; } This should actually be: -keepclasseswithmembernames,includedescriptorclasses class * { native <methods>; } Since native methods are callable only via Java, we actually only want to prevent obfuscation. We don't want to force them to be kept if they are never used. Making this change to Chrome.apk saves ~4kb of dex, and causes ~200 methods to be removed. The list of them are attached (via //tools/android/dexdiffer) Some methods look to be used only by webview (thus not needed in Chrome pre-monochrome). E.g.: nativeClearSslPreferences() Some methods just look to be dead code. e.g.: nativeDistillCurrentPageAndView() The proguard tweak makes it easy to remove the java-side of this overhead, but these unused methods are still being included in the .so, and manually registered via JNI. In order to eliminate that overhead, I think we would first need to implement bug 683256 , and then use the list of progurad-removed native functions as a blacklist for JNI-generator registration. Note: You can easily see the list of removed-by-proguard methods via: grep native gen/chrome/android/chrome_public_apk/chrome_public_apk.proguard.jar.usage
,
Feb 13 2017
,
Feb 13 2017
,
Mar 6 2017
,
May 9 2017
,
Jun 26 2017
Since Monochrome is our future (webview & chrome together), I don't think we should focus much on removing webview-only methods. We could implement stripping of unused methods by generating a linker map file based on proguard results so that only kept native methods are exported from the .so (and thus unused ones will be removed by --gc-sections). "Unused" may include test-only native methods, in which case this would be worth doing if the wins were non-trivial. If not though, it might be better to add some assertions to our build that just fail builds when native methods show up in chrome_public_apk.proguard.jar.usage
,
Jun 27 2018
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 3
This bug has an owner, thus, it's been triaged. Changing status to "assigned".
,
Aug 27
Didn't realize this before, but since monochrome doesn't use explicit JNI registration, it should be possible to enable this only for Monochrome without much trouble.
,
Sep 4
Assigning to Eric - I think the thing to do here is to fix this for monochrome then mark as fixed.
,
Sep 25
,
Sep 26
The following revision refers to this bug: https://chromium.googlesource.com/angle/angle/+/c84510f4a31366aee6602cbdb60b50e1425e8f37 commit c84510f4a31366aee6602cbdb60b50e1425e8f37 Author: Eric Stevenson <estevenson@chromium.org> Date: Wed Sep 26 15:01:32 2018 Angle: Remove obsolete requires_sdk_api_level_23 from BUILD.gn. Will be removed upstream. BUG=chromium:688465 Change-Id: If50ac3624a5f692755797eaabcc0c7d6e7408655 Reviewed-on: https://chromium-review.googlesource.com/1244079 Reviewed-by: Yuly Novikov <ynovikov@google.com> Commit-Queue: Yuly Novikov <ynovikov@google.com> [modify] https://crrev.com/c84510f4a31366aee6602cbdb60b50e1425e8f37/BUILD.gn
,
Sep 26
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c2505bdba5eb52d433202118a07d4c5b78fbd0e1 commit c2505bdba5eb52d433202118a07d4c5b78fbd0e1 Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com> Date: Wed Sep 26 17:46:48 2018 Roll src/third_party/angle a602f9064b3e..c84510f4a313 (1 commits) https://chromium.googlesource.com/angle/angle.git/+log/a602f9064b3e..c84510f4a313 git log a602f9064b3e..c84510f4a313 --date=short --no-merges --format='%ad %ae %s' 2018-09-26 estevenson@chromium.org Angle: Remove obsolete requires_sdk_api_level_23 from BUILD.gn. Created with: gclient setdep -r src/third_party/angle@c84510f4a313 The AutoRoll server is located here: https://autoroll.skia.org/r/angle-chromium-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff, who should be CC'd on the roll, and stop the roller if necessary. CQ_INCLUDE_TRYBOTS=luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel BUG=chromium:688465 TBR=syoussefi@chromium.org Change-Id: I5b9c1211a44f0edb8f7ff124c7def44604595ff2 Reviewed-on: https://chromium-review.googlesource.com/1246261 Reviewed-by: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com> Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com> Cr-Commit-Position: refs/heads/master@{#594377} [modify] https://crrev.com/c2505bdba5eb52d433202118a07d4c5b78fbd0e1/DEPS
,
Sep 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dc8f3c0aba827f550bef775a6645bcbd978052f7 commit dc8f3c0aba827f550bef775a6645bcbd978052f7 Author: Eric Stevenson <estevenson@chromium.org> Date: Thu Sep 27 16:15:39 2018 Android: Allow proguard to strip unused native methods. This CL changes Monochrome APKs to allow proguard to strip out Java native methods that aren't used. For APKs that use explicit JNI registration, this isn't possible since RegisterNatives ends up trying to load classes that are unused and removed by proguard. Bug: 688465 Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;master.tryserver.chromium.android:android_cronet_tester Change-Id: I6816bd68ce47e59bfc9f75dace1b34ab3c200222 Reviewed-on: https://chromium-review.googlesource.com/1244605 Reviewed-by: Paul Jensen <pauljensen@chromium.org> Reviewed-by: agrieve <agrieve@chromium.org> Commit-Queue: Eric Stevenson <estevenson@chromium.org> Cr-Commit-Position: refs/heads/master@{#594743} [modify] https://crrev.com/dc8f3c0aba827f550bef775a6645bcbd978052f7/base/android/proguard/chromium_code.flags [add] https://crrev.com/dc8f3c0aba827f550bef775a6645bcbd978052f7/base/android/proguard/explicit_jni_registration.flags [add] https://crrev.com/dc8f3c0aba827f550bef775a6645bcbd978052f7/base/android/proguard/implicit_jni_registration.flags [modify] https://crrev.com/dc8f3c0aba827f550bef775a6645bcbd978052f7/build/config/android/rules.gni [modify] https://crrev.com/dc8f3c0aba827f550bef775a6645bcbd978052f7/components/cronet/android/BUILD.gn
,
Sep 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/373b4928b64b6980d039810695c2c77181d1d6cf commit 373b4928b64b6980d039810695c2c77181d1d6cf Author: Moe Ahmadi <mahmadi@chromium.org> Date: Thu Sep 27 17:48:06 2018 Revert "Android: Allow proguard to strip unused native methods." This reverts commit dc8f3c0aba827f550bef775a6645bcbd978052f7. Reason for revert: Casing failures https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/android-rel/2442 Original change's description: > Android: Allow proguard to strip unused native methods. > > This CL changes Monochrome APKs to allow proguard to strip out Java > native methods that aren't used. > > For APKs that use explicit JNI registration, this isn't possible since > RegisterNatives ends up trying to load classes that are unused and > removed by proguard. > > Bug: 688465 > Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;master.tryserver.chromium.android:android_cronet_tester > Change-Id: I6816bd68ce47e59bfc9f75dace1b34ab3c200222 > Reviewed-on: https://chromium-review.googlesource.com/1244605 > Reviewed-by: Paul Jensen <pauljensen@chromium.org> > Reviewed-by: agrieve <agrieve@chromium.org> > Commit-Queue: Eric Stevenson <estevenson@chromium.org> > Cr-Commit-Position: refs/heads/master@{#594743} TBR=pauljensen@chromium.org,agrieve@chromium.org,estevenson@chromium.org Change-Id: Ic8e1f808a63c9c0fd119ee36efa1cc2b1e9a0616 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 688465 Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;master.tryserver.chromium.android:android_cronet_tester Reviewed-on: https://chromium-review.googlesource.com/1249825 Reviewed-by: Moe Ahmadi <mahmadi@chromium.org> Commit-Queue: Moe Ahmadi <mahmadi@chromium.org> Cr-Commit-Position: refs/heads/master@{#594774} [modify] https://crrev.com/373b4928b64b6980d039810695c2c77181d1d6cf/base/android/proguard/chromium_code.flags [delete] https://crrev.com/d670d8b867975810309fa6e89410d53fe7d6d395/base/android/proguard/explicit_jni_registration.flags [delete] https://crrev.com/d670d8b867975810309fa6e89410d53fe7d6d395/base/android/proguard/implicit_jni_registration.flags [modify] https://crrev.com/373b4928b64b6980d039810695c2c77181d1d6cf/build/config/android/rules.gni [modify] https://crrev.com/373b4928b64b6980d039810695c2c77181d1d6cf/components/cronet/android/BUILD.gn
,
Sep 27
From: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8934214433486178784/+/steps/compile/0/stdout Failing target: //chrome/android:monochrome_public_test_ar_apk__apk__proguard What I think is happening: ExternalPrerenderHandler has native methods that aren't used in monochrome_public_apk. So proguard is allowed to shrink and obfuscate the class name, and this gets added to the mapping file. Then the test apk uses some test native methods and so thinks (correctly) that the class name can't be obfuscated, but the mapping file passed from monochrome_public_apk indicates otherwise. Not sure how to fix though other than to implement "synchronized proguarding" for test apks..
,
Sep 28
Ideas: * Move the native methods to chrome_public_for_test_apk * Wait for having no more "under_test" apk, and this problem will go away.
,
Sep 28
,
Nov 14
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1a055a9b5ffeda1dd5e09f2ad6c57a93caba8253 commit 1a055a9b5ffeda1dd5e09f2ad6c57a93caba8253 Author: Eric Stevenson <estevenson@chromium.org> Date: Wed Nov 14 18:12:03 2018 Reland "Android: Allow proguard to strip unused native methods." Reland of dc8f3c0aba827f550bef775a6645bcbd978052f7. chrome_public_test_apk is now merged with chrome_public_apk_for_test which should fix the proguard issue. TBR: agrieve@chromium.org Bug: 688465 Change-Id: Ie43eae7f945e5ae444f1cb0adc0703b334402f3c Reviewed-on: https://chromium-review.googlesource.com/c/1333988 Reviewed-by: Eric Stevenson <estevenson@chromium.org> Commit-Queue: Eric Stevenson <estevenson@chromium.org> Cr-Commit-Position: refs/heads/master@{#608038} [modify] https://crrev.com/1a055a9b5ffeda1dd5e09f2ad6c57a93caba8253/base/android/proguard/chromium_code.flags [add] https://crrev.com/1a055a9b5ffeda1dd5e09f2ad6c57a93caba8253/base/android/proguard/explicit_jni_registration.flags [add] https://crrev.com/1a055a9b5ffeda1dd5e09f2ad6c57a93caba8253/base/android/proguard/implicit_jni_registration.flags [modify] https://crrev.com/1a055a9b5ffeda1dd5e09f2ad6c57a93caba8253/build/config/android/rules.gni [modify] https://crrev.com/1a055a9b5ffeda1dd5e09f2ad6c57a93caba8253/components/cronet/android/BUILD.gn
,
Nov 14
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d8af30684b21a6141ab4b4e163eb300f6cb1c4af commit d8af30684b21a6141ab4b4e163eb300f6cb1c4af Author: Roger Tawa <rogerta@chromium.org> Date: Wed Nov 14 19:27:01 2018 Revert "Reland "Android: Allow proguard to strip unused native methods."" This reverts commit 1a055a9b5ffeda1dd5e09f2ad6c57a93caba8253. Reason for revert: broken tree: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/android-rel/4386 Original change's description: > Reland "Android: Allow proguard to strip unused native methods." > > Reland of dc8f3c0aba827f550bef775a6645bcbd978052f7. > > chrome_public_test_apk is now merged with chrome_public_apk_for_test > which should fix the proguard issue. > > TBR: agrieve@chromium.org > Bug: 688465 > Change-Id: Ie43eae7f945e5ae444f1cb0adc0703b334402f3c > Reviewed-on: https://chromium-review.googlesource.com/c/1333988 > Reviewed-by: Eric Stevenson <estevenson@chromium.org> > Commit-Queue: Eric Stevenson <estevenson@chromium.org> > Cr-Commit-Position: refs/heads/master@{#608038} TBR=estevenson@chromium.org Change-Id: Ia718741dad550c0a4c861bd634a21232518dd161 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 688465 Reviewed-on: https://chromium-review.googlesource.com/c/1336064 Reviewed-by: Roger Tawa <rogerta@chromium.org> Commit-Queue: Roger Tawa <rogerta@chromium.org> Cr-Commit-Position: refs/heads/master@{#608074} [modify] https://crrev.com/d8af30684b21a6141ab4b4e163eb300f6cb1c4af/base/android/proguard/chromium_code.flags [delete] https://crrev.com/bec98d732e86eaac12f258610cc49c265315ac24/base/android/proguard/explicit_jni_registration.flags [delete] https://crrev.com/bec98d732e86eaac12f258610cc49c265315ac24/base/android/proguard/implicit_jni_registration.flags [modify] https://crrev.com/d8af30684b21a6141ab4b4e163eb300f6cb1c4af/build/config/android/rules.gni [modify] https://crrev.com/d8af30684b21a6141ab4b4e163eb300f6cb1c4af/components/cronet/android/BUILD.gn |
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by agrieve@chromium.org
, Feb 6 2017