Skip to content
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

Merged
merged 2 commits into from
May 18, 2022

Conversation

agrieve
Copy link
Contributor

@agrieve agrieve commented Apr 21, 2022

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

@agrieve
Copy link
Contributor Author

agrieve commented Apr 21, 2022

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:

-whyareyoukeeping class kotlinx.coroutines.android.AndroidDispatcherFactory {*;}
-keep class kotlinx.coroutines.internal.MainDispatcherLoader { *; }

Then it shows that R8 indeed understands the ServiceLoader.load() call and keeps the dispatcher:

kotlinx.coroutines.android.AndroidDispatcherFactory
|- is reflected from:
|  kotlinx.coroutines.MainCoroutineDispatcher kotlinx.coroutines.internal.MainDispatcherLoader.loadMainDispatcher()
|- is referenced in keep rule:
|  ../../base/android/proguard/chromium_code.flags:112:1
void kotlinx.coroutines.android.AndroidDispatcherFactory.<init>()
|- is reflected from:
|  kotlinx.coroutines.MainCoroutineDispatcher kotlinx.coroutines.internal.MainDispatcherLoader.loadMainDispatcher()
|- is referenced in keep rule:
|  ../../base/android/proguard/chromium_code.flags:112:1
kotlinx.coroutines.MainCoroutineDispatcher kotlinx.coroutines.android.AndroidDispatcherFactory.createDispatcher(java.util.List)
|- is overriding method:
|  kotlinx.coroutines.MainCoroutineDispatcher kotlinx.coroutines.internal.MainDispatcherFactory.createDispatcher(java.util.List)
|- is invoked from:
|  kotlinx.coroutines.MainCoroutineDispatcher kotlinx.coroutines.internal.MainDispatcherLoader.loadMainDispatcher()
|- is referenced in keep rule:
|  ../../base/android/proguard/chromium_code.flags:112:1
java.lang.String kotlinx.coroutines.android.AndroidDispatcherFactory.hintOnError()
|- is overriding method:
|  java.lang.String kotlinx.coroutines.internal.MainDispatcherFactory.hintOnError()
|- is invoked from:
|  kotlinx.coroutines.MainCoroutineDispatcher kotlinx.coroutines.internal.MainDispatcherLoader.loadMainDispatcher()
|- is referenced in keep rule:
|  ../../base/android/proguard/chromium_code.flags:112:1
int kotlinx.coroutines.android.AndroidDispatcherFactory.getLoadPriority()
|- is overriding method:
|  int kotlinx.coroutines.internal.MainDispatcherFactory.getLoadPriority()
|- is invoked from:
|  kotlinx.coroutines.MainCoroutineDispatcher kotlinx.coroutines.internal.MainDispatcherLoader.loadMainDispatcher()
|- is referenced in keep rule:
|  ../../base/android/proguard/chromium_code.flags:112:1

@dkhalanskyjb dkhalanskyjb self-requested a review April 22, 2022 07:42
@dkhalanskyjb
Copy link
Collaborator

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? r8-upto-3.0.0 is used? r8-from-1.6.0? Are they merged together? It looks like the correct behavior is for them to be merged, but then, for version < 1.6, don't we have r8-upto-1.6.0 and r8-upto-3.0.0 also merged, leading to -keep class kotlinx.coroutines.android.AndroidDispatcherFactory {*;} being repeated? I don't get a consistent picture of which files are used.

@agrieve agrieve force-pushed the keep-rules branch 2 times, most recently from 0d02367 to 4c40e79 Compare May 13, 2022 13:50
@agrieve
Copy link
Contributor Author

agrieve commented May 13, 2022

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 FastServiceLoader vs FastServiceLoaderKt :/.

@dkhalanskyjb
Copy link
Collaborator

FastServiceLoader vs FastServiceLoaderKt

FastServiceLoaderKt is the collection of things in the Kotlin file with the name FastServiceLoader.kt, and FastServiceLoader is a class with the name FastServiceLoader. Does this clear things up?

@@ -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 {*;}
Copy link
Collaborator

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.

Copy link
Contributor Author

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).

@dkhalanskyjb
Copy link
Collaborator

My guess is that the upto-1.6.0 file should be deleted, yes. Logically, FastServiceLoaderKt is the right class. In 2019, the rules were written for FastServiceLoader, and then, in https://github.com/Kotlin/kotlinx.coroutines/pull/1282/files#diff-9fdf07285a96f536cd228e56b8888de7a54f998891cc4287010d89ebf807b6fdR8, this was changed for from-1.6.0, but the other file was not edited.

@elizarov, do you by any chance remember if there was a particular reason not to change the from-1.6.0 file as well (see the link above)?

@dkhalanskyjb
Copy link
Collaborator

dkhalanskyjb commented May 17, 2022

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
@agrieve
Copy link
Contributor Author

agrieve commented May 17, 2022

Sounds good! Updated to remove the file.

@dkhalanskyjb dkhalanskyjb merged commit d14b9c5 into Kotlin:develop May 18, 2022
@dkhalanskyjb
Copy link
Collaborator

Thanks!

pablobaxter pushed a commit to pablobaxter/kotlinx.coroutines that referenced this pull request Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants