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

Consider getting rid of common locks and posix interop #324

Open
qwwdfsad opened this issue Jul 19, 2023 · 5 comments
Open

Consider getting rid of common locks and posix interop #324

qwwdfsad opened this issue Jul 19, 2023 · 5 comments
Assignees
Labels

Comments

@qwwdfsad
Copy link
Member

Currently, atomicfu provides access to two [on first glance] distinct API sets that are off overall atomicfu direction and are rather legacy of the past that obstruct the reasoning, maintenance and performance work of other libraries:

  1. Interoperability API. https://github.com/Kotlin/kotlinx-atomicfu/blob/master/atomicfu/src/nativeInterop/cinterop/interop.def

This one leaks to public API (due to how interop is implemented in K/N) and creates additional difficulties in train/aggregate/overall maintenance. This interop part can be completely removed and replaced with the corresponding calls from posix.platform package (e.g. platform.posix.pthread_mutex_init()) .
We consider this step fully backwards compatible.

  1. kotlinx.atomicfu.locks package.
    With the rising popularity of multiplatform, atomicfu starts to be [transitively] present in various projects, including other foundational libraries (link, e.g. Ktor), meaning that our quite ad-hoc and tailored for a very specific need API (SyncrhonizedObject) is leaking into public surface.
    Effectively it means that soon we won't be able to do anything with kotlinx.atomicfu.locks package, sealing its public API.

I propose to start decommissioning it -- starting from warning-level deprecation, eventually raising it to error; in kotlinx.coroutines we can just roll our own, tailored SyncrhonizedObject/ReentrantLock

@qwwdfsad
Copy link
Member Author

It would be nice to do it in 1.9.20 scope, but taking into account, it's a library-level change, it worth returning to it after all Kotlin-repo-related changes are merged. Feel free to split the issue into multiple ones as you see fit

@elizarov
Copy link
Contributor

elizarov commented Jul 19, 2023

Note, that other libraries (like Compose multiplatform) also need locks and, AFAIU, currently depend on atomicfu's locks. Thus, deprecating them without providing a replacement will raise questions. Atomicfu locks need to be replaced with the proper (and fast) locks that should be integrated into the Kotlin standard library. They'll definitely have to come to stdlib as a part of wider support for threads in stdlib, but, in reality, locks don't have to wait for threads and can appear earlier.

P.S. Atomicfu locks are slow (which is another reason to redo them anyway) and that is actually causing concerns in Compose multiplatform.

@qwwdfsad
Copy link
Member Author

Indeed. The moment we deprecate it, we'll see the proper demand and will act accordingly, depending on our constraints.

The poor man's [initial] replacement is to copy-paste SyncrhonizedObject.kt into your codebase and call it a day. Even though it sounds terrible, it's still a better option than locking the ecosystem on the ad-hoc underdesigned intrusive (i.e. encourages subclassing) solution.

should be integrated into the Kotlin standard library.

Sure thing. But first we both have to design proper threading abstraction and gather at least the initial requirements for proper locking

@whyoleg
Copy link

whyoleg commented Jul 19, 2023

Note about 1: removing previously published cinterop could be incompatible change in dependency resolution: icerockdev/moko-units#81 (not sure, that there is YouTrack issue about this)

@whyoleg
Copy link

whyoleg commented Jul 20, 2023

Just speculating and for sure don't know all details, but considering that some day in future:

  • locks could be implemented in stdlib
  • there could be some threads support in sdtlib (JVM and Native?)
  • atomicfu-runtime will become almost just simple no-op artifact where all it declarations will be replaces by compiler plugin (where plugin is coupled with compiler version)
  • there is already internal K/N atomic API which is used by atomicfu compiler plugin

May be in that case it make sense to merge atomic(X) declarations from atomicfu into stdlib? If no, it will be a little strange, that stdlib has volatile, locks and threads, but no atomics.

Or another idea to not clutter stdlib with things needed for specific use-cases (threads, locks, etc) may be it's possible to introduce some new kotlinx.concurrent (name just TBD) with all this, plus may be things like ThreadLocal and some concurrent data structures if needed? (similar to what Stately is/was)

mvicsokolova added a commit that referenced this issue Jun 18, 2024
mvicsokolova added a commit that referenced this issue Jun 20, 2024
@fzhinkin fzhinkin reopened this Jun 24, 2024
@fzhinkin fzhinkin reopened this Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants