Mutex for Kotlin/Common #508

bbrockbernd · 2025-02-07T09:50:59Z

A FIFO reentrant mutex for Kotlin/Common.
Depends on Thread parking for Kotlin/Common #498 (Parking logic is under review there)

dkhalanskyjb · 2025-02-07T10:42:50Z

It would be better to set native-thread-parking as the base branch of this PR. It's more difficult to review just the mutex when the changes related to parking are also included.

atomicfu/src/commonMain/kotlin/kotlinx/atomicfu/locks/Mutex.kt

dkhalanskyjb · 2025-02-10T12:46:30Z

atomicfu/src/commonMain/kotlin/kotlinx/atomicfu/locks/Mutex.kt

+ * Multiplatform mutex.
+ * On native based on pthread system calls.
+ * On JVM delegates to ReentrantLock.
+ */


This is user-visible documentation, and so it must explain the contracts of the data structure. What is a mutex? What would an example of its usage look like? Is it reentrant? Is it fair? It should mention that it's unsuitable for async (suspend) code, blocks the thread, and is only suitable for low-level technical work.

The same goes for every function in the file: all API entries need to be documented. When does each function return? When can it throw an exception? What does it do?

I have adjusted the docs, wdyt?

Great, thanks!

dkhalanskyjb · 2025-02-10T12:53:50Z

atomicfu/src/commonMain/kotlin/kotlinx/atomicfu/locks/Mutex.kt

+ * On JVM delegates to ReentrantLock.
+ */
+expect class Mutex() {
+    fun isLocked(): Boolean


Even if it turns out someone does need this function, they can do fun Mutex.isLocked() = if (tryLock()) { unlock(); true } else false (right?), so including this function is purely a performance optimization for a performance problem that may not even exist. So, unless we know of some specific use cases for this function, let's not take on the extra commitment of including it.

That is correct. I have tried to keep the api similar to the coroutines mutex's api. But will remove it for now.

P.S. true and false should be swapped right?

On a second thought, this won't work if I am holding the lock myself since it is reentrant.

atomicfu/src/concurrentMain/kotlin/kotlinx/atomicfu/locks/Mutex.kt

atomicfu/src/jsAndWasmSharedMain/kotlin/kotlinx/atomicfu/locks/Mutex.jsAndWasmShared.kt

atomicfu/src/concurrentMain/kotlin/kotlinx/atomicfu/locks/Mutex.kt

dkhalanskyjb · 2025-02-10T13:22:07Z

atomicfu/src/commonMain/kotlin/kotlinx/atomicfu/locks/Mutex.kt

+/**
+ * Multiplatform mutex.
+ * On native based on pthread system calls.
+ * On JVM delegates to ReentrantLock.


This doesn't seem accurate, though? ReentrantLock does get used in synchronized and kotlinx.atomicfu.ReentrantLock, but not in Mutex, right?

If so, there seems to be some inconsistency: on the one hand, atomicfu.ReentrantLock delegates to Java's ReentrantLock, on the other, atomicfu.Mutex doesn't, whereas on Native, atomicfu.ReentrantLock and atomcifu.Mutex are basically the same thing.
If we believe that our mutex is better, we should use it throughout; if we don't, we should use ReentrantLock throughout. Are there measurements showing which one performs better?

atomicfu/api/atomicfu.api

dkhalanskyjb · 2025-02-11T11:42:30Z

atomicfu/src/commonMain/kotlin/kotlinx/atomicfu/locks/Mutex.kt

+ * Multiplatform mutex.
+ * On native based on pthread system calls.
+ * On JVM delegates to ReentrantLock.
+ */


Great, thanks!

atomicfu/src/commonMain/kotlin/kotlinx/atomicfu/locks/Mutex.kt

atomicfu/src/jsAndWasmSharedMain/kotlin/kotlinx/atomicfu/locks/Mutex.jsAndWasmShared.kt

dkhalanskyjb · 2025-06-20T10:56:49Z

atomicfu/api/atomicfu.klib.api

-
-    final fun lock() // kotlinx.atomicfu.locks/NativeMutexNode.lock|lock(){}[0]
-    final fun unlock() // kotlinx.atomicfu.locks/NativeMutexNode.unlock|unlock(){}[0]
-}


I assume removing this implementation detail is safe, but cc @fzhinkin: he probably knows if this was already used by someone else.

Well, there's a lot of API entries that were unintentionally left public.
Last time I checked, nobody actually use them, but it might be worth explicitly announcing the removal prior the release.

explicitly announcing the removal

@fzhinkin Do you mean deprecating these functions?

Yes. Or posting a message in kotlinlang slack, to bring some attention. Anyway, I'll handle it, don't worry about it.

atomicfu/build.gradle.kts

atomicfu/src/commonMain/kotlin/kotlinx/atomicfu/locks/SynchronousMutex.kt

atomicfu/src/concurrentMain/kotlin/kotlinx/atomicfu/locks/NativeMutex.kt

dkhalanskyjb

Well done! All I have left are stylistic nitpicks.

atomicfu/src/jvmTest/kotlin/kotlinx/atomicfu/locks/NativeMutexTimeoutLincheckTest.kt

atomicfu/src/jvmTest/kotlin/kotlinx/atomicfu/locks/NativeMutexReentrantLincheckTest.kt

atomicfu/src/jvmTest/kotlin/kotlinx/atomicfu/locks/NativeMutexLincheckTest.kt

atomicfu/src/jsAndWasmSharedMain/kotlin/kotlinx/atomicfu/locks/Mutex.jsAndWasmShared.kt

atomicfu/src/concurrentTest/kotlin/kotlinx/atomicfu/locks/VaryingContentionTest.kt

atomicfu/src/concurrentTest/kotlin/kotlinx/atomicfu/locks/NativeMutexTest.kt

atomicfu/src/concurrentTest/kotlin/kotlinx/atomicfu/locks/LockWithTimoutTests.kt

fzhinkin

Just in case: did you check the performance characteristics of the new lock with existing kotlinx-coroutines benchmarks?

atomicfu/build.gradle.kts

fzhinkin · 2025-07-15T20:46:55Z

atomicfu/api/atomicfu.klib.api

-
-    final fun lock() // kotlinx.atomicfu.locks/NativeMutexNode.lock|lock(){}[0]
-    final fun unlock() // kotlinx.atomicfu.locks/NativeMutexNode.unlock|unlock(){}[0]
-}


Well, there's a lot of API entries that were unintentionally left public.
Last time I checked, nobody actually use them, but it might be worth explicitly announcing the removal prior the release.

atomicfu/src/concurrentMain/kotlin/kotlinx/atomicfu/locks/NativeMutex.kt

fzhinkin · 2025-07-15T21:00:00Z

It seems like rebase is needed to make CI green.

…tion logic to lincheck tests to check that queue is empty after each execution.

…ggestions

bbrockbernd · 2025-07-24T12:42:47Z

Just in case: did you check the performance characteristics of the new lock with existing kotlinx-coroutines benchmarks?

@dkhalanskyjb tested coroutines performance with the new mutex a while back. It might be worth retesting this since there have been some implementation changes in the meantime.

bbrockbernd requested review from ndkoval and dkhalanskyjb February 7, 2025 09:50

bbrockbernd mentioned this pull request Feb 7, 2025

Mutex for Kotlin Common #494

Closed

bbrockbernd changed the base branch from master to native-thread-parking February 7, 2025 10:49

pjBooms mentioned this pull request Feb 7, 2025

Potentially harfmul SynchronizedObject usages on iOS platform Kotlin/kotlinx.coroutines#4282

Open

dkhalanskyjb suggested changes Feb 10, 2025

View reviewed changes

atomicfu/src/concurrentMain/kotlin/kotlinx/atomicfu/locks/Mutex.kt Outdated Show resolved Hide resolved

dkhalanskyjb suggested changes Feb 10, 2025

View reviewed changes

bbrockbernd requested a review from dkhalanskyjb February 10, 2025 16:07

dkhalanskyjb reviewed Feb 11, 2025

View reviewed changes

bbrockbernd force-pushed the bbrockbernd/common-mutex branch from 71dfd4b to 28f0d71 Compare March 13, 2025 14:42

bbrockbernd force-pushed the bbrockbernd/common-mutex branch 2 times, most recently from bfa21fb to cdc1df9 Compare April 25, 2025 14:18

bbrockbernd force-pushed the native-thread-parking branch from 519e59d to 0d9ea07 Compare April 29, 2025 11:59

bbrockbernd force-pushed the bbrockbernd/common-mutex branch from cdc1df9 to f9c6167 Compare May 1, 2025 08:57

bbrockbernd force-pushed the bbrockbernd/common-mutex branch from f9c6167 to 7a35ef7 Compare June 3, 2025 13:21

bbrockbernd changed the base branch from native-thread-parking to develop June 4, 2025 12:44

bbrockbernd requested a review from dkhalanskyjb June 4, 2025 12:45

dkhalanskyjb suggested changes Jun 25, 2025

View reviewed changes

bbrockbernd requested a review from dkhalanskyjb July 2, 2025 17:57

dkhalanskyjb requested a review from fzhinkin July 3, 2025 06:40

dkhalanskyjb approved these changes Jul 11, 2025

View reviewed changes

fzhinkin self-assigned this Jul 11, 2025

fzhinkin requested changes Jul 15, 2025

View reviewed changes

fzhinkin assigned bbrockbernd and unassigned fzhinkin Jul 23, 2025

bbrockbernd added 2 commits July 24, 2025 14:39

Introduce mutex

bd7b050

Minor improvements

2ca81db

bbrockbernd added 17 commits July 24, 2025 14:39

Revert lincheck version

d01638b

Fix inconsistent package defs

50b57bf

Lazy init native mutex in lincheck tests

59acf29

Add comment

2ec167a

Exclude lincheck tests from the transformed test jobs

72e4a43

Fixed node not being dequed properly after timeout. Also added valida…

79acfb0

…tion logic to lincheck tests to check that queue is empty after each execution.

Fix jsAndWasm comments, remove redundant return, apply terminology su…

44ca9de

…ggestions

Turn redundant ifs into checks, assert for native is still experimental.

072099f

Exclude all tests from transformation that end with "LincheckTest"

8bd65f9

Bugfix mutex

94e82d5

Remove volatiles

2f09419

Use check(){} api

6bb6db5

Apply suggestions

af4a73b

Remove LockIntTest class

9b56875

Refactor nativemutextest

35427d0

Moved lincheck dependency to toml file.

b0fc370

Updated comment according to suggestion

dfb6ea0

bbrockbernd force-pushed the bbrockbernd/common-mutex branch from 5768a08 to dfb6ea0 Compare July 24, 2025 12:39

Update lincheck to latest

c653f27

bbrockbernd requested a review from fzhinkin July 25, 2025 14:06

fzhinkin approved these changes Jul 25, 2025

View reviewed changes

ndkoval removed their request for review August 13, 2025 10:36

Mutex for Kotlin/Common #508

Are you sure you want to change the base?

Mutex for Kotlin/Common #508

Uh oh!

Conversation

bbrockbernd commented Feb 7, 2025

Uh oh!

dkhalanskyjb commented Feb 7, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbrockbernd Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dkhalanskyjb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fzhinkin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fzhinkin commented Jul 15, 2025

Uh oh!

bbrockbernd commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

bbrockbernd Feb 10, 2025 •

edited

Loading

bbrockbernd commented Jul 24, 2025 •

edited

Loading