Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Improve resulting APK size by using @Keep annotation #12490

Merged
merged 1 commit into from
Jul 26, 2018
Merged

Conversation

LukasPaczos
Copy link
Contributor

@LukasPaczos LukasPaczos commented Jul 26, 2018

Closes #8978.

Up to this point, when Proguard was run we were keeping all class in the com.mapbox.mapboxsdk package, forbidding removal of unused methods or obfuscation. This PR proposes a switch to @Keep annotation to indicate which resources should be left untouched by Proguard. This targets any code that references Java classes, methods or fields from JNI context, or generally via reflection.

As this can be error-prone, because it's extremely easy to forget to add a @Keep annotation or an entry in the proguard-rules.pro file, which will result in runtime errors, this PR adds lint checks that will make the tests fail. Those checks search for:

  • Any method that has a native modificator - those need to be kept at all cost
  • Any field that has the type long and contains native substring in the name - by convention, this is how we name native pointer for peer objects. This should catch most of the cases, but is by no means sufficient, we need to pay attention to native pointers being kept ourselves.
  • Any constructor that has a long type parameter which contains native substring in the name - similarly to above, by convention, these are constructor invoked from JNI.

For classes that are referenced by the JNI and are outside of the com.mapbox.mapboxsdk we should add an entry to the proguard-rules.pro file.

The most important thing is to remember about keeping Java fields, methods or constructors referenced from the native context, other than above, as those won't be highlighted by the lint check.

The quick tests show, that by minifying our test app we are able to:

  • Decrease method references by 1179 (~15%)
  • Remove 1118 unused methods (~20%)
  • Decrease the size of the Mapbox Maps SDK Java classes in the dex file by 133.8KB (~23%)
  • Get additional benefits of the obfuscation

What's worth noting is that this was done on the internal test app which uses the Maps SDK in nearly every possible way. Benefits in regular consumer apps will be even greater.

I tested the stability by running every single activity in the internal test app and exhausting its options, let me know if there is anything else I can do.

/cc @mapbox/maps-android @mapbox/gl-core

@LukasPaczos LukasPaczos added the Android Mapbox Maps SDK for Android label Jul 26, 2018
@LukasPaczos LukasPaczos added this to the android-v6.4.0 milestone Jul 26, 2018
@LukasPaczos LukasPaczos requested a review from tobrun July 26, 2018 11:47
@tobrun
Copy link
Member

tobrun commented Jul 26, 2018

I tested the stability by running every single activity in the internal test app and exhausting its options, let me know if there is anything else I can do.

Let's get this in the next alpha, this should buy us some time to track down issues that we haven't foreseen.

Copy link
Member

@tobrun tobrun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@@ -1,11 +1,13 @@
# By default, the flags in this file are appended to flags specified
# in ../sdk/tools/proguard/proguard-android.txt,
# contents of this file will be appended into proguard-android.txt
-keepattributes Signature, *Annotation*, EnclosingMethod
-keepattributes Signature, *Annotation*, EnclosingMethod, InnerClasses, Exceptions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to keep attributes as InnerClasses and Exceptions? can this be simplified?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need, added them when testing things, good catch!

@LukasPaczos LukasPaczos merged commit 94ad8e2 into master Jul 26, 2018
@LukasPaczos LukasPaczos deleted the lp-keep-wip branch July 26, 2018 17:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants