search

Kotlin / kotlinx-atomicfu

The idiomatic way to use atomic operations in Kotlin
Other
910 stars 58 forks source link

Consider getting rid of common locks and posix interop #324

Open qwwdfsad opened 1 year ago

qwwdfsad commented 1 year ago

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.

2) 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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

whyoleg commented 1 year ago

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

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)