-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Slow android Dispatchers.Main init #878
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
Comments
ServiceLoader should definitely be avoided on Android. I filed https://issuetracker.google.com/issues/120436373 recently to rewrite it in release builds. |
UPDATE: Now I finally saw what is going on... OMG. What would be the options that we have to avoid ServiceLoader? One option is that we can package a separate android-specific jar-artifact that comes pre-bundled with Android Main dispatcher, but then it'll be a pain to use with coroutine-using non-android specific libraries. |
Does https://github.com/Kotlin/kotlinx.coroutines/blob/master/ui/kotlinx-coroutines-android/src/HandlerDispatcher.kt seems to do a lot more. Looking for temporary workaround as I can't really rollback now :) |
|
More or less, modulo |
Did a quick test by duplicating the original code and making the Main public. The gain is more in the 100/125ms seems I have something else to find, Maybe 100ms is less important and can wait for the R8 solution, providing a simple public getter for the rare people that will notice and care about it in the meantime could be enough? |
What and why is exactly slow in What if we write our own (compatible) implementation that uses |
In the attached flamegraph most of the time is spent in signed JAR verification. |
It appears that JAR is verified all the time even in dex. UPD. But I am not sure whether we should write it: it will add maintenance burden, it may not work on some devices (especially on new Example of how to read service name without verification on Android:
|
Maybe I was not clear but the given values where from production app in release mode and heavily optimized ;) If you do nothing I would highly appreciate to have a public getter in https://github.com/Kotlin/kotlinx.coroutines/blob/master/ui/kotlinx-coroutines-android/src/HandlerDispatcher.kt for those who care about the delay in application init. Currently it seems Kotlin prevent using a package trick to access the internal Main, leading to necessary code duplication just to pass the var public and avoid the issue. Maybe add another public alias with a clear name / Kdoc about it's usage not recommended? |
One more detail, I got some ANR in prod due to that too. Probably due to accessing file from main thread on a busy file system.
|
There's a solution to have the Main dispatcher being automatically injected before You can see some example on something I done for this: https://github.com/LouisCAD/Splitties/blob/e77c909585f1b6d457af0fe18655e4794434ce50/initprovider/README.md |
But... why? That mechanism provides no value here except breaking
multi-process use.
…On Wed, Dec 12, 2018, 3:50 AM Louis CAD ***@***.*** wrote:
There's a solution to have the Main dispatcher being automatically
injected before Application.onCreate() is called. It is by using a
ContentProvider (which are eagerly initialized) and taking advantage of
the default manifest placeholder for applicationId so there's no clash
between apps.
This needs to have the android artifact be published as an aar though.
This could also be done with a separate artifact, provided there's a public
API to make it possible.
You can see some example on something I done for this:
https://github.com/LouisCAD/Splitties/blob/e77c909585f1b6d457af0fe18655e4794434ce50/initprovider/README.md
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#878 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEEEZirmiTG1okdxaAdG8dPy0-8tLEIks5u4MNKgaJpZM4ZJtg4>
.
|
@JakeWharton Because it requires no additional code in consumer apps. in other words, it can fix the issue for the default process in a convenient way (having nothing to do apart from updating). The current slow path can still be used for multiprocess use (so it doesn't break anything). Such processes are usually not the processes where the main UI is run anyway, so cold start time doesn't matter as much. |
That still doesn't answer why...
You know what else doesn't require additional code but is lazy and works in
all processes? Regular class loading.
The content provider hack is only for getting access to a Context. Regular
static initialization is superior in every way when that isn't needed.
…On Wed, Dec 12, 2018, 5:37 AM Louis CAD ***@***.*** wrote:
@JakeWharton <https://github.com/JakeWharton> Because it requires no
additional code in consumer apps. in other words, it can fix the issue for
the default process in a convenient way (having nothing to do apart from
updating).
The current slow path can still be used for multiprocess use (so it
doesn't break anything). Such processes are usually not the processes where
the main UI is run anyway, so cold start time doesn't matter as much.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#878 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEEETqMbX4oA1JMgT0H3s3XxJZr15-Bks5u4NxmgaJpZM4ZJtg4>
.
|
The current design doesn't allow static initialization because About the ContentProvider hack, it is not just for getting access to a Context but equally to run before |
Before talking about how, let's talk about if it should be done as @qwwdfsad seems to think it's not :( When talking about how will come the do 'we do file access on main thread' (as possible solution proposed by @qwwdfsad do too), and does the solution needs to be global or optional? (Meaning should the faster path replace the Dispatchers.Main or be something else that won't interfere with other implementations and open a lot more solutions). |
I see multiple solutions here and I'm especially interested in @JakeWharton opinion on that.
|
Let me disagree on that, was hard to reach top 3% of Play Store ANR (And can't go further due to Android 8+ bug) and any file access on main thread as small as it can be, can have impact on low end phones and block for many seconds before getting access due to some low level locks. |
@qwwdfsad Is there a reason you don't talk about adding a public (possibly experimental) API to inject an instance of the main dispatcher? |
Yes, I've explained them in #810 (comment) |
@qwwdfsad To me, the solution I mentioned, using the I can try to make a PR for this. |
Got confirmation that 1) won't be planned before quite some time as priority is R8 stability before those things and handling the jar change directly at OS is out of possibility minSdk 29 would be 10 years away :) For my needs, I'll go to prod with duplication of the handler and not using the Dispatchers.Main. In what cases / at what point is the CoroutineExceptionHandler serviceLoader started? |
So the argument is that allowing to change the MainDispatcher publicly and globally is bad for any core library like coroutines because this could lead to third-party libraries conflicts, right? Would this argument also hold, if one would just allow global configuration once and further changes are disallowed by the framework (by throwing a Runtime Exception that explains that re-assignment of the main dispatcher is not possible) Isn't this conceptionally the same as defining the service class in META-INF/services/..., in regards of conflicts?: The tradeoff would be compile-time safety vs performance here. I personally could live with the Runtime Exception when loading two 'UI coroutine libraries'. |
Good point you made Sebastian, allowing once is a good option, and since
we're in Kotlin 1.3, this API could be marked as exeprimental with error
level, preventing unaware usages, and likely limiting it to just
kotlinx.coroutines android artifact (if using the ContentProvider hack).
However, instead of throwing, a Boolean could be returned, so only first
set returns true, and subsequent, if any, wouldn't crash the app by default
but would just be ignored.
…On Thu, Dec 13, 2018, 8:43 PM Sebastian Sellmair ***@***.***> wrote:
Yes, I've explained them in #810 (comment)
<#810 (comment)>
So the argument is that allowing to change the default Dispatchers
publicly and globally is bad for any core library like coroutines because
this could lead to third-party libraries conflicts, right?
Would this argument also hold, if one would just allow global
configuration once and further changes are disallowed by the framework (by
throwing a Runtime Exception that explains that re-assignment of the main
dispatcher is not possible)
Isn't this conceptionally the same as defining the service class in
META-INF/services/...?:
Anyone could define his own service there.
The tradeoff would be compile-time safety vs performance here. I
personally could live with the Runtime Exception when loading two 'UI
coroutine libraries'.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#878 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AGpvBYrGt2CKMmJR-OjiSBnMK6ciaBEzks5u4q3bgaJpZM4ZJtg4>
.
|
It is not. Service loader (and actual service interface) mechanism is purely internal. We do not allow third-parties to implement custom main dispatchers, we do not provide any backward compatibility guarantees and there are chances we even forbid implementations not known to our service loader. Set-once is still a source of confusion, non-determinism and (as any other global mutable state provided by a library or a runtime) eventually will end up with reordering your project dependencies along with reflection hacks until you get the desired behaviour. Yes, it is better than "set anything anytime", but it still has the same flaws.
There are no tradeoffs in your solution. The tradeoff between safety and performance is to start using
Yes, everyone could. Until they have tens of dependencies that have 7 different versions of 4 conflicting frameworks* (yes, log4j, slf4j, jul and logback, I'm talking to you) What you (and others) are proposing is a good solution for a local project or for a library which is used only within a single company, especially when you've already seen a similar pattern in * No jokes: https://github.com/eBay/Spark/blob/master/core/src/main/scala/org/apache/spark/Logging.scala#L162 |
While I do understand and approve all the reasons, currently the main issue is that there's no workaround due to everything being internal / private. Can't you just offer a way for those "small" or non multiplatform a way to access the Android Main dispatcher so that we can inject that and use that if we need to? So not a way to inject or alter Dispatchers.Main but a way to access to X that would be the AndroidMainDispatcher that we can then use as we want? Avoid code duplication and maintenance for us too. And can you answer this please?: I fear that's the reason of a slow down when Crashlytics initialize but can't pinpoint it :( |
@Tolriq There is a public non-experimental API for you: https://kotlin.github.io/kotlinx.coroutines/kotlinx-coroutines-android/kotlinx.coroutines.android/android.os.-handler/as-coroutine-dispatcher.html You can statically create a coroutine dispatcher from your handler and then inject it statically throughout your code. As long as you do not explicitly use |
@elizarov yes but as confirmed by @qwwdfsad it does not do exactly the same, I currently have copied the https://github.com/Kotlin/kotlinx.coroutines/blob/master/ui/kotlinx-coroutines-android/src/HandlerDispatcher.kt made Main public and inject that and it works nicely, just sad to have to copy the file to have the var public. (Usually I would use the package trick to access it, but it seems for file level variable this does not work) For the moment I can use that workaround, but it means I may need to update my code copy when I update coroutines as result might be unpredictable if I forget. Do you have the answer to the question about CoroutineExceptionHandler serviceloader call? Trying to find why Crashlytics now impact my startup time even if initialized on background thread and want to know if it can be linked or not. |
It happens the first time this kotlinx.coroutines/core/kotlinx-coroutines-core/src/CoroutineExceptionHandlerImpl.kt Line 21 in d6a5a39
It happens if and only if there is uncaught exception and it was not handled by your code. For Android it means that your app is about to crash.
The only thing you might need to copy from there is this |
For those who will find this, the "fix" mitigate the issue, but this is still disk IO on main thread. On Android, this is still an issue and can still cause slowdown / ANR on busy devices. (Obviously no more than without that patch, but still an issue for those who seek 100% ANR free sessions) |
It is a very isolated IO for a single file that should be in a disk cache anyway. Feel free to open a new issue if you still experience ANR after |
I do Android apps since 8 years every single disk access on main thread can and will trigger an ANR or slow down at some point, flash is slow, 50$ phones have even more bad quality flash. I no more use the official dispatcher but my own to address that, so won't be able to give "evidence" about something well known. I had to provide Google with a patch for their Billing library because on recent updates Google Play did a simple file access on service connection, leading to tons of ANRs. Most users do not check their ANRs stats or vitals and it's really hard to spot on tests without forcing the device to have a busy IO queue, so you probably won't have other users reporting the issue for a while, but it will still be present. I totally understand that Android is not the only / main target and that 0,x% of slow down and 0,0X% of ANR is not a priority. But it's important that for those who matters, they know there's still a disk IO and the cause. |
I just submitted a feature request for the Android Emulator to allow simulating these slow I/O performances while the storage is already busy and risks triggering an ANR because a file has been naively read from the main thread: https://issuetracker.google.com/issues/129900564 Hopefully, this will at some point allow us to reproduce these situations more easily and consistently. |
What we really need is an R8 feature that would desugar |
Preliminary support landed last week. Haven't tested it yet, but I plan to
this week.
…On Sun, Apr 7, 2019, 6:44 AM Roman Elizarov ***@***.***> wrote:
What we really need in a R8 feature that would desugar ServiceLoader
usage into static class initialization based on the META-INF/services
files that are found when building project.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#878 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEEEShh9xry-rQDov0OzzprSzuOtklPks5vecwegaJpZM4ZJtg4>
.
|
@JakeWharton We also need to figure out how we shall write the code so that:
|
Detecting you're being processed by R8 is pretty easy. Detecting what
version of R8... that is less easy.
…On Sun, Apr 7, 2019 at 1:49 PM Roman Elizarov ***@***.***> wrote:
@JakeWharton <https://github.com/JakeWharton> We also need to figure out
how we shall write the code so that:
if (newR8isAvailable) useRegualarServiceLoader() else useOurOwnWorkAround()
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#878 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEEEaQblPR-Jfl6aFtMvMsIak4DXHLCks5vei-qgaJpZM4ZJtg4>
.
|
Why was this issue closed? @qwwdfsad Are there any plans to address this so developers can develop with strict mode on? |
@nak411 Can you, please, create a separate issue with a reproducer code from that SO question, so that we have an appropriate place to discuss and track it. |
No I/O on the Main thread since 1.3.3, we gave up on using |
1) Transaction instance creation on background 2) ExchangeRate api initialization on background 3) Coroutines 1.3.5 no longer accesses disk while constructing main dispatcher (Kotlin/kotlinx.coroutines#878)
It seems Android Main dispatcher initialization / discovery can be quite slow currently.
On a Nokia 8 device my application startup time went from 250ms to 500ms (on release mode). I know that 250ms does not sound much but it's pretty much 2 times slower startup time and it does impact the users as they often want the fastest possible startup time for this kind of application. And I still have to see on older devices if the impact is 250ms or linear with the device power.
See attached flamechart (It's from the debug app with full trace profiling so the times are insanely slow but was necessary to find the issue).
Is there a way to bypass that slow factory discovery and statically create my own Main dispatcher on Android to avoid that slowdown?
The text was updated successfully, but these errors were encountered: