-
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
Potentially harfmul SynchronizedObject usages on iOS platform #4282
Comments
Waiting for the specific coroutines version, as atomicfu got a lock upgrade recently |
Will this issue be considered solved if |
Would be nice to understand the root of the issue (it seems like it's |
I'll double-check, but after a brief refresher on the source code, I see no fault in |
Might be the case with allocation-spamming lock either, I've asked the user about coroutines version |
Thanks! The culprit seems to be https://github.com/Kotlin/kotlinx-atomicfu/blob/bad63e743ac905a298386446a82a88225c2f71fc/atomicfu/src/nativeMain/kotlin/kotlinx/atomicfu/locks/Synchronized.kt#L13-L66. There is a work in progress to replace them with a more efficient implementation on the atomicfu side, so the problem is expected to be solved in future releases. It's not going to be very soon, though, as writing efficient mutex implementations takes a lot of effort. It doesn't seem like there's evidence of coroutines using the mutex incorrectly, which was the original topic of this issue. Can this be closed? |
I suppose, but a few follow up questions:
Thanks! |
I thought this no longer happened with newer Kotlin and Compose versions? Is there a reason not to upgrade?
Yes, this would be excellent, thanks! You could share a link / an archive / whatever with me in the kotlinlang Slack (
Good point, I don't think there's a public Github issue in atomicfu for tracking this. Let's leave this one open, then, and I'll close it if we do open an atomicfu issue. |
Thank you for the reply! I've sent you my app's codebase on the Koltinlang Slack. Unfortunately, it is still happening with Compose 1.7 and Kotlin 2.1. I just profiled my app in release mode and was able to obtain the freeze. It appears most likely to happen when scrolling lists; I'll be scrolling, navigating back and forth, scroll some more, and then freeze. What's interesting is that the "long" usage of The file is too big to attach here, so I will send it to you as well. |
As another update, as I was scrolling through the same list that can sometimes cause the freezing, I got this Thread Performance Checker message:
|
We've heard that mutex implementations like our one can suffer from QoS problems (Kotlin/kotlinx-atomicfu#462), and this looks like solid proof. Thanks for the codebase! I'm sure it will be invaluable in our investigation. |
@creativedrewy Would it be feasible for you to verify that this updated file for compose-multiplatform-core ios addresses the problem: https://gist.github.com/stefanhaustein/5a36e66672390a8b314e63f46e7baefe (I have a slightly better pull request for atomicfu but unfortunately compose used their own version of synchronizedobject...) |
@stefanhaustein thanks for the reply! I would need to clone the compose core/runtime repositories locally and consume in my app in order to replace with this implementation, correct? |
Yes, exactly (probably via publishing the modified compose core repro to local maven and redirecting your dependency accordingly) P.S: Relevant part of my notes:
|
Sadly we don't have any of this infrastructure in place currently; we're a small team and glancing at the onboarding docs this appears to be a significant effort to get setup. Would you be able to provide a sample configured codebase or binary that I could drop in to my repo? |
Yeah, I fell for the generic androidx onboarding doc as well and wasted half a day setting things up as described there... ¯\(ツ)/¯ More relevant documentation for setting up compose-multiplatform-core locally is here: https://github.com/JetBrains/compose-multiplatform-core/blob/jb-main/MULTIPLATFORM.md Unfortunately, I have not created a fork, so it's a bit tricky to share my changed project... But I really didn't change much (if anything) apart from replacing SynchronizedObject... (I can double-check tomorrow when back at my desk) Basically I did the following steps:
|
P.S.: Turns out I did a fork... https://github.com/stefanhaustein/compose-multiplatform-core It has some extra half-cooked logging but should just work "out of the box" on ios via the local maven repro |
@stefanhaustein what published local version number are you using, and which dependencies should be updated to utilize the local build? For example, I have updated my compose dependency:
Which is the local version number I have specified, but changes to said locally published code do not appear to show in the app. |
I did not set any number and then was using
in the "multiplatform-compose" project that contains some benchmarks for "multiplatform-compose-core". (the change has some noise but should convey the idea O:) |
@creativedrewy Were you able to make any progress here? It would be really great if we could get some more insights into addressing this problem... |
We have a report in public Slack about very slow CMP application: https://kotlinlang.slack.com/archives/C0346LWVBJ4/p1732321550585009
Luckily, the user provided an instrument profile trace, and, it's visible that some huge chunks of work are spent in our synchronization.
Notably, a good chunk (41%) of the time between 02:01 and 02:10 is spent within
kfun:kotlinx.coroutines.internal.LimitedDispatcher.obtainTaskOrDeallocateWorker#internal
->kfun:kotlinx.atomicfu.locks.SynchronizedObject#lock(){}
. It requires further investigation, but it is worth taking a lookInstruments file for the history: Hang iPhone SE CMP 1.7.0.trace.zip
The text was updated successfully, but these errors were encountered: