Remove unnecessary -keep rule for AndroidDispatcherFactory, AndroidEx…

[agrieve]

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]

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]

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]

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]

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?

[dkhalanskyjb]

I think this change should also be in META-INF/com.android.tools/proguard/coroutines.pro.

[agrieve]

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]

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]

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.

[agrieve]

Sounds good! Updated to remove the file.

[dkhalanskyjb]

Thanks!