-
Notifications
You must be signed in to change notification settings - Fork 22
Description
I was writing some additional unit test and uncovered a bug or two in my code and some odd behaviour in HashMap and friends, in that when updating a Map the semantics are inconsistent
I think that we need some rules and guidelines for what the semantics of an update are
something like
"if the newly inserted key == the existing key and the values are the same the map is unchanged, otherwise a new map it returned containing the new key and value"
I am not sure who owns these libraries, the semantic definition, and whether this is a change for 2.12 or 2.13, or even if you agree. I would be happy to work on this change after confirmation of the semantics required
I had a look at the implementations of += updated etc in the various immutable maps, and they seem inconsistent, and overly memory thirsty
for Map1-4 keys and values are compared with ==, keys a retained if values differ
when adding keys are compared with == values is ignored even if identical
Strangely the original key is maintained - this looks to be a bug IMO
override def updated [V1 >: V] (key: K, value: V1): Map[K, V1] =
if (key == key1) new Map1(key1, value)
else new Map2(key1, value1, key, value)For a HashMap.HashMap1 the comparison is done via identity of the AnyRef view of the value
this can introduce boxing overheads, and false memory churn where the keys and values are primitives due to the vaguaries of boxing
private[collection] override def updated0[B1 >: B](key: A, hash: Int, level: Int, value: B1, kv: (A, B1), merger: Merger[A, B1]): HashMap[A, B1] =
if (hash == this.hash && key == this.key ) {
if (merger eq null) {
if (this.value.asInstanceOf[AnyRe f] eq value.asInstanceOf[AnyRef]) this
else new HashMap1(key, hash, value, kv)ListMap doesnt bother checking the values, it always removes and adds a new (key,value) at the head
This is used in HashMap.HashMapCollision1, as well as directly as a collection class
ListMap implementation is out of step with its scaladoc which states
- This class implements immutable maps using a list-based data structure. List map iterators and
- traversal methods visit key-value pairs in the order whey were first inserted.
IMO I dont thisnk that the order is part of this spec, and limits optimisations anyway, and since it is not correct
I personally think that the scaladoc should not include the ordering
override def +[B2 >: B1](kv: (A, B2)): ListMap[A, B2] = {
val m = this - kv._1
new m.Node[B2](kv._1, kv._2)
}HashTrieMap just delegated to children (HashMap1, HashMapCollision1,HashTrieMap)
so this will follow the semantics of the children, but currently be a victim to the over copying of the data from
HashMap1 and HashMapCollision1
Having a quick look at other Maps
TreeMap always creates
override def updated [B1 >: B](key: A, value: B1): TreeMap[A, B1] = new TreeMap(RB.update(tree, key, value, overwrite = true))
as does IntMap, LongMap etc
Looking a the mutable Maps
ListMap always replaces unconditionally
LinkedHashMap, AnyRefMap, and HashMap updates the value and retains the key
some code that I was using to investigate
package scala.collection.immutable
class Test(val v1:Any, memo:String = "") {
override def equals(o:Any) = o match {
case other:Test => v1 == other.v1
case ignore => false
}
override def hashCode = 0
override def toString=s"Test[$v1, $memo]"
}
object MapBehaviour extends App {
val smallMap = new Map.Map1[Any,Any](0,0) + (0.0 -> 0.0)
println (s"smallMap - $smallMap ${smallMap.getClass}")
val listMap1 = ListMap[Any,Any](0 -> 0) + (1 -> 1)
val listMap2 = listMap1 + (0 -> 0)
println (s"listMap1,2 - $listMap1 $listMap2")
val hashMap1 = HashMap[Any,Any]() + (0 -> 0) + (0.0 -> 0.0)
println (s"hashMap1 - $hashMap1 ${hashMap1.getClass}")
val hashMap10 = HashMap[Any,Any]() + (0 -> 0)
val hashMap11 = hashMap10 + (0 -> 0)
println(s" hashmap10, 11 ${hashMap10 == hashMap11} ${hashMap10 eq hashMap11} ")
val hashMap20 = HashMap[Any,Any]() + (0.0 -> 0.0)
val hashMap21 = hashMap20 + (0.0 -> 0.0)
println(s" hashmap20, 21 ${hashMap20 == hashMap21} ${hashMap20 eq hashMap21} ")
}
and the output ...
smallMap - Map(0 -> 0.0) class scala.collection.immutable.Map $Map1
listMap1,2 - ListMap(0 -> 0, 1 -> 1) ListMap(1 -> 1, 0 -> 0)
hashMap1 - Map(0.0 -> 0.0) class scala.collection.immutable.Has hMap$HashMap1
hashmap10, 11 true true
hashmap20, 21 true false