Memoization via AtomicMap
#3002
Conversation
Kover Report
|
arrow-libs/core/arrow-atomic/src/commonMain/kotlin/arrow/atomic/AtomicMap.kt
Outdated
Show resolved
Hide resolved
| */ | ||
| public fun merge(key: K, value: V, combine: (old: V, new: V) -> V) { | ||
| while (true) { | ||
| val old = map.get() |
There was a problem hiding this comment.
The old name for the current state of the map is a bit confusing because it matches the parameter used in the combine function.
Maybe you could use oldValue and newValue for combine, couldn't you?
| // used to signal that we have a 'null' as value | ||
| public object Null | ||
|
|
||
| private val map: ConcurrentHashMap<K, Any> = ConcurrentHashMap() |
There was a problem hiding this comment.
Does this give the same guarantees & semantics as Atomic wrapping Map? 🤔
Stately has this implementation, https://github.com/touchlab/Stately/blob/main/stately-concurrent-collections/src/commonMain/kotlin/co/touchlab/stately/collections/ConcurrentMutableMap.kt but that seems to complex to port.
There was a problem hiding this comment.
According to the JVM docs,
A Map providing thread safety and atomicity guarantees.
[..] retrieval operations do not entail locking, and there is not any support for locking the entire table in a way that prevents all access.
If I understood correctly, using ConcurrentHashMap allows us to have different threads updating different keys (for example, if we are memoizing and parallelizing Fibonacci) without locking, as opposed to a single concurrent map.
There was a problem hiding this comment.
Yes, I am more thinking outside of our use-case of memoization by exposing this as a public type. I am not sureAtomicMap and ConcurrentHashMap offer the same guarantees, or if our AtomicMap can be dangerous for performance.
Some simple Google searches don't turn up much results, and I am not familiar with the details of why ConcurrentHashMap exists over AtomicRef + Map.
If your expectation is that through using an AtomicReference, you'll receive a consistent or thread-safe view of the map, or that your operations will be performed atomically, you're most likely to be incorrect
There was a problem hiding this comment.
Fair enough. Since this came from #2998, maybe I can make the corresponding changes there and call it a day?
There was a problem hiding this comment.
Sounds good to me. I am not opposed to this type, but I don't have enough confidence with it to commit to it long term. We can always introduce it later if we have more confidence.
| @@ -0,0 +1,3 @@ | |||
| package arrow.atomic | |||
|
|
|||
| public actual typealias AtomicMap<K, V> = DefaultAtomicMap<K, V> | |||
There was a problem hiding this comment.
This can actually be regular mutable map, no? 🤔
There was a problem hiding this comment.
But then I don't know how to implement the operations like putIfAbsent ':)
There was a problem hiding this comment.
Since JS is single threaded, can it not just be if(!containsKey(k)) put(k, v)? 🤔
There was a problem hiding this comment.
Sorry, I didn't explain my problem correctly. The problem is that I don't know how to use expect/actual in that case. If I do:
public actual typealias AtomicMap<K, V> = HashMap<K, V>then I get the following weird error:
Actual typealias 'AtomicMap' has no corresponding expected declaration
The following declaration is incompatible because some supertypes are missing in the actual declaration:
* public final expect class AtomicMap<K, V> : Map<K, V> defined in arrow.atomic in file AtomicMap.kt
…c/AtomicMap.kt Co-authored-by: Francisco Diaz <[email protected]>
The PR #2998 by @abendt fixes memoization for handling nullable values correctly. The work is wonderful, but when reviewing it two things came to my mind:
nulland supports concurrency is very useful in general,ConcurrentMapin the JVM which we could use to provide faster access in that platform.So I've taken the ideas in @abendt's PR (and also the new tests) and created this new version with an
AtomicMapforarrow-atomic.The crux of the implementation is using a special
Nullobject to representnullwithin theConcurrentMap, because the implementation in the JVM doesn't support storingnullvalues. This is similar to what we do in other parts of Arrow withEMPTY_VALUE.