Skip to content

Conversation

@retronym
Copy link
Member

@retronym retronym commented Oct 2, 2020

In #8726, the optimization to HashMapBuilder fails to overwrite a value
if the existing value is == to it. Map implementations should only
call == on keys and never on values.

This PR does not go further to align other collections with 2.13.x
rules for preserving existing keys on updates.

As added to 2.13.x in df69eb8 and expanded in aa8bf99.

In this commit, the test is commented out. The next commit
will adapt it to compile on 2.12.x.
@ignore tests that currently fail.
Make sure we update values both in `Map.+` and `Builder.++=`.
@scala-jenkins scala-jenkins added this to the 2.12.13 milestone Oct 2, 2020
@retronym retronym requested review from lrytz and mkeskells October 2, 2020 00:19
if (merger eq null) {
if (this.value.asInstanceOf[AnyRef] eq value.asInstanceOf[AnyRef]) this
else new HashMap1(key, hash, value, kv)
else new HashMap1(this.key, hash, value, null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should use kv if we can. I agree the previous was wrong, but how about

  // preserve kv if we can. We know that kv matches key and value
  // and we know if kv ne null that the value is eq, so if the key is also eq 
  // then we can retain the tuple
 else new HashMap1(this.key, hash, value, if (this.key.asInstanceOf[AnyRef] eq key.asInstanceOf[AnyRef]) kv else null)`

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, it ought to be if (this.key eq key) kv else null.

// 32-bit hash collision (rare, but not impossible)
new HashMapCollision1(hash, ListMap.empty.updated(this.key, this.value).updated(key, value))
} else {
val nkv = if ((kv ne null) && (kv._1.asInstanceOf[AnyRef] eq this.key.asInstanceOf[AnyRef])) kv else null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change look wrong to me, and the original looked correct
This is the case where the hashes don't match (as I see it) so kv._1 is unrelated to this.key

As I see it this would cause a corruption in the tree, as the new value would be lost
so - conceptually

val previous = HashMap1(hash1, "a", "A")
val result = previous.updated("b", "B")
//which calls previous.updated(hash2, "b", "B", null, null)

this would give us a
HashTrieMap containing
previous and HashMap1(hash2, "a", "B")
and should be
previous and HashMap1(hash2, "b", "B")

I guess there isn't a test for that, or I am misreading the code

elems = elems + elem
} else {
// assert(elems.size == 4)
elems.get(elem._1) match {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change that you mane is better that the original ...but

It looks to me like the new and old code don't cope with a different value in the map for the same key, and convert to a hashMapBuilder when we could still use a Map4.
Also we could remove the creation of a Some via use of a getOrElse

something like

in object Map

private val flag = new AbstractFunction0[Any, Any] {
  def apply() = flag
}

and here

val valueOrFlag = elems.getOrElse(elem._1, Map.flag).asInstanceOf[AnyRef]
if (valueOrFlag eq Map.flag) {
  //its a new key
 convertToHashMapBuilder()
 hashMapBuilder += elem
} else if (valueOrFlag ne elem._2.asInstanceOf[AnyRef]) 
  // its an existing key with a new value 
  elems = elems + elem
//else as key is == and value is eq, its no change

* rules have explicit tests.
*/
@RunWith(classOf[JUnit4])
class SetMapRulesTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
new tests - Looks like I have some reading and understanding to do!

@retronym retronym force-pushed the backport/set-map-rules-precursor branch 2 times, most recently from 2f82021 to 1409acb Compare October 3, 2020 03:13
Fix these properties:

```
    checkAllValuesUpdated(gen, "++ (identical key)")(_.++(Seq((Value(1,1), Value(101,2)))).filter(_._1.id == 1))

    checkAllValuesUpdated(gen, "++ (identical key)")(_.++(Seq((Value(1,1), Value(101,2)))).filter(_._1.id == 1))
```

Unskip immutable.HashMap
@retronym retronym force-pushed the backport/set-map-rules-precursor branch from 1409acb to 980e2e0 Compare October 4, 2020 23:32
@retronym
Copy link
Member Author

retronym commented Oct 5, 2020

/rebuild

@retronym retronym force-pushed the backport/set-map-rules-precursor branch from 980e2e0 to b60fd93 Compare October 5, 2020 03:19
@retronym retronym requested a review from mkeskells October 5, 2020 03:19
@retronym retronym force-pushed the backport/set-map-rules-precursor branch from 609487b to 5208677 Compare October 5, 2020 08:12
Copy link
Contributor

@mkeskells mkeskells left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants