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

KTOR-7698 Make ServiceLoader.load call optimizable by R8 #4454

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

osipxd
Copy link
Member

@osipxd osipxd commented Nov 5, 2024

Subsystem
Client/Server

Motivation
KTOR-7698 ServiceLoader.load call is slow on Android

Solution
Upstreamed @whyoleg's optimization done by him in #4441 and applied it to all ServiceLoader.load calls.

@osipxd osipxd self-assigned this Nov 5, 2024
@osipxd osipxd force-pushed the osipxd/optimize-service-loader-calls branch from e8c4b6d to 0a4432b Compare November 5, 2024 14:05
* See: https://r8.googlesource.com/r8/+/refs/heads/main/src/main/java/com/android/tools/r8/ir/optimize/ServiceLoaderRewriter.java
*/
@InternalAPI
public inline fun <reified T> loadService(): Iterable<T> = Iterable {
Copy link
Contributor

Choose a reason for hiding this comment

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

note: it's better to recheck, that R8 will optimize this pattern with inline and reified usage. While from the code perspective it should be the same, I'm not confident, that the byte code will be the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I'm not sure either 😅 I'm going to test it

Copy link
Member Author

@osipxd osipxd Nov 6, 2024

Choose a reason for hiding this comment

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

Yes, the optimization works!

During the testing I've found a more serious startup performance issue not related to ServiceLoader. We call typeOf during initialization of each plugin, and it is extremely slow when user have kotlin-reflect in dependencies.
Unoptimized service loader call takes ~20ms while initialization of HttpClient with five plugins installed takes more than a second! 😱 I'll file a new issue for this problem.

Copy link
Member Author

@osipxd osipxd Nov 6, 2024

Choose a reason for hiding this comment

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

KTOR-7715 Plugins’ initialization is slow on Android if kotlin-reflect is present

@osipxd osipxd requested a review from bjhham November 6, 2024 14:58
@osipxd osipxd force-pushed the osipxd/optimize-service-loader-calls branch from 0a4432b to 304441b Compare November 6, 2024 15:42
@osipxd osipxd force-pushed the osipxd/optimize-service-loader-calls branch from 304441b to 1fe37d7 Compare November 6, 2024 17:54
Comment on lines +18 to +23
public inline fun <reified T> loadService(): Iterable<T> = Iterable {
ServiceLoader.load(
T::class.java,
T::class.java.classLoader,
).iterator()
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I've tried to replace it with the following code, to retrieve iterator only once:

public inline fun <reified T> loadService(): Iterable<T>
    val iterator = ServiceLoader.load(
        T::class.java,
        T::class.java.classLoader,
    ).iterator()
    
    return Iterable { iterator }
}

But for some reason this change breaks serialization tests 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

iterators are single use, so they can not be used like this and looks like in serialization tests, result of loadService() is called multiple times and so when it's called second time, it will immediately start returning hasNext=false. (e.g https://pl.kotl.in/JkZSEveSX)

So looks like it's better to use Iterable { ... }.toList() as it was before

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants