Re: [scala-bts] (SI-7943) TrieMap method getOrElseUpdate is not thread-safe

[Aleksandar Prokopec]

Adjust is called computeIfPresent iirc, in java world.
computeifabsent was to be added for consistency and conversion reasons, as was done in the past with the first concurrent.map trait.

On 12 February 2015 15:10:40 GMT, "Jeff Olson (JIRA)" <scala-i...@googlegroups.com> wrote:

Jeff Olson commented on SI-7943

Re: TrieMap method getOrElseUpdate is not thread-safe This bug report, and most of the conversation, seems to surround the question of whether or not op is executed exactly once, and only if inserted. But this is not the primary issue with the current implementation in my opinion. When I say that getOrElseUpdate is not atomic, I mean that it may return a value that is not, and never was, in the map. If I have multiple threads all calling getOrElseUpdate (and no one calling any other update operation) then only one thread should successfully update the map and all threads should get the value that the first inserted. This is not the case with the current implementation, nor the implementation proposed by Aleksandar above. An implementation that achieves this is quite trivial: override def getOrElseUpdate(key: A, op: => B): B = get(key) match { case Some(v) => v case None => val v = op putIfAbsent(key, v).getOrElse(v) } This should be the default implementation in scala.collection.concurrent.Map (and any pull request that does not include something equivalent is woefully incomplete). Of course, implementations like TrieMap should feel free to override the default implementation if they can achieve the same effect more efficiently or if they wish to add execute-only-if-inserted semantics. Aside: There is no need to add a computeIfAbsent function. This is something I would argue against anyway. I'd rather see a def adjust(key: A, op: Option[B] => Option[B]): Unit function added to the mutable.Map interface, with the additional restriction in concurrent.Map that the whole operation is atomic. Note that this is the equivalent function to Java 8's compute function with null being replaced by Option. Add Comment

This message was sent by Atlassian JIRA (v6.3.11#6341-sha1:83c4d29)

--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[Aleksandar Prokopec]

The fact that it may return a value that was never in the map is precisely what the PR was supposed to fix, and did to my knowledge.

[Aleksandar Prokopec]

A correction:

https://issues.scala-lang.org/browse/SI-7943?focusedCommentId=71796&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-71796

On 2/12/2015 7:08 PM, Aleksandar Prokopec wrote:
> Adjust is called computeIfPresent iirc, in java world.
> computeifabsent was to be added for consistency and conversion reasons, as was done in the past with the first concurrent.map trait.
>
> On 12 February 2015 15:10:40 GMT, "Jeff Olson (JIRA)" <scala-i...@googlegroups.com> wrote:

>> Jeff Olson commented on SI-7943 Re: TrieMap method getOrElseUpdate

>> is not thread-safe
>>
>> This bug report, and most of the conversation, seems to surround the
>> question of whether or not op is executed exactly once, and only if
>> inserted. But this is not the primary issue with the current
>> implementation in my opinion. When I say that getOrElseUpdate is not
>> atomic, I mean that it may return a value that is not, and never was,
>> in the map. If I have multiple threads all calling getOrElseUpdate (and
>> no one calling any other update operation) then only one thread should
>> successfully update the map and all threads should get the value that
>> the first inserted. This is not the case with the current
>> implementation, nor the implementation proposed by Aleksandar above. An
>> implementation that achieves this is quite trivial:
>>
>> override def getOrElseUpdate(key: A, op: => B): B = get(key) match {
>> case Some(v) => v case None => val v = op putIfAbsent(key,
>> v).getOrElse(v) }
>>

>> This should be the default implementation in
>> scala.collection.concurrent.Map (and any pull request that does not
>> include something equivalent is woefully incomplete). Of course,
>> implementations like TrieMap should feel free to override the default
>> implementation if they can achieve the same effect more efficiently or
>> if they wish to add execute-only-if-inserted semantics.
>>
>> Aside:
>> There is no need to add a computeIfAbsent function. This is something I
>> would argue against anyway. I'd rather see a
>>
>> def adjust(key: A, op: Option[B] => Option[B]): Unit
>>

>> function added to the mutable.Map interface, with the additional
>> restriction in concurrent.Map that the whole operation is atomic. Note
>> that this is the equivalent function to Java 8's compute function with
>> null being replaced by Option.
>>