As discussed at <https://kotlinlang.slack.com/arch...

# korge

[Mayank]

As discussed at https://kotlinlang.slack.com/archives/CJEF0LB6Y/p1608383468306500?thread_ts=1608035617.266900&amp;cid=CJEF0LB6Y, I'm trying to make changes to Logger so that it can be used in multithreaded environments. The main problem is using it from K/N due to concurrency model when using the functionality to have a single logger based on name. I have identified few approaches as follows:
1. Loading the properties from file/str in an immutable shared variable. Remove option to set level and output in Logger (effectively removing mutable variables from Logger) and instead set them using props. This would build a new object even for same name but as the props are immutable, configuration is effectively the same.
2. Use atomic references to hold Logger_* variables. This would have minimal change in functionality but atomicfu dependency need to be added and would also have performance penalty as everytime we need to log, atomic ref will be accessed.
3. Using ThreadLocal Logger and SharedImmutable/ThreadLocal Logger_* global variables. Modifications to Logger in one thread won't be visible to Logger in another thread
What are your thoughts @Deactivated User?

If anyone has any suggestions, I would be happy to have them.

[Deactivated User]

I would vote for option 2

[Mayank]

I was considering option 1 but considering it requires more changes and would reduce some functionality, option 2 looks better.
Thanks for your input.

[Mayank]

I made the changes according to the discussion and raised the PR https://github.com/korlibs/korge-next/pull/116

Out of curiosity, is there any reason why Logger_levels and Logger_outputs were global variable instead of each Logger instance having its own level and output ?

[Deactivated User]

Thanks! I will try to check it today

[Mayank]

Its strange that atomicfu requires reflection. Thanks for updating Klogger 😃
https://github.com/korlibs/korge-next/pull/126

I noticed that instead of update set was used https://github.com/korlibs/korge-next/pull/126/files#diff-ec1a7dc65da05f8a699c16fb4012c916c660ab8bc15943a535d5376347319729R127-R129

I am not sure if there is an issue or not, but in edge case an entry might be missed. What update did was use compareAndSet (https://github.com/Kotlin/kotlinx.atomicfu/blob/5ed01f453286cceca224b191f9e36e362b[…]micfu/src/commonMain/kotlin/kotlinx/atomicfu/AtomicFU.common.kt).

[Deactivated User]

I believe that would only be an issue if there is lots of concurrent updates, and you want to be sure that the new value depends on the previous one. If happening two updates at once, and there is no lock between the generation using the previous value, the old value would be lost. So without a lock you need that loop

[Deactivated User]

Maybe we can place a lock instead

[Mayank]

Lock will work but afaik locks are not available in common. As you said it will happen rarely so loop solution doesn’t look bad (if implementing lock takes more time)

[Deactivated User]

No need to have locks in common. In the end we can provide one implementation for JVM and other for native, JS doesn't require any of this

[Mayank]

Makes sense. I would like to implement it and create a PR.

[Deactivated User]

I'm checking what you proposed

[Deactivated User]

let me verify if it works

[Deactivated User]

becayse I have noticed that Kotlin/Native doesn't seems to provide locks directly. And what it provides is atomics with compareAndSet, so maybe the best option is to directly do that

[Mayank]

👍

[Deactivated User]

Index: klogger/src/commonMain/kotlin/com/soywiz/klogger/atomic/KloggerAtomicRef.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/klogger/src/commonMain/kotlin/com/soywiz/klogger/atomic/KloggerAtomicRef.kt b/klogger/src/commonMain/kotlin/com/soywiz/klogger/atomic/KloggerAtomicRef.kt
--- a/klogger/src/commonMain/kotlin/com/soywiz/klogger/atomic/KloggerAtomicRef.kt	(revision 41dd275d03f5e89e7a8792d415e26fbf5833b194)
+++ b/klogger/src/commonMain/kotlin/com/soywiz/klogger/atomic/KloggerAtomicRef.kt	(date 1611326398353)
@@ -2,19 +2,17 @@
 
 import kotlin.reflect.KProperty
 
-abstract class KloggerAtomicRef<T> {
-    abstract var value: T
-
-    inline fun update(func: (T) -> T) {
-        value = func(value)
-    }
-    operator fun setValue(receiver: Any?, prop: KProperty<*>, newValue: T) {
-        value = newValue
-    }
-    operator fun getValue(receiver: Any?, prop: KProperty<*>): T {
-        return value
-    }
+expect class KloggerAtomicRef<T>(initial: T) {
+    val value: T
+    inline fun update(block: (T) -> T)
+}
+
+operator fun <T> KloggerAtomicRef<T>.setValue(receiver: Any?, prop: KProperty<*>, newValue: T) {
+    update { newValue }
+}
+operator fun <T> KloggerAtomicRef<T>.getValue(receiver: Any?, prop: KProperty<*>): T {
+    return value
 }
 
-expect fun <T> kloggerAtomicRef(initial: T): KloggerAtomicRef<T>
-
+//expect fun <T> kloggerAtomicRef(initial: T): KloggerAtomicRef<T>
+fun <T> kloggerAtomicRef(initial: T): KloggerAtomicRef<T> = KloggerAtomicRef(initial)
Index: klogger/src/nativeCommonMain/kotlin/com/soywiz/klogger/atomic/KloggerAtomicRefNative.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/klogger/src/nativeCommonMain/kotlin/com/soywiz/klogger/atomic/KloggerAtomicRefNative.kt b/klogger/src/nativeCommonMain/kotlin/com/soywiz/klogger/atomic/KloggerAtomicRefNative.kt
--- a/klogger/src/nativeCommonMain/kotlin/com/soywiz/klogger/atomic/KloggerAtomicRefNative.kt	(revision 41dd275d03f5e89e7a8792d415e26fbf5833b194)
+++ b/klogger/src/nativeCommonMain/kotlin/com/soywiz/klogger/atomic/KloggerAtomicRefNative.kt	(date 1611326398355)
@@ -2,10 +2,16 @@
 
 import kotlin.native.concurrent.*
 
-actual fun <T> kloggerAtomicRef(initial: T): KloggerAtomicRef<T> = object : KloggerAtomicRef<T>() {
-    private val ref = FreezableAtomicReference<T>(initial.freeze())
+actual class KloggerAtomicRef<T> actual constructor(initial: T) {
+    @PublishedApi
+    internal val ref = FreezableAtomicReference<T>(initial.freeze())
 
-    override var value: T
-        get() = ref.value
-        set(value) { ref.value = value.freeze() }
+    actual val value: T get() = ref.value
+    actual inline fun update(block: (T) -> T) {
+        //synchronized(ref) { ref.set(ref.get()) }
+        do {
+            val old = ref.value
+            val new = block(old).freeze()
+        } while (!ref.compareAndSet(old, new))
+    }
 }
Index: klogger/src/commonMain/kotlin/com/soywiz/klogger/Logger.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/klogger/src/commonMain/kotlin/com/soywiz/klogger/Logger.kt b/klogger/src/commonMain/kotlin/com/soywiz/klogger/Logger.kt
--- a/klogger/src/commonMain/kotlin/com/soywiz/klogger/Logger.kt	(revision 41dd275d03f5e89e7a8792d415e26fbf5833b194)
+++ b/klogger/src/commonMain/kotlin/com/soywiz/klogger/Logger.kt	(date 1611326398353)
@@ -125,6 +125,6 @@
 
 private inline operator fun <K, V> AtomicMap<K, V>.get(key: K) = value[key]
 private inline operator fun <K, V> AtomicMap<K, V>.set(key: K, value: V) {
-    this.value = HashMap(this.value).also { it[key] = value }
+    this.update { HashMap(it).also { nmap -> nmap[key] = value } }
 }
 
Index: klogger/src/jsMain/kotlin/com/soywiz/klogger/atomic/KloggerAtomicRefJs.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/klogger/src/jsMain/kotlin/com/soywiz/klogger/atomic/KloggerAtomicRefJs.kt b/klogger/src/jsMain/kotlin/com/soywiz/klogger/atomic/KloggerAtomicRefJs.kt
--- a/klogger/src/jsMain/kotlin/com/soywiz/klogger/atomic/KloggerAtomicRefJs.kt	(revision 41dd275d03f5e89e7a8792d415e26fbf5833b194)
+++ b/klogger/src/jsMain/kotlin/com/soywiz/klogger/atomic/KloggerAtomicRefJs.kt	(date 1611326398354)
@@ -1,5 +1,11 @@
 package com.soywiz.klogger.atomic
 
-actual fun <T> kloggerAtomicRef(initial: T): KloggerAtomicRef<T> = object : KloggerAtomicRef<T>() {
-    override var value: T = initial
+actual class KloggerAtomicRef<T> actual constructor(initial: T) {
+    @PublishedApi
+    internal var _value: T = initial
+    actual val value: T get() = _value
+    actual inline fun update(block: (T) -> T) {
+        _value = block(_value)
+    }
 }
+
Index: klogger/src/jvmAndroidMain/kotlin/com/soywiz/klogger/atomic/KloggerAtomicRefJvm.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/klogger/src/jvmAndroidMain/kotlin/com/soywiz/klogger/atomic/KloggerAtomicRefJvm.kt b/klogger/src/jvmAndroidMain/kotlin/com/soywiz/klogger/atomic/KloggerAtomicRefJvm.kt
--- a/klogger/src/jvmAndroidMain/kotlin/com/soywiz/klogger/atomic/KloggerAtomicRefJvm.kt	(revision 41dd275d03f5e89e7a8792d415e26fbf5833b194)
+++ b/klogger/src/jvmAndroidMain/kotlin/com/soywiz/klogger/atomic/KloggerAtomicRefJvm.kt	(date 1611326398354)
@@ -2,9 +2,16 @@
 
 import java.util.concurrent.atomic.AtomicReference
 
-actual fun <T> kloggerAtomicRef(initial: T): KloggerAtomicRef<T> = object : KloggerAtomicRef<T>() {
-    private val ref = AtomicReference<T>(initial)
-    override var value: T
-        get() = ref.get()
-        set(value) { ref.set(value) }
+actual class KloggerAtomicRef<T> actual constructor(initial: T) {
+    @PublishedApi
+    internal val ref = AtomicReference<T>(initial)
+
+    actual val value: T get() = ref.get()
+    actual inline fun update(block: (T) -> T) {
+        //synchronized(ref) { ref.set(ref.get()) }
+        do {
+            val old = ref.get()
+            val new = block(old)
+        } while (!ref.compareAndSet(old, new))
+    }
 }

[Deactivated User]

This is my proposal and seems to work (the other day I tried something else and had problems on JS and Kotlin/Native due to some bugs in the compiler):

[Deactivated User]

Since it seems to work. Can you make a PR in korge-next with your proposed changes, or those if you think that would work for your case?

[Deactivated User]

I plan to make a release of all the libraries later this week, maybe on sunday targeting 1.4.30-RC

[Deactivated User]

so if we can get this right, would be awesome

[Mayank]

On a quick glance of your proposal, it looks goods to me. As you have already implemented it, can you create a branch? I can take an in depth look and can propose some changes on top if that if required.

[Deactivated User]

https://github.com/korlibs/korge-next/tree/klogger.atomic.ref

[Deactivated User]

maybe the implementation should be internal? I think the issue I had with compilers bugs was when the expect class was internal or something like that

[Mayank]

I can try to make it internal. Was the build failing or was it a runtime error?

[Deactivated User]

compile-time when running tests

[Deactivated User]

but it was a different implementation with mixed expect external extensions

[Mayank]

Got it. Let me see what I can do.

[Deactivated User]

👍 thanks!

[Mayank]

I tried adding internal modifier and all tests passed. I also ran an example. It also worked fine.
Sorry, I’m relatively new to KMM, why not use an interface instead of expect/actual for KloggerAtomicRef? I’m asking this because I saw that

kloggerAtomicRef

fun had expect implementation but it is commented so I was wondering if it is required anymore. We can instead directly use the class instead of a function

[Deactivated User]

Not required anymore

[Deactivated User]

It is possible to call the constructor directly. In fact I created the expect function to be able to use AtomicReference from JVM and Kotlin/Native directly as typealiases and make it internal, but in the end I had some compilers bugs, so I ended simplifying stuff. But with the current implementation only the expect class should be required. Would be awesome if the expect classes allowed to provide default implementations for some methods and properties, but since it is not the case I moved some stuff to extension members

[Mayank]

Yes, support for default implementation would be awesome.

[Mayank]

I have made some changes at https://github.com/korlibs/korge-next/pull/130. Request you to do a commit wise review as it should be easier to understand why I took certain decisions and the flow of thought.
Please let me know if any changes are required.