-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove unnecessary -keep rule for AndroidDispatcherFactory, AndroidEx… #3263
Conversation
This change was motivated noticing that an androidx update in chrome increased apks size by 90kb. Some investigation found a large chunk was from these unconditional keep rules. It turns out that androidx is currently pulling in the kotlin coroutine dependency, but not has no code that uses the main dispatcher, and so the -keep rule is keeping the class unnecessarily. Some discussion with R8 team led me to find this solution. I couldn't figure out how to properly build the kotlin-coroutines-android.jar, but applying the same changes in chrome show that class to be entirely removed now. If I add:
Then it shows that R8 indeed understands the
|
Ok, I reviewed these changes and understand them in general, but I need some information on how r8 chooses the rule files to use. Searching https://www.google.com/search?hl=en&q=%22r8%22%20%22upto%22, I find only our coroutines project, not any documentation. What happens for R8 with 1.6 < version < 3.0? |
0d02367
to
4c40e79
Compare
I had trouble figuring out how these work as well. Best I could find was this code that shows all matches are applied, and some example matches. So, you're certainly right about the duplicate keep rule being unnecessary. I've removed it from the -upto-1.6.0 file. I don't know what's going on with |
|
@@ -5,3 +5,4 @@ | |||
# - META-INF/com.android.tools/r8-upto-1.6.0/coroutines.pro | |||
|
|||
-keep class kotlinx.coroutines.android.AndroidDispatcherFactory {*;} | |||
-keep class kotlinx.coroutines.android.AndroidExceptionPreHandler {*;} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change should also be in META-INF/com.android.tools/proguard/coroutines.pro
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doh! Done.
Do you know why in r8-from-1.6.0
there is:
-assumenosideeffects class kotlinx.coroutines.internal.FastServiceLoaderKt {
boolean ANDROID_DETECTED return true;
}
but in r8-upto-1.6.0
there is:
-assumenosideeffects class kotlinx.coroutines.internal.FastServiceLoader {
boolean ANDROID_DETECTED return true;
}
E.g. Maybe R8 applied these rules in an odd way before 1.6, or maybe the rule is just wrong in the upto-1.6.0 file (and the file should be deleted).
My guess is that the upto-1.6.0 file should be deleted, yes. Logically, @elizarov, do you by any chance remember if there was a particular reason not to change the |
I think we should just go for it and delete the file. It looks entirely like a typo that nobody investigated before, not a workaround for an R8 bug, and I couldn't find evidence to the contrary. |
…ceptionPreHandler R8 supports keeping and/or optimizing classes found in META-INF/services: https://b.corp.google.com/issues/120436373#comment7 The last R8 commit I can find related to ServerLoader is https://r8-review.googlesource.com/53824 (Sept 2020), so I think R8 1.6 still needs the -keep rule, but 3.0+ should not. Fixes: 3111
Sounds good! Updated to remove the file. |
Thanks! |
R8 supports keeping and/or optimizing classes found in META-INF/services:
https://b.corp.google.com/issues/120436373#comment7
The last R8 commit I can find related to ServerLoader is
https://r8-review.googlesource.com/53824 (Sept 2020),
so I think R8 1.6 still needs the -keep rule, but 3.0+ should not.
I tested with
./gradlew test
, but if failed in the same way with & without my change on javafx tests.Fixes: 3111