Apple QoS support via donation #499
Conversation
|
Platform dependencies change diagram: https://docs.google.com/presentation/d/1JtQ5GsjDmhtPKCYkzxPwRp-fD58NiKMw4nIi85_M7kg/edit?usp=sharing |
There was a problem hiding this comment.
Great job!
I have a few essential comments on Apple-related implementation, and it's good to go. To avoid extra-round trips please take a look at new warnings produced by ./gradlew build.
I would also kindly ask (but I'll accept the CR without it) to align non-apple SynchronizedObject with the current implementation as well -- you can copy-paste it but remove QoS mentions, or you can make this code part of nativeMain with expect DonatingMonitor, whatever is easier. Having two different implementations going to be real PITA here
|
|
||
|
|
||
| @ThreadLocal | ||
| var currentThreadId = 0L |
There was a problem hiding this comment.
| var currentThreadId = 0L | |
| internal var currentThreadId = 0L |
| public actual open class SynchronizedObject { | ||
|
|
||
| companion object { | ||
| private const val NO_OWNER = 0L |
There was a problem hiding this comment.
Please move it close to currentThreadId, it does not have to be in its own companion, does it?
| if (lockOwner == NO_OWNER) { | ||
| return | ||
| } | ||
| val ourQosClass = qos_class_self() as UInt |
There was a problem hiding this comment.
This cast might be redundant
|
|
||
| private val owner: AtomicLong = atomic(NO_OWNER) | ||
| private var reEnterCount: Int = 0 | ||
| private val waiters: AtomicInt = atomic(0) |
There was a problem hiding this comment.
Calling this waiters is semantically confusing: if there exists only a single thread that locks the lock, waiters fluctuates between 0 and 1.
Have you considered renaming it? (e.g. threadsOnLock or something like that) and dropping a comment?
| private const val NO_OWNER = 0L | ||
| } | ||
|
|
||
| private val owner: AtomicLong = atomic(NO_OWNER) |
There was a problem hiding this comment.
Please consider naming it ownerThreadId -- generally owner means Any in our lingua
| } else { | ||
| // No existing override, check if we need to set one up. | ||
| memScoped { | ||
| val lockOwnerQosClass = alloc<UIntVar>() |
There was a problem hiding this comment.
It's pretty much likely that this allocation will be happen on each contention and it's a real malloc.
Is it possible to extract this alloc into DonatingMonitor level and free it within cleaner?
There was a problem hiding this comment.
Shouldn't this just be a stack allocation (via memScoped)? It's just used short-lived for getting the values back, perhaps I am holding it wrong?
| val lockOwnerRelPrio = alloc<IntVar>() | ||
| pthread_get_qos_class_np(lockOwner.toCPointer(), lockOwnerQosClass.ptr, lockOwnerRelPrio.ptr) | ||
| if (ourQosClass > lockOwnerQosClass.value) { | ||
| qosOverride = pthread_override_qos_class_start_np(lockOwner.toCPointer(), qos_class_self(), 0) |
There was a problem hiding this comment.
We can reuse ourQosClass here instead of calling qos_class_self, can't we?
atomicfu/src/appleMain/kotlin/kotlinx/atomicfu/locks/Synchronized.kt
Outdated
Show resolved
Hide resolved
I checked the IDE for warnings and fixed the threadlocal package. Not sure about the redundant "public" modifiers? For some bits (forked from the compose implementation) I was not sure to which extent they were outdated or intended for some reason. Unfortunately,
SGTM but let me do this in a followup. |
Addressing issue #462