-
Notifications
You must be signed in to change notification settings - Fork 315
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
improvement: Add atomic update on TrieMap #5938
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
package scala.meta.internal.mtags | ||
|
||
import java.util.concurrent.ConcurrentHashMap | ||
|
||
import scala.collection.concurrent.TrieMap | ||
|
||
/** | ||
* This class is a wrapper around TrieMap that provides atomic updateWith | ||
*/ | ||
final class AtomicTrieMap[K, V] { | ||
private val trieMap = new TrieMap[K, V]() | ||
private val concurrentMap = new ConcurrentHashMap[K, V] | ||
|
||
def get(key: K): Option[V] = trieMap.get(key) | ||
|
||
def contains(key: K): Boolean = trieMap.contains(key) | ||
|
||
def updateWith(key: K)(remappingFunc: Option[V] => Option[V]): Unit = { | ||
val computeFunction = new java.util.function.BiFunction[K, V, V] { | ||
override def apply(k: K, v: V): V = { | ||
trieMap.get(key) match { | ||
case Some(value) => | ||
remappingFunc(Some(value)) match { | ||
case Some(newValue) => | ||
trieMap.update(key, newValue) | ||
case None => | ||
trieMap.remove(key) | ||
} | ||
case None => | ||
remappingFunc(None).foreach(trieMap.update(key, _)) | ||
} | ||
null.asInstanceOf[V] | ||
} | ||
} | ||
concurrentMap.compute(key, computeFunction) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't this duplicate the amount of data we hold? Should we just use ConcurrentMap instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Size of concurrentMap is always 0, its only used to provide atomic updateWith for trieMap. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Can you ELI5 this? :D
Can you share some source about that, benchmark maybe? I did some reading, found https://medium.com/@igabaydulin/java-concurrenthashmap-vs-scala-concurrent-map-e185e8a0b798 which then led me to scala/scala#10027 and I need to read everything once again because I'm already lost. Best part - those are not even about TrieMap 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sure :D
To be honest I'm not sure anymore 😅 . I thought using a |
||
} | ||
} | ||
|
||
object AtomicTrieMap { | ||
def empty[K, V]: AtomicTrieMap[K, V] = new AtomicTrieMap[K, V] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, but maybe if inside this function we referenced
k
instead ofkey
(so input ofcomputeFunction
rather than theupdateWith
method) we could then extract it into a constant defined on theAtomicTrieMap
class level so that there's no need to allocate a new one each time anupdateWith
is called? 🤔There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see its possible, because we need to pass it
remappingFunc
.We could change
trieMap
type toTrieMap[K, Set[V]]
, since we only use it like this and add aappend
function, that would look the way you suggested 🤔Edit: I think it won't work either, since we need to pass the value to append
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, you're absolutely right, somehow I missed the
remappingFunc
. 👍