From adef358acc7b8debb2f8deceeaacf974fc21e0f4 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Mon, 14 Apr 2025 13:08:02 +0400 Subject: [PATCH 01/58] 198: Add simple equals test --- .../stress/map/PersistentOrderedMapTest.kt | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 core/commonTest/src/stress/map/PersistentOrderedMapTest.kt diff --git a/core/commonTest/src/stress/map/PersistentOrderedMapTest.kt b/core/commonTest/src/stress/map/PersistentOrderedMapTest.kt new file mode 100644 index 00000000..b4e281cc --- /dev/null +++ b/core/commonTest/src/stress/map/PersistentOrderedMapTest.kt @@ -0,0 +1,20 @@ +/* + * Copyright 2016-2025 JetBrains s.r.o. + * Use of this source code is governed by the Apache 2.0 License that can be found in the LICENSE.txt file. + */ + +package tests.stress.map + +import kotlinx.collections.immutable.persistentMapOf +import kotlin.test.Test +import kotlin.test.assertEquals + +class PersistentOrderedMapTest { + + @Test + fun equalsTest() { + val expected = persistentMapOf("a" to 1, "b" to 2, "c" to 3) + val actual = persistentMapOf().put("a", 1).put("b", 2).put("c", 3) + assertEquals(expected, actual) + } +} \ No newline at end of file From 323f5e66631321401531e84758d7a5de2a635951 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Mon, 14 Apr 2025 13:18:52 +0400 Subject: [PATCH 02/58] 198: Add test from GitHub issue, it passes --- .../stress/map/PersistentOrderedMapTest.kt | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/core/commonTest/src/stress/map/PersistentOrderedMapTest.kt b/core/commonTest/src/stress/map/PersistentOrderedMapTest.kt index b4e281cc..c1661f58 100644 --- a/core/commonTest/src/stress/map/PersistentOrderedMapTest.kt +++ b/core/commonTest/src/stress/map/PersistentOrderedMapTest.kt @@ -17,4 +17,40 @@ class PersistentOrderedMapTest { val actual = persistentMapOf().put("a", 1).put("b", 2).put("c", 3) assertEquals(expected, actual) } + + private class ChosenHashCode( + private val hashCode: Int, + private val name: String, + ) { + override fun equals(other: Any?): Boolean { + return other is ChosenHashCode && (other.name == name) + } + + override fun hashCode(): Int { + return hashCode + } + + override fun toString(): String { + return name + } + } + + @Test + fun testFromGitHubIssue() { + val a = ChosenHashCode(123, "A") + val b = ChosenHashCode(123, "B") + val c = ChosenHashCode(123, "C") + + val abc = persistentMapOf( + a to "x", + b to "y", + c to "z", + ) + + val minusAb = abc.minus(arrayOf(a, b)) + val cOnly = persistentMapOf(c to "z") + + assertEquals(minusAb.entries, cOnly.entries) + assertEquals(minusAb, cOnly) + } } \ No newline at end of file From e91449cce92ae7c5a22928bfc36551953a10d0ad Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Mon, 14 Apr 2025 13:33:57 +0400 Subject: [PATCH 03/58] 198: Add some checks --- core/commonTest/src/stress/map/PersistentOrderedMapTest.kt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/commonTest/src/stress/map/PersistentOrderedMapTest.kt b/core/commonTest/src/stress/map/PersistentOrderedMapTest.kt index c1661f58..49622b4e 100644 --- a/core/commonTest/src/stress/map/PersistentOrderedMapTest.kt +++ b/core/commonTest/src/stress/map/PersistentOrderedMapTest.kt @@ -50,7 +50,8 @@ class PersistentOrderedMapTest { val minusAb = abc.minus(arrayOf(a, b)) val cOnly = persistentMapOf(c to "z") - assertEquals(minusAb.entries, cOnly.entries) + assertEquals(cOnly.entries, minusAb.entries) + assertEquals(cOnly, minusAb) assertEquals(minusAb, cOnly) } } \ No newline at end of file From a6d39a58e3f320df19803daa5b2692bc41f07a49 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Mon, 14 Apr 2025 13:46:15 +0400 Subject: [PATCH 04/58] 204: PersistentOrderedSetTest.equalsTestFromGitHubIssue --- .../stress/set/PersistentOrderedSetTest.kt | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 core/commonTest/src/stress/set/PersistentOrderedSetTest.kt diff --git a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt new file mode 100644 index 00000000..e8302df4 --- /dev/null +++ b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt @@ -0,0 +1,28 @@ +/* + * Copyright 2016-2025 JetBrains s.r.o. + * Use of this source code is governed by the Apache 2.0 License that can be found in the LICENSE.txt file. + */ + +package tests.stress.set + +import kotlinx.collections.immutable.persistentSetOf +import kotlin.test.Test +import kotlin.test.assertEquals + +class PersistentOrderedSetTest { + + @Test + fun equalsTestFromGitHubIssue() { + val set1 = persistentSetOf(-486539264, 16777216, 0, 67108864) + val builder = set1.builder() + + assertEquals(set1, builder.build().toSet()) + assertEquals(set1, builder.build()) + + val set2 = set1.remove(0) + builder.remove(0) + + assertEquals(set2, builder.build().toSet()) + assertEquals(set2, builder.build()) + } +} \ No newline at end of file From 0502de2e08b41735b428c379145dd4ac8666caed Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Mon, 14 Apr 2025 13:48:46 +0400 Subject: [PATCH 05/58] 204: Simplify equalsTestFromGitHubIssue --- core/commonTest/src/stress/set/PersistentOrderedSetTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt index e8302df4..5690f4ec 100644 --- a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt +++ b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt @@ -13,7 +13,7 @@ class PersistentOrderedSetTest { @Test fun equalsTestFromGitHubIssue() { - val set1 = persistentSetOf(-486539264, 16777216, 0, 67108864) + val set1 = persistentSetOf(-1, 0, 67108864) val builder = set1.builder() assertEquals(set1, builder.build().toSet()) From 3f6e472e6857b4861659003ed969b30e9b91e830 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Mon, 14 Apr 2025 13:49:44 +0400 Subject: [PATCH 06/58] 204: Simplify equalsTestFromGitHubIssue --- core/commonTest/src/stress/set/PersistentOrderedSetTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt index 5690f4ec..babb477f 100644 --- a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt +++ b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt @@ -13,7 +13,7 @@ class PersistentOrderedSetTest { @Test fun equalsTestFromGitHubIssue() { - val set1 = persistentSetOf(-1, 0, 67108864) + val set1 = persistentSetOf(-1, 0, 1_000_000) val builder = set1.builder() assertEquals(set1, builder.build().toSet()) From 0091e35927ea4f67c8ac4bae0b2e741afb487998 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Mon, 14 Apr 2025 13:50:08 +0400 Subject: [PATCH 07/58] 204: Simplify equalsTestFromGitHubIssue --- core/commonTest/src/stress/set/PersistentOrderedSetTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt index babb477f..e37b95f0 100644 --- a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt +++ b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt @@ -13,7 +13,7 @@ class PersistentOrderedSetTest { @Test fun equalsTestFromGitHubIssue() { - val set1 = persistentSetOf(-1, 0, 1_000_000) + val set1 = persistentSetOf(-1, 0, 100_000) val builder = set1.builder() assertEquals(set1, builder.build().toSet()) From c31a5cbca1ab0db8d3dc0b2b51a368a86bc24b25 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Mon, 14 Apr 2025 13:50:43 +0400 Subject: [PATCH 08/58] 204: Simplify equalsTestFromGitHubIssue --- core/commonTest/src/stress/set/PersistentOrderedSetTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt index e37b95f0..16a9e117 100644 --- a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt +++ b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt @@ -13,7 +13,7 @@ class PersistentOrderedSetTest { @Test fun equalsTestFromGitHubIssue() { - val set1 = persistentSetOf(-1, 0, 100_000) + val set1 = persistentSetOf(-1, 0, 65536) val builder = set1.builder() assertEquals(set1, builder.build().toSet()) From 7d7f44bcbf57701808b1b30474d622c6d73a1e0c Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Mon, 14 Apr 2025 13:52:22 +0400 Subject: [PATCH 09/58] 204: Add comment with strange behaviour --- core/commonTest/src/stress/set/PersistentOrderedSetTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt index 16a9e117..f9a91257 100644 --- a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt +++ b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt @@ -13,7 +13,7 @@ class PersistentOrderedSetTest { @Test fun equalsTestFromGitHubIssue() { - val set1 = persistentSetOf(-1, 0, 65536) + val set1 = persistentSetOf(-1, 0, 65536) // test passes with 65535 third parameter val builder = set1.builder() assertEquals(set1, builder.build().toSet()) From fd8d6bf01acb9c48da6439daa79f0104b2480a80 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Mon, 14 Apr 2025 15:49:30 +0400 Subject: [PATCH 10/58] 204: Add equalsTestPersistentMap --- .../stress/set/PersistentOrderedSetTest.kt | 25 ++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt index f9a91257..fb35174a 100644 --- a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt +++ b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt @@ -5,6 +5,9 @@ package tests.stress.set +import kotlinx.collections.immutable.PersistentSet +import kotlinx.collections.immutable.persistentHashMapOf +import kotlinx.collections.immutable.persistentMapOf import kotlinx.collections.immutable.persistentSetOf import kotlin.test.Test import kotlin.test.assertEquals @@ -13,7 +16,7 @@ class PersistentOrderedSetTest { @Test fun equalsTestFromGitHubIssue() { - val set1 = persistentSetOf(-1, 0, 65536) // test passes with 65535 third parameter + val set1: PersistentSet = persistentSetOf(-1, 0, 65536) // test passes with 65535 third parameter val builder = set1.builder() assertEquals(set1, builder.build().toSet()) @@ -22,7 +25,27 @@ class PersistentOrderedSetTest { val set2 = set1.remove(0) builder.remove(0) + println("set2.equals(builder.build()): ${set2.equals(builder.build())}") + assertEquals(set2, builder.build().toSet()) assertEquals(set2, builder.build()) } + + @Test + fun equalsTestPersistentMap() { + val map1 = persistentHashMapOf(-1 to "1", 0 to "0", 65536 to "65536") + val builder = map1.builder() + + assertEquals(map1, builder.build().toMap()) + assertEquals(map1, builder.build()) + + val map2 = map1.remove(0) + builder.remove(0) + val map3 = builder.build() + + println("map2.equals(map3): ${map2.equals(map3)}") + + assertEquals(map2, builder.build().toMap()) + assertEquals(map2, builder.build()) + } } \ No newline at end of file From 9b7d94f2b9fbc97a5d0223da3f76062394760b48 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Mon, 14 Apr 2025 15:51:27 +0400 Subject: [PATCH 11/58] 204: Refactor equalsTestPersistentMap --- .../src/stress/set/PersistentOrderedSetTest.kt | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt index fb35174a..b8117b21 100644 --- a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt +++ b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt @@ -35,17 +35,18 @@ class PersistentOrderedSetTest { fun equalsTestPersistentMap() { val map1 = persistentHashMapOf(-1 to "1", 0 to "0", 65536 to "65536") val builder = map1.builder() + val map11 = builder.build() - assertEquals(map1, builder.build().toMap()) - assertEquals(map1, builder.build()) + assertEquals(map1, map11.toMap()) + assertEquals(map1, map11) val map2 = map1.remove(0) builder.remove(0) - val map3 = builder.build() + val map22 = builder.build() - println("map2.equals(map3): ${map2.equals(map3)}") + println("map2.equals(map22): ${map2.equals(map22)}") - assertEquals(map2, builder.build().toMap()) - assertEquals(map2, builder.build()) + assertEquals(map2, map22.toMap()) + assertEquals(map2, map22) } } \ No newline at end of file From 0bac5393c66dc9c082c7a3f2199b9afba1de15c5 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Mon, 14 Apr 2025 15:56:08 +0400 Subject: [PATCH 12/58] 204: Refactor equalsTestPersistentMap --- core/commonTest/src/stress/set/PersistentOrderedSetTest.kt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt index b8117b21..ead9a8ea 100644 --- a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt +++ b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt @@ -6,6 +6,7 @@ package tests.stress.set import kotlinx.collections.immutable.PersistentSet +import kotlinx.collections.immutable.implementations.immutableMap.PersistentHashMap import kotlinx.collections.immutable.persistentHashMapOf import kotlinx.collections.immutable.persistentMapOf import kotlinx.collections.immutable.persistentSetOf @@ -33,7 +34,7 @@ class PersistentOrderedSetTest { @Test fun equalsTestPersistentMap() { - val map1 = persistentHashMapOf(-1 to "1", 0 to "0", 65536 to "65536") + val map1 = persistentHashMapOf(-1 to "1", 0 to "0", 65536 to "65536") as PersistentHashMap val builder = map1.builder() val map11 = builder.build() From 8d4cda717c45bc5b5c1c3793a23eb4376cf536ed Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Fri, 18 Apr 2025 12:05:33 +0400 Subject: [PATCH 13/58] 204: equalsTestPersistentMap --- core/commonTest/src/stress/set/PersistentOrderedSetTest.kt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt index ead9a8ea..a45fec73 100644 --- a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt +++ b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt @@ -34,7 +34,7 @@ class PersistentOrderedSetTest { @Test fun equalsTestPersistentMap() { - val map1 = persistentHashMapOf(-1 to "1", 0 to "0", 65536 to "65536") as PersistentHashMap + val map1 = persistentHashMapOf(-1 to "minus-one", 0 to "zero", 65536 to "power-of-two") as PersistentHashMap val builder = map1.builder() val map11 = builder.build() @@ -45,6 +45,8 @@ class PersistentOrderedSetTest { builder.remove(0) val map22 = builder.build() + val a = arrayOf(map1.node, map11.node, map2.node, map22.node) + println("map2.equals(map22): ${map2.equals(map22)}") assertEquals(map2, map22.toMap()) From 08d93ed4fd5dadc4ed051d0c674059e4dac04172 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Fri, 18 Apr 2025 12:16:40 +0400 Subject: [PATCH 14/58] 204: Add simpleTest --- .../stress/set/PersistentOrderedSetTest.kt | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt index a45fec73..d6e24042 100644 --- a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt +++ b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt @@ -15,6 +15,27 @@ import kotlin.test.assertEquals class PersistentOrderedSetTest { + @Test + fun simpleTest() { + data class Data(val value: Int, val hash: Int = 1) { + override fun hashCode(): Int { + return hash + } + } + + var map1: PersistentHashMap = persistentHashMapOf(Data(1) to Data(1)) as PersistentHashMap + map1 = map1.put(Data(2), Data(2)) + map1 = map1.remove(Data(2)) + + var map2: PersistentHashMap = persistentHashMapOf(Data(2) to Data(2)) as PersistentHashMap + map2 = map2.put(Data(1), Data(1)) + map2 = map2.remove(Data(2)) + + println(map1.equals(map2)) + + assertEquals(map1, map2) + } + @Test fun equalsTestFromGitHubIssue() { val set1: PersistentSet = persistentSetOf(-1, 0, 65536) // test passes with 65535 third parameter From 5e175fbc13b2a05872013268098fb9179d368297 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Fri, 18 Apr 2025 14:58:40 +0400 Subject: [PATCH 15/58] 204: Add simpleTest2 --- .../implementations/immutableMap/TrieNode.kt | 3 ++- .../stress/set/PersistentOrderedSetTest.kt | 27 ++++++++++++++----- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/core/commonMain/src/implementations/immutableMap/TrieNode.kt b/core/commonMain/src/implementations/immutableMap/TrieNode.kt index 9fa6898b..12f02cc2 100644 --- a/core/commonMain/src/implementations/immutableMap/TrieNode.kt +++ b/core/commonMain/src/implementations/immutableMap/TrieNode.kt @@ -287,7 +287,8 @@ internal class TrieNode( if (shift > MAX_SHIFT) { // assert(key1 != key2) // when two key hashes are entirely equal: the last level subtrie node stores them just as unordered list - return TrieNode(0, 0, arrayOf(key1, value1, key2, value2), owner) + val node = TrieNode(0, 0, arrayOf(key1, value1, key2, value2), owner) + return node } val setBit1 = indexSegment(keyHash1, shift) diff --git a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt index d6e24042..2cddab51 100644 --- a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt +++ b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt @@ -15,14 +15,29 @@ import kotlin.test.assertEquals class PersistentOrderedSetTest { - @Test - fun simpleTest() { - data class Data(val value: Int, val hash: Int = 1) { - override fun hashCode(): Int { - return hash - } + data class Data(val value: Int) { + override fun hashCode(): Int { + return 1644 } + } + + @Test + fun simpleTest2() { + var map1: PersistentHashMap = persistentHashMapOf(Data(1) to 10) as PersistentHashMap + map1 = map1.put(Data(2), 20) + map1 = map1.put(Data(3), 30) + + var map2: PersistentHashMap = persistentHashMapOf(Data(2) to 20) as PersistentHashMap + map2 = map2.put(Data(1), 10) + println(map1) + println(map2) + + println(map1.equals(map2)) + } + + @Test + fun simpleTest() { var map1: PersistentHashMap = persistentHashMapOf(Data(1) to Data(1)) as PersistentHashMap map1 = map1.put(Data(2), Data(2)) map1 = map1.remove(Data(2)) From 5903400daf7c016157b66919fb1b34701d99de7f Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Fri, 18 Apr 2025 16:12:51 +0400 Subject: [PATCH 16/58] 204: Add Node value promouting --- .../implementations/immutableMap/TrieNode.kt | 21 ++++++++++++------- .../stress/set/PersistentOrderedSetTest.kt | 10 +++++---- 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/core/commonMain/src/implementations/immutableMap/TrieNode.kt b/core/commonMain/src/implementations/immutableMap/TrieNode.kt index 12f02cc2..bc8e0ae7 100644 --- a/core/commonMain/src/implementations/immutableMap/TrieNode.kt +++ b/core/commonMain/src/implementations/immutableMap/TrieNode.kt @@ -201,15 +201,20 @@ internal class TrieNode( } /** The given [newNode] must not be a part of any persistent map instance. */ - private fun mutableUpdateNodeAtIndex(nodeIndex: Int, newNode: TrieNode, owner: MutabilityOwnership): TrieNode { + private fun mutableUpdateNodeAtIndex(nodeIndex: Int, positionMask: Int, newNode: TrieNode, owner: MutabilityOwnership): TrieNode { assert(newNode.ownedBy === owner) // assert(buffer[nodeIndex] !== newNode) - // nodes (including collision nodes) that have only one entry are upped if they have no siblings - if (buffer.size == 1 && newNode.buffer.size == ENTRY_SIZE && newNode.nodeMap == 0) { -// assert(dataMap == 0 && nodeMap xor positionMask == 0) - newNode.dataMap = nodeMap - return newNode + val newNodeBuffer = newNode.buffer + if (newNode.buffer.size == ENTRY_SIZE && newNode.nodeMap == 0) { + if (buffer.size == 1) { + newNode.dataMap = nodeMap + return newNode + } + + val keyIndex = entryKeyIndex(positionMask) + val newBuffer = buffer.replaceNodeWithEntry(nodeIndex, keyIndex, newNodeBuffer[0], newNodeBuffer[1]) + return TrieNode(dataMap xor positionMask, nodeMap xor positionMask, newBuffer) } if (ownedBy === owner) { @@ -717,7 +722,7 @@ internal class TrieNode( if (targetNode === newNode) { return this } - return mutableUpdateNodeAtIndex(nodeIndex, newNode, mutator.ownership) + return mutableUpdateNodeAtIndex(nodeIndex, keyPositionMask, newNode, mutator.ownership) } // key is absent @@ -792,7 +797,7 @@ internal class TrieNode( newNode == null -> mutableRemoveNodeAtIndex(nodeIndex, positionMask, owner) targetNode !== newNode -> - mutableUpdateNodeAtIndex(nodeIndex, newNode, owner) + mutableUpdateNodeAtIndex(nodeIndex, positionMask, newNode, owner) else -> this } diff --git a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt index 2cddab51..8ef4c3fc 100644 --- a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt +++ b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt @@ -17,15 +17,17 @@ class PersistentOrderedSetTest { data class Data(val value: Int) { override fun hashCode(): Int { - return 1644 + return if (value % 2 == 0) 123456789 else -1 } } @Test fun simpleTest2() { - var map1: PersistentHashMap = persistentHashMapOf(Data(1) to 10) as PersistentHashMap + var map1: PersistentHashMap = persistentHashMapOf() as PersistentHashMap +// map1 = map1.put(Data(1), 10) map1 = map1.put(Data(2), 20) - map1 = map1.put(Data(3), 30) +// map1 = map1.put(Data(3), 30) + map1 = map1.put(Data(4), 40) var map2: PersistentHashMap = persistentHashMapOf(Data(2) to 20) as PersistentHashMap map2 = map2.put(Data(1), 10) @@ -70,7 +72,7 @@ class PersistentOrderedSetTest { @Test fun equalsTestPersistentMap() { - val map1 = persistentHashMapOf(-1 to "minus-one", 0 to "zero", 65536 to "power-of-two") as PersistentHashMap + val map1 = persistentHashMapOf(-2 to "minus-one", 0 to "zero", 32 to "power-of-two") as PersistentHashMap val builder = map1.builder() val map11 = builder.build() From 6b3feba23dc769c0134cf9d5c06d22c2a7c423f8 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Fri, 18 Apr 2025 16:22:52 +0400 Subject: [PATCH 17/58] 204: Rollback return in makeNode --- core/commonMain/src/implementations/immutableMap/TrieNode.kt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/core/commonMain/src/implementations/immutableMap/TrieNode.kt b/core/commonMain/src/implementations/immutableMap/TrieNode.kt index bc8e0ae7..b59c4ed1 100644 --- a/core/commonMain/src/implementations/immutableMap/TrieNode.kt +++ b/core/commonMain/src/implementations/immutableMap/TrieNode.kt @@ -292,8 +292,7 @@ internal class TrieNode( if (shift > MAX_SHIFT) { // assert(key1 != key2) // when two key hashes are entirely equal: the last level subtrie node stores them just as unordered list - val node = TrieNode(0, 0, arrayOf(key1, value1, key2, value2), owner) - return node + return TrieNode(0, 0, arrayOf(key1, value1, key2, value2), owner) } val setBit1 = indexSegment(keyHash1, shift) From 217a0aa172362d65abc9a502686aeda6ed55811c Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Fri, 18 Apr 2025 16:31:12 +0400 Subject: [PATCH 18/58] 204: Remove mutableUpdateNodeAtIndex --- .../implementations/immutableMap/TrieNode.kt | 35 ++++--------------- 1 file changed, 7 insertions(+), 28 deletions(-) diff --git a/core/commonMain/src/implementations/immutableMap/TrieNode.kt b/core/commonMain/src/implementations/immutableMap/TrieNode.kt index b59c4ed1..60427fbf 100644 --- a/core/commonMain/src/implementations/immutableMap/TrieNode.kt +++ b/core/commonMain/src/implementations/immutableMap/TrieNode.kt @@ -180,7 +180,7 @@ internal class TrieNode( } /** The given [newNode] must not be a part of any persistent map instance. */ - private fun updateNodeAtIndex(nodeIndex: Int, positionMask: Int, newNode: TrieNode): TrieNode { + private fun updateNodeAtIndex(nodeIndex: Int, positionMask: Int, newNode: TrieNode, owner: MutabilityOwnership? = null): TrieNode { // assert(buffer[nodeIndex] !== newNode) val newNodeBuffer = newNode.buffer if (newNodeBuffer.size == 2 && newNode.nodeMap == 0) { @@ -195,35 +195,14 @@ internal class TrieNode( return TrieNode(dataMap xor positionMask, nodeMap xor positionMask, newBuffer) } - val newBuffer = buffer.copyOf(buffer.size) - newBuffer[nodeIndex] = newNode - return TrieNode(dataMap, nodeMap, newBuffer) - } - - /** The given [newNode] must not be a part of any persistent map instance. */ - private fun mutableUpdateNodeAtIndex(nodeIndex: Int, positionMask: Int, newNode: TrieNode, owner: MutabilityOwnership): TrieNode { - assert(newNode.ownedBy === owner) -// assert(buffer[nodeIndex] !== newNode) - - val newNodeBuffer = newNode.buffer - if (newNode.buffer.size == ENTRY_SIZE && newNode.nodeMap == 0) { - if (buffer.size == 1) { - newNode.dataMap = nodeMap - return newNode - } - - val keyIndex = entryKeyIndex(positionMask) - val newBuffer = buffer.replaceNodeWithEntry(nodeIndex, keyIndex, newNodeBuffer[0], newNodeBuffer[1]) - return TrieNode(dataMap xor positionMask, nodeMap xor positionMask, newBuffer) - } - - if (ownedBy === owner) { + if (owner != null && ownedBy === owner) { buffer[nodeIndex] = newNode return this } - val newBuffer = buffer.copyOf() + + val newBuffer = buffer.copyOf(buffer.size) newBuffer[nodeIndex] = newNode - return TrieNode(dataMap, nodeMap, newBuffer, owner) + return TrieNode(dataMap, nodeMap, newBuffer) } private fun removeNodeAtIndex(nodeIndex: Int, positionMask: Int): TrieNode? { @@ -721,7 +700,7 @@ internal class TrieNode( if (targetNode === newNode) { return this } - return mutableUpdateNodeAtIndex(nodeIndex, keyPositionMask, newNode, mutator.ownership) + return updateNodeAtIndex(nodeIndex, keyPositionMask, newNode, mutator.ownership) } // key is absent @@ -796,7 +775,7 @@ internal class TrieNode( newNode == null -> mutableRemoveNodeAtIndex(nodeIndex, positionMask, owner) targetNode !== newNode -> - mutableUpdateNodeAtIndex(nodeIndex, positionMask, newNode, owner) + updateNodeAtIndex(nodeIndex, positionMask, newNode, owner) else -> this } From 06cd71193c0bb0efdc2cb03aceabffaa4ed10fc0 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Fri, 18 Apr 2025 16:50:21 +0400 Subject: [PATCH 19/58] 204: Add tests --- .../src/stress/map/PersistentHashMapTest.kt | 21 +++++ .../stress/set/PersistentOrderedSetTest.kt | 76 ++----------------- 2 files changed, 29 insertions(+), 68 deletions(-) diff --git a/core/commonTest/src/stress/map/PersistentHashMapTest.kt b/core/commonTest/src/stress/map/PersistentHashMapTest.kt index 6eb0c08a..0849e852 100644 --- a/core/commonTest/src/stress/map/PersistentHashMapTest.kt +++ b/core/commonTest/src/stress/map/PersistentHashMapTest.kt @@ -6,6 +6,7 @@ package tests.stress.map import kotlinx.collections.immutable.PersistentMap +import kotlinx.collections.immutable.implementations.immutableMap.PersistentHashMap import kotlinx.collections.immutable.persistentHashMapOf import tests.NForAlgorithmComplexity import tests.distinctStringValues @@ -351,6 +352,26 @@ class PersistentHashMapTest : ExecutionTimeMeasuringTest() { } } + @Test + fun testValuePromotionAfterMutableRemoving() { + val map1: PersistentHashMap = + persistentHashMapOf(-1 to "a", 0 to "b", 32 to "c") as PersistentHashMap + val builder = map1.builder() + val map2 = builder.build() + + assertTrue(map1.equals(builder)) + assertEquals(map1, map2.toMap()) + assertEquals(map1, map2) + + val map3 = map1.remove(0) + builder.remove(0) + val map4 = builder.build() + + assertTrue(map3.equals(builder)) + assertEquals(map3, map4.toMap()) + assertEquals(map3, map4) + } + private fun testAfterOperation( expected: Map, actual: Map, diff --git a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt index 8ef4c3fc..ea70323e 100644 --- a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt +++ b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt @@ -5,89 +5,29 @@ package tests.stress.set -import kotlinx.collections.immutable.PersistentSet -import kotlinx.collections.immutable.implementations.immutableMap.PersistentHashMap -import kotlinx.collections.immutable.persistentHashMapOf -import kotlinx.collections.immutable.persistentMapOf import kotlinx.collections.immutable.persistentSetOf import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertTrue class PersistentOrderedSetTest { - data class Data(val value: Int) { - override fun hashCode(): Int { - return if (value % 2 == 0) 123456789 else -1 - } - } - - @Test - fun simpleTest2() { - var map1: PersistentHashMap = persistentHashMapOf() as PersistentHashMap -// map1 = map1.put(Data(1), 10) - map1 = map1.put(Data(2), 20) -// map1 = map1.put(Data(3), 30) - map1 = map1.put(Data(4), 40) - - var map2: PersistentHashMap = persistentHashMapOf(Data(2) to 20) as PersistentHashMap - map2 = map2.put(Data(1), 10) - - println(map1) - println(map2) - - println(map1.equals(map2)) - } - - @Test - fun simpleTest() { - var map1: PersistentHashMap = persistentHashMapOf(Data(1) to Data(1)) as PersistentHashMap - map1 = map1.put(Data(2), Data(2)) - map1 = map1.remove(Data(2)) - - var map2: PersistentHashMap = persistentHashMapOf(Data(2) to Data(2)) as PersistentHashMap - map2 = map2.put(Data(1), Data(1)) - map2 = map2.remove(Data(2)) - - println(map1.equals(map2)) - - assertEquals(map1, map2) - } - + /** + * Test from issue: https://github.com/Kotlin/kotlinx.collections.immutable/issues/204 + */ @Test - fun equalsTestFromGitHubIssue() { - val set1: PersistentSet = persistentSetOf(-1, 0, 65536) // test passes with 65535 third parameter + fun testValuePromotionAfterMutableRemoving() { + val set1 = persistentSetOf(-486539264, 16777216, 0, 67108864) val builder = set1.builder() - assertEquals(set1, builder.build().toSet()) + assertTrue(set1.equals(builder)) assertEquals(set1, builder.build()) + assertEquals(set1, builder.build().toSet()) val set2 = set1.remove(0) builder.remove(0) - println("set2.equals(builder.build()): ${set2.equals(builder.build())}") - assertEquals(set2, builder.build().toSet()) assertEquals(set2, builder.build()) } - - @Test - fun equalsTestPersistentMap() { - val map1 = persistentHashMapOf(-2 to "minus-one", 0 to "zero", 32 to "power-of-two") as PersistentHashMap - val builder = map1.builder() - val map11 = builder.build() - - assertEquals(map1, map11.toMap()) - assertEquals(map1, map11) - - val map2 = map1.remove(0) - builder.remove(0) - val map22 = builder.build() - - val a = arrayOf(map1.node, map11.node, map2.node, map22.node) - - println("map2.equals(map22): ${map2.equals(map22)}") - - assertEquals(map2, map22.toMap()) - assertEquals(map2, map22) - } } \ No newline at end of file From ba76ebbf88fac93e2b8f85ff290be2cb208aaba5 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Fri, 18 Apr 2025 16:55:17 +0400 Subject: [PATCH 20/58] 204: Add persistentMapEqualsTest tests --- .../stress/map/PersistentOrderedMapTest.kt | 45 +++++++++---------- 1 file changed, 20 insertions(+), 25 deletions(-) diff --git a/core/commonTest/src/stress/map/PersistentOrderedMapTest.kt b/core/commonTest/src/stress/map/PersistentOrderedMapTest.kt index 49622b4e..df004c25 100644 --- a/core/commonTest/src/stress/map/PersistentOrderedMapTest.kt +++ b/core/commonTest/src/stress/map/PersistentOrderedMapTest.kt @@ -11,32 +11,28 @@ import kotlin.test.assertEquals class PersistentOrderedMapTest { + /** + * Test from issue: https://github.com/Kotlin/kotlinx.collections.immutable/issues/198 + */ @Test - fun equalsTest() { - val expected = persistentMapOf("a" to 1, "b" to 2, "c" to 3) - val actual = persistentMapOf().put("a", 1).put("b", 2).put("c", 3) - assertEquals(expected, actual) - } - - private class ChosenHashCode( - private val hashCode: Int, - private val name: String, - ) { - override fun equals(other: Any?): Boolean { - return other is ChosenHashCode && (other.name == name) - } - - override fun hashCode(): Int { - return hashCode + fun persistentMapEqualsTest() { + class ChosenHashCode( + private val hashCode: Int, + private val name: String, + ) { + override fun equals(other: Any?): Boolean { + return other is ChosenHashCode && (other.name == name) + } + + override fun hashCode(): Int { + return hashCode + } + + override fun toString(): String { + return name + } } - override fun toString(): String { - return name - } - } - - @Test - fun testFromGitHubIssue() { val a = ChosenHashCode(123, "A") val b = ChosenHashCode(123, "B") val c = ChosenHashCode(123, "C") @@ -50,8 +46,7 @@ class PersistentOrderedMapTest { val minusAb = abc.minus(arrayOf(a, b)) val cOnly = persistentMapOf(c to "z") - assertEquals(cOnly.entries, minusAb.entries) - assertEquals(cOnly, minusAb) + assertEquals(minusAb.entries, cOnly.entries) assertEquals(minusAb, cOnly) } } \ No newline at end of file From 3510f7c1b15866935e7dafee44042d39db5fbfb4 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Fri, 18 Apr 2025 16:57:46 +0400 Subject: [PATCH 21/58] 204: Refactoring --- core/commonTest/src/stress/map/PersistentOrderedMapTest.kt | 2 +- core/commonTest/src/stress/set/PersistentOrderedSetTest.kt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/core/commonTest/src/stress/map/PersistentOrderedMapTest.kt b/core/commonTest/src/stress/map/PersistentOrderedMapTest.kt index df004c25..4be9100a 100644 --- a/core/commonTest/src/stress/map/PersistentOrderedMapTest.kt +++ b/core/commonTest/src/stress/map/PersistentOrderedMapTest.kt @@ -15,7 +15,7 @@ class PersistentOrderedMapTest { * Test from issue: https://github.com/Kotlin/kotlinx.collections.immutable/issues/198 */ @Test - fun persistentMapEqualsTest() { + fun equalsTest() { class ChosenHashCode( private val hashCode: Int, private val name: String, diff --git a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt index ea70323e..18f62508 100644 --- a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt +++ b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt @@ -16,7 +16,7 @@ class PersistentOrderedSetTest { * Test from issue: https://github.com/Kotlin/kotlinx.collections.immutable/issues/204 */ @Test - fun testValuePromotionAfterMutableRemoving() { + fun equalsTest() { val set1 = persistentSetOf(-486539264, 16777216, 0, 67108864) val builder = set1.builder() From b2a1e476b2396071d5ff912fa7d94218b13b3ba3 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Fri, 18 Apr 2025 17:03:17 +0400 Subject: [PATCH 22/58] 204: Rename tests --- core/commonTest/src/stress/map/PersistentOrderedMapTest.kt | 2 +- core/commonTest/src/stress/set/PersistentOrderedSetTest.kt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/core/commonTest/src/stress/map/PersistentOrderedMapTest.kt b/core/commonTest/src/stress/map/PersistentOrderedMapTest.kt index 4be9100a..43d4a318 100644 --- a/core/commonTest/src/stress/map/PersistentOrderedMapTest.kt +++ b/core/commonTest/src/stress/map/PersistentOrderedMapTest.kt @@ -15,7 +15,7 @@ class PersistentOrderedMapTest { * Test from issue: https://github.com/Kotlin/kotlinx.collections.immutable/issues/198 */ @Test - fun equalsTest() { + fun persistentMapEqualityAfterRemovingKeysWithHashCodeCollisionsTest() { class ChosenHashCode( private val hashCode: Int, private val name: String, diff --git a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt index 18f62508..3e648667 100644 --- a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt +++ b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt @@ -16,7 +16,7 @@ class PersistentOrderedSetTest { * Test from issue: https://github.com/Kotlin/kotlinx.collections.immutable/issues/204 */ @Test - fun equalsTest() { + fun persistentSetAndBuilderEqualityBeforeAndAfterModificationTest() { val set1 = persistentSetOf(-486539264, 16777216, 0, 67108864) val builder = set1.builder() From 400870dded25375f00c6be957bff3041542af82d Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Sat, 19 Apr 2025 16:24:34 +0400 Subject: [PATCH 23/58] 204: Add testReproduceOverIterationIssue --- .../map/PersistentHashMapBuilderTest.kt | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt index deb20e4b..590be754 100644 --- a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt +++ b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt @@ -6,6 +6,7 @@ package tests.stress.map import kotlinx.collections.immutable.PersistentMap +import kotlinx.collections.immutable.implementations.immutableMap.PersistentHashMap import kotlinx.collections.immutable.persistentHashMapOf import tests.NForAlgorithmComplexity import tests.distinctStringValues @@ -13,6 +14,8 @@ import tests.remove import tests.stress.ExecutionTimeMeasuringTest import tests.stress.IntWrapper import tests.stress.WrapperGenerator +import kotlin.collections.component1 +import kotlin.collections.component2 import kotlin.random.Random import kotlin.test.* @@ -225,6 +228,24 @@ class PersistentHashMapBuilderTest : ExecutionTimeMeasuringTest() { } } + @Test + fun testReproduceOverIterationIssue() { + val map1: PersistentHashMap = + persistentHashMapOf(1 to "a", 2 to "b", 3 to "c", 0 to "y", 32 to "z") as PersistentHashMap + val iterator = map1.builder().entries.iterator() + + val expectedCount = map1.size + var actualCount = 0 + + while (iterator.hasNext()) { + val (key, _) = iterator.next() + if (key == 0) iterator.remove() + actualCount++ + } + + assertEquals(expectedCount, actualCount) + } + @Test fun removeTests() { val builder = persistentHashMapOf().builder() From e58c4d6087bad2f77c83bf45a6f970606a1b7f80 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Sat, 19 Apr 2025 16:25:26 +0400 Subject: [PATCH 24/58] 204: Refactoring --- .../src/stress/map/PersistentHashMapBuilderTest.kt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt index 590be754..0ac47051 100644 --- a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt +++ b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt @@ -230,11 +230,11 @@ class PersistentHashMapBuilderTest : ExecutionTimeMeasuringTest() { @Test fun testReproduceOverIterationIssue() { - val map1: PersistentHashMap = + val map: PersistentHashMap = persistentHashMapOf(1 to "a", 2 to "b", 3 to "c", 0 to "y", 32 to "z") as PersistentHashMap - val iterator = map1.builder().entries.iterator() + val iterator = map.builder().entries.iterator() - val expectedCount = map1.size + val expectedCount = map.size var actualCount = 0 while (iterator.hasNext()) { From 95c399eb938afb604c315c94e92765198f274303 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Sun, 20 Apr 2025 14:25:33 +0400 Subject: [PATCH 25/58] 204: Refactor testReproduceOverIterationIssue --- core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt index 0ac47051..7c288f66 100644 --- a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt +++ b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt @@ -232,7 +232,8 @@ class PersistentHashMapBuilderTest : ExecutionTimeMeasuringTest() { fun testReproduceOverIterationIssue() { val map: PersistentHashMap = persistentHashMapOf(1 to "a", 2 to "b", 3 to "c", 0 to "y", 32 to "z") as PersistentHashMap - val iterator = map.builder().entries.iterator() + val builder = map.builder() + val iterator = builder.entries.iterator() val expectedCount = map.size var actualCount = 0 From 7a609f6fc796aeafe4fc8e8d673ba9c14db11d85 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Sun, 20 Apr 2025 14:40:01 +0400 Subject: [PATCH 26/58] 204: Add iteratorRemoveCalledTwiceThrowsIllegalStateExceptionTest --- .../map/PersistentHashMapBuilderTest.kt | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt index 7c288f66..aca4657e 100644 --- a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt +++ b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt @@ -247,6 +247,25 @@ class PersistentHashMapBuilderTest : ExecutionTimeMeasuringTest() { assertEquals(expectedCount, actualCount) } + @Test + fun iteratorRemoveCalledTwiceThrowsIllegalStateExceptionTest() { + val map: PersistentHashMap = + persistentHashMapOf(1 to "a", 2 to "b", 3 to "c", 0 to "y", 32 to "z") as PersistentHashMap + val builder = map.builder() + val iterator = builder.entries.iterator() + + assertFailsWith { + while (iterator.hasNext()) { + val (key, _) = iterator.next() + if (key == 0) iterator.remove() + if (key == 0) { + iterator.remove() + iterator.remove() + } + } + } + } + @Test fun removeTests() { val builder = persistentHashMapOf().builder() From 7301ded0626a170a11ad2040e8f441fa7173256c Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Sun, 20 Apr 2025 14:47:21 +0400 Subject: [PATCH 27/58] 204: Add removing elements from different iterators throws ConcurrentModificationException test --- .../map/PersistentHashMapBuilderTest.kt | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt index aca4657e..c3cdac59 100644 --- a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt +++ b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt @@ -248,7 +248,7 @@ class PersistentHashMapBuilderTest : ExecutionTimeMeasuringTest() { } @Test - fun iteratorRemoveCalledTwiceThrowsIllegalStateExceptionTest() { + fun `removing twice on iterators throws IllegalStateException`() { val map: PersistentHashMap = persistentHashMapOf(1 to "a", 2 to "b", 3 to "c", 0 to "y", 32 to "z") as PersistentHashMap val builder = map.builder() @@ -266,6 +266,24 @@ class PersistentHashMapBuilderTest : ExecutionTimeMeasuringTest() { } } + @Test + fun `removing elements from different iterators throws ConcurrentModificationException`() { + val map: PersistentHashMap = + persistentHashMapOf(1 to "a", 2 to "b", 3 to "c", 0 to "y", 32 to "z") as PersistentHashMap + val builder = map.builder() + val iterator1 = builder.entries.iterator() + val iterator2 = builder.entries.iterator() + + assertFailsWith { + while (iterator1.hasNext()) { + val (key, _) = iterator1.next() + iterator2.next() + if (key == 0) iterator1.remove() + if (key == 2) iterator2.remove() + } + } + } + @Test fun removeTests() { val builder = persistentHashMapOf().builder() From 810ebb7507ad6372345d479451781824f245d737 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Sun, 20 Apr 2025 15:00:42 +0400 Subject: [PATCH 28/58] 204: Refactor testReproduceOverIterationIssue --- .../commonTest/src/stress/map/PersistentHashMapBuilderTest.kt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt index c3cdac59..fa082684 100644 --- a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt +++ b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt @@ -240,7 +240,9 @@ class PersistentHashMapBuilderTest : ExecutionTimeMeasuringTest() { while (iterator.hasNext()) { val (key, _) = iterator.next() - if (key == 0) iterator.remove() + if (key == 0) { + iterator.remove() + } actualCount++ } From 0612c43992085d185ccc44fa99aba9aaf4737a9f Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Sun, 20 Apr 2025 18:43:23 +0400 Subject: [PATCH 29/58] 204: Add promotion check to resetPath --- .../PersistentHashMapBuilderContentIterators.kt | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/core/commonMain/src/implementations/immutableMap/PersistentHashMapBuilderContentIterators.kt b/core/commonMain/src/implementations/immutableMap/PersistentHashMapBuilderContentIterators.kt index 986369ca..b618092f 100644 --- a/core/commonMain/src/implementations/immutableMap/PersistentHashMapBuilderContentIterators.kt +++ b/core/commonMain/src/implementations/immutableMap/PersistentHashMapBuilderContentIterators.kt @@ -57,7 +57,7 @@ internal open class PersistentHashMapBuilderBaseIterator( val currentKey = currentKey() builder.remove(lastIteratedKey) - resetPath(currentKey.hashCode(), builder.node, currentKey, 0) + resetPath(currentKey.hashCode(), builder.node, currentKey, 0, lastIteratedKey.hashCode()) } else { builder.remove(lastIteratedKey) } @@ -82,7 +82,7 @@ internal open class PersistentHashMapBuilderBaseIterator( expectedModCount = builder.modCount } - private fun resetPath(keyHash: Int, node: TrieNode<*, *>, key: K, pathIndex: Int) { + private fun resetPath(keyHash: Int, node: TrieNode<*, *>, key: K, pathIndex: Int, removedKeyHash: Int? = null) { val shift = pathIndex * LOG_MAX_BRANCHING_FACTOR if (shift > MAX_SHIFT) { // collision @@ -99,6 +99,12 @@ internal open class PersistentHashMapBuilderBaseIterator( if (node.hasEntryAt(keyPositionMask)) { // key is directly in buffer val keyIndex = node.entryKeyIndex(keyPositionMask) + val removedKeyPositionMask = removedKeyHash?.let { 1 shl indexSegment(it, shift) } ?: 0 + + if (keyPositionMask == removedKeyPositionMask) { + println("WAS PROMOTION!") + } + // assert(node.keyAtIndex(keyIndex) == key) path[pathIndex].reset(node.buffer, ENTRY_SIZE * node.entryCount(), keyIndex) From 4efe7be1ee07c4c611a2a4d2aadab71c8b547825 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Sun, 20 Apr 2025 20:03:53 +0400 Subject: [PATCH 30/58] 204: testReproduceOverIterationIssue passes --- .../immutableMap/PersistentHashMapBuilderContentIterators.kt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/core/commonMain/src/implementations/immutableMap/PersistentHashMapBuilderContentIterators.kt b/core/commonMain/src/implementations/immutableMap/PersistentHashMapBuilderContentIterators.kt index b618092f..d2d07d70 100644 --- a/core/commonMain/src/implementations/immutableMap/PersistentHashMapBuilderContentIterators.kt +++ b/core/commonMain/src/implementations/immutableMap/PersistentHashMapBuilderContentIterators.kt @@ -99,10 +99,13 @@ internal open class PersistentHashMapBuilderBaseIterator( if (node.hasEntryAt(keyPositionMask)) { // key is directly in buffer val keyIndex = node.entryKeyIndex(keyPositionMask) - val removedKeyPositionMask = removedKeyHash?.let { 1 shl indexSegment(it, shift) } ?: 0 + val removedKeyPositionMask = removedKeyHash?.let { 1 shl indexSegment(it, shift) } if (keyPositionMask == removedKeyPositionMask) { println("WAS PROMOTION!") + path[pathIndex + 1].reset(arrayOf(node.buffer[keyIndex], node.buffer[keyIndex + 1]), ENTRY_SIZE, 0) + pathLastIndex = pathIndex + 1 + return } // assert(node.keyAtIndex(keyIndex) == key) From dade1a3ac2161be0de43e62c15f541336df49b5f Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Sun, 20 Apr 2025 20:34:01 +0400 Subject: [PATCH 31/58] 204: Add testReproduceOverIterationIssue2 --- core/commonTest/src/stress/ObjectWrapper.kt | 2 +- .../map/PersistentHashMapBuilderTest.kt | 36 +++++++++++++++++-- 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/core/commonTest/src/stress/ObjectWrapper.kt b/core/commonTest/src/stress/ObjectWrapper.kt index c217de6a..48a1b962 100644 --- a/core/commonTest/src/stress/ObjectWrapper.kt +++ b/core/commonTest/src/stress/ObjectWrapper.kt @@ -10,7 +10,7 @@ import kotlin.js.JsName class ObjectWrapper>( val obj: K, - @JsName("_hashCode") val hashCode: Int + @JsName("_hashCode") val hashCode: Int = obj.hashCode() ) : Comparable> { override fun hashCode(): Int { return hashCode diff --git a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt index fa082684..75229b59 100644 --- a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt +++ b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt @@ -249,10 +249,42 @@ class PersistentHashMapBuilderTest : ExecutionTimeMeasuringTest() { assertEquals(expectedCount, actualCount) } + @Test + fun testReproduceOverIterationIssue2() { + val zeroKey = IntWrapper(0, 0) + + val map: PersistentHashMap = persistentHashMapOf( + zeroKey to "a", + IntWrapper(1, 0) to "b", + IntWrapper(2, 32) to "c", + IntWrapper(3, 32) to "d", + IntWrapper(4, 64) to "e", + IntWrapper(5, 64) to "f", + IntWrapper(6) to "g", + IntWrapper(7) to "h", + ) as PersistentHashMap + + val builder = map.builder() + val iterator = builder.entries.iterator() + + val expectedCount = map.size + var actualCount = 0 + + while (iterator.hasNext()) { + val (key, _) = iterator.next() + if (key == zeroKey) { + iterator.remove() + } + actualCount++ + } + + assertEquals(expectedCount, actualCount) + } + @Test fun `removing twice on iterators throws IllegalStateException`() { val map: PersistentHashMap = - persistentHashMapOf(1 to "a", 2 to "b", 3 to "c", 0 to "y", 32 to "z") as PersistentHashMap + persistentHashMapOf(1 to "a", 2 to "b", 3 to "c", 0 to "y", 32 to "z") as PersistentHashMap val builder = map.builder() val iterator = builder.entries.iterator() @@ -271,7 +303,7 @@ class PersistentHashMapBuilderTest : ExecutionTimeMeasuringTest() { @Test fun `removing elements from different iterators throws ConcurrentModificationException`() { val map: PersistentHashMap = - persistentHashMapOf(1 to "a", 2 to "b", 3 to "c", 0 to "y", 32 to "z") as PersistentHashMap + persistentHashMapOf(1 to "a", 2 to "b", 3 to "c", 0 to "y", 32 to "z") as PersistentHashMap val builder = map.builder() val iterator1 = builder.entries.iterator() val iterator2 = builder.entries.iterator() From df1f188d218f9459c0a2d48d5c7bfb258b1b77f8 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Sun, 20 Apr 2025 20:35:40 +0400 Subject: [PATCH 32/58] 204: Refactor testReproduceOverIterationIssue2 --- core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt index 75229b59..4e02cb19 100644 --- a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt +++ b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt @@ -251,7 +251,7 @@ class PersistentHashMapBuilderTest : ExecutionTimeMeasuringTest() { @Test fun testReproduceOverIterationIssue2() { - val zeroKey = IntWrapper(0, 0) + val zeroKey = IntWrapper(0) val map: PersistentHashMap = persistentHashMapOf( zeroKey to "a", From 9e86a7451138d0d47e55f915c191c80873b7c817 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Sun, 20 Apr 2025 20:40:07 +0400 Subject: [PATCH 33/58] 204: Simplify testReproduceOverIterationIssue2 --- .../src/stress/map/PersistentHashMapBuilderTest.kt | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt index 4e02cb19..b677c140 100644 --- a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt +++ b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt @@ -257,11 +257,7 @@ class PersistentHashMapBuilderTest : ExecutionTimeMeasuringTest() { zeroKey to "a", IntWrapper(1, 0) to "b", IntWrapper(2, 32) to "c", - IntWrapper(3, 32) to "d", - IntWrapper(4, 64) to "e", - IntWrapper(5, 64) to "f", - IntWrapper(6) to "g", - IntWrapper(7) to "h", + IntWrapper(3, 32) to "d" ) as PersistentHashMap val builder = map.builder() From 42cf3127e964ed719c8a23d2e8d87aec76a323dd Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Sun, 20 Apr 2025 21:17:26 +0400 Subject: [PATCH 34/58] 204: testReproduceOverIterationIssue2 passes --- .../immutableMap/PersistentHashMapBuilderContentIterators.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/commonMain/src/implementations/immutableMap/PersistentHashMapBuilderContentIterators.kt b/core/commonMain/src/implementations/immutableMap/PersistentHashMapBuilderContentIterators.kt index d2d07d70..4d65b292 100644 --- a/core/commonMain/src/implementations/immutableMap/PersistentHashMapBuilderContentIterators.kt +++ b/core/commonMain/src/implementations/immutableMap/PersistentHashMapBuilderContentIterators.kt @@ -120,7 +120,7 @@ internal open class PersistentHashMapBuilderBaseIterator( val nodeIndex = node.nodeIndex(keyPositionMask) val targetNode = node.nodeAtIndex(nodeIndex) path[pathIndex].reset(node.buffer, ENTRY_SIZE * node.entryCount(), nodeIndex) - resetPath(keyHash, targetNode, key, pathIndex + 1) + resetPath(keyHash, targetNode, key, pathIndex + 1, removedKeyHash) } private fun checkNextWasInvoked() { From b93994c467e355aa1304274cf59c49ccc13774e4 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Sun, 20 Apr 2025 23:14:59 +0400 Subject: [PATCH 35/58] 204: Fix all iterator tests! --- .../PersistentHashMapBuilderContentIterators.kt | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/core/commonMain/src/implementations/immutableMap/PersistentHashMapBuilderContentIterators.kt b/core/commonMain/src/implementations/immutableMap/PersistentHashMapBuilderContentIterators.kt index 4d65b292..e55c7ca5 100644 --- a/core/commonMain/src/implementations/immutableMap/PersistentHashMapBuilderContentIterators.kt +++ b/core/commonMain/src/implementations/immutableMap/PersistentHashMapBuilderContentIterators.kt @@ -101,10 +101,8 @@ internal open class PersistentHashMapBuilderBaseIterator( val removedKeyPositionMask = removedKeyHash?.let { 1 shl indexSegment(it, shift) } - if (keyPositionMask == removedKeyPositionMask) { - println("WAS PROMOTION!") - path[pathIndex + 1].reset(arrayOf(node.buffer[keyIndex], node.buffer[keyIndex + 1]), ENTRY_SIZE, 0) - pathLastIndex = pathIndex + 1 + if (keyPositionMask == removedKeyPositionMask && pathIndex < pathLastIndex) { + path[pathLastIndex].reset(arrayOf(node.buffer[keyIndex], node.buffer[keyIndex + 1]), ENTRY_SIZE, 0) return } From 4f9c54b58f0420ad142946784c616fdf79a02289 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Sun, 20 Apr 2025 23:42:10 +0400 Subject: [PATCH 36/58] 204: Remove ` char from test names --- .../commonTest/src/stress/map/PersistentHashMapBuilderTest.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt index b677c140..ecea49b4 100644 --- a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt +++ b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt @@ -278,7 +278,7 @@ class PersistentHashMapBuilderTest : ExecutionTimeMeasuringTest() { } @Test - fun `removing twice on iterators throws IllegalStateException`() { + fun removingTwiceOnIteratorsThrowsIllegalStateException() { val map: PersistentHashMap = persistentHashMapOf(1 to "a", 2 to "b", 3 to "c", 0 to "y", 32 to "z") as PersistentHashMap val builder = map.builder() @@ -297,7 +297,7 @@ class PersistentHashMapBuilderTest : ExecutionTimeMeasuringTest() { } @Test - fun `removing elements from different iterators throws ConcurrentModificationException`() { + fun removingElementsFromDifferentIteratorsThrowsConcurrentModificationException() { val map: PersistentHashMap = persistentHashMapOf(1 to "a", 2 to "b", 3 to "c", 0 to "y", 32 to "z") as PersistentHashMap val builder = map.builder() From 5bcb1564d0cb7484dca6501dd19a4b09a25d0d5a Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Sun, 20 Apr 2025 23:55:17 +0400 Subject: [PATCH 37/58] 204: Rollback default field in ObjectWrapper --- core/commonTest/src/stress/ObjectWrapper.kt | 2 +- core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/core/commonTest/src/stress/ObjectWrapper.kt b/core/commonTest/src/stress/ObjectWrapper.kt index 48a1b962..c217de6a 100644 --- a/core/commonTest/src/stress/ObjectWrapper.kt +++ b/core/commonTest/src/stress/ObjectWrapper.kt @@ -10,7 +10,7 @@ import kotlin.js.JsName class ObjectWrapper>( val obj: K, - @JsName("_hashCode") val hashCode: Int = obj.hashCode() + @JsName("_hashCode") val hashCode: Int ) : Comparable> { override fun hashCode(): Int { return hashCode diff --git a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt index ecea49b4..fb437b7c 100644 --- a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt +++ b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt @@ -251,7 +251,7 @@ class PersistentHashMapBuilderTest : ExecutionTimeMeasuringTest() { @Test fun testReproduceOverIterationIssue2() { - val zeroKey = IntWrapper(0) + val zeroKey = IntWrapper(0, 0) val map: PersistentHashMap = persistentHashMapOf( zeroKey to "a", From b1a5b24c6c6f949b474308ceb4f20282e297d7d3 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Mon, 21 Apr 2025 00:00:23 +0400 Subject: [PATCH 38/58] 204: Pass an owner in updateNodeAtIndex --- .../commonMain/src/implementations/immutableMap/TrieNode.kt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/commonMain/src/implementations/immutableMap/TrieNode.kt b/core/commonMain/src/implementations/immutableMap/TrieNode.kt index 60427fbf..e00eaa7a 100644 --- a/core/commonMain/src/implementations/immutableMap/TrieNode.kt +++ b/core/commonMain/src/implementations/immutableMap/TrieNode.kt @@ -192,7 +192,7 @@ internal class TrieNode( val keyIndex = entryKeyIndex(positionMask) val newBuffer = buffer.replaceNodeWithEntry(nodeIndex, keyIndex, newNodeBuffer[0], newNodeBuffer[1]) - return TrieNode(dataMap xor positionMask, nodeMap xor positionMask, newBuffer) + return TrieNode(dataMap xor positionMask, nodeMap xor positionMask, newBuffer, owner) } if (owner != null && ownedBy === owner) { @@ -200,9 +200,9 @@ internal class TrieNode( return this } - val newBuffer = buffer.copyOf(buffer.size) + val newBuffer = buffer.copyOf() newBuffer[nodeIndex] = newNode - return TrieNode(dataMap, nodeMap, newBuffer) + return TrieNode(dataMap, nodeMap, newBuffer, owner) } private fun removeNodeAtIndex(nodeIndex: Int, positionMask: Int): TrieNode? { From 97256e50a7f67fcb545f55dfd27b09dc51fd511c Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Mon, 21 Apr 2025 00:00:39 +0400 Subject: [PATCH 39/58] 204: Remove default 0 index --- .../immutableMap/PersistentHashMapBuilderContentIterators.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/commonMain/src/implementations/immutableMap/PersistentHashMapBuilderContentIterators.kt b/core/commonMain/src/implementations/immutableMap/PersistentHashMapBuilderContentIterators.kt index e55c7ca5..921c3626 100644 --- a/core/commonMain/src/implementations/immutableMap/PersistentHashMapBuilderContentIterators.kt +++ b/core/commonMain/src/implementations/immutableMap/PersistentHashMapBuilderContentIterators.kt @@ -102,7 +102,7 @@ internal open class PersistentHashMapBuilderBaseIterator( val removedKeyPositionMask = removedKeyHash?.let { 1 shl indexSegment(it, shift) } if (keyPositionMask == removedKeyPositionMask && pathIndex < pathLastIndex) { - path[pathLastIndex].reset(arrayOf(node.buffer[keyIndex], node.buffer[keyIndex + 1]), ENTRY_SIZE, 0) + path[pathLastIndex].reset(arrayOf(node.buffer[keyIndex], node.buffer[keyIndex + 1]), ENTRY_SIZE) return } From fdaa0f74f4374d6b73557602b17ebceadefee427 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Mon, 21 Apr 2025 00:04:20 +0400 Subject: [PATCH 40/58] 204: Remove component1, component2 imports --- core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt | 2 -- 1 file changed, 2 deletions(-) diff --git a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt index fb437b7c..f3fd09c5 100644 --- a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt +++ b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt @@ -14,8 +14,6 @@ import tests.remove import tests.stress.ExecutionTimeMeasuringTest import tests.stress.IntWrapper import tests.stress.WrapperGenerator -import kotlin.collections.component1 -import kotlin.collections.component2 import kotlin.random.Random import kotlin.test.* From 112cf9ae33767f06f491e56b3b3eaf1a2d618128 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Mon, 21 Apr 2025 10:29:02 +0400 Subject: [PATCH 41/58] 204: Add removingElementFromOneIteratorAndAccessingAnotherThrowsConcurrentModificationException test --- .../src/stress/map/PersistentHashMapBuilderTest.kt | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt index f3fd09c5..7af9dcd6 100644 --- a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt +++ b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt @@ -312,6 +312,20 @@ class PersistentHashMapBuilderTest : ExecutionTimeMeasuringTest() { } } + @Test + fun removingElementFromOneIteratorAndAccessingAnotherThrowsConcurrentModificationException() { + val map = persistentHashMapOf(1 to "a", 2 to "b", 3 to "c") + val builder = map.builder() + val iterator1 = builder.entries.iterator() + val iterator2 = builder.entries.iterator() + + assertFailsWith { + iterator1.next() + iterator1.remove() + iterator2.next() + } + } + @Test fun removeTests() { val builder = persistentHashMapOf().builder() From 4971505724f549d7e65556fb38b1384ad859af60 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Mon, 21 Apr 2025 10:32:48 +0400 Subject: [PATCH 42/58] 204: Rename tests --- .../commonTest/src/stress/map/PersistentHashMapBuilderTest.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt index 7af9dcd6..c0a0f2b0 100644 --- a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt +++ b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt @@ -227,7 +227,7 @@ class PersistentHashMapBuilderTest : ExecutionTimeMeasuringTest() { } @Test - fun testReproduceOverIterationIssue() { + fun iterationsAfterPromotionTest() { val map: PersistentHashMap = persistentHashMapOf(1 to "a", 2 to "b", 3 to "c", 0 to "y", 32 to "z") as PersistentHashMap val builder = map.builder() @@ -248,7 +248,7 @@ class PersistentHashMapBuilderTest : ExecutionTimeMeasuringTest() { } @Test - fun testReproduceOverIterationIssue2() { + fun iterationsAfterPromotionWithIntWrapperTest() { val zeroKey = IntWrapper(0, 0) val map: PersistentHashMap = persistentHashMapOf( From 94c33a3d0d9fdef097ad6688e74be9c03d9d223f Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Mon, 21 Apr 2025 10:35:14 +0400 Subject: [PATCH 43/58] 204: Rename valuePromotionAfterMutableRemovingTest test --- core/commonTest/src/stress/map/PersistentHashMapTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/commonTest/src/stress/map/PersistentHashMapTest.kt b/core/commonTest/src/stress/map/PersistentHashMapTest.kt index 0849e852..d8fe184d 100644 --- a/core/commonTest/src/stress/map/PersistentHashMapTest.kt +++ b/core/commonTest/src/stress/map/PersistentHashMapTest.kt @@ -353,7 +353,7 @@ class PersistentHashMapTest : ExecutionTimeMeasuringTest() { } @Test - fun testValuePromotionAfterMutableRemoving() { + fun valuePromotionAfterMutableRemovingTest() { val map1: PersistentHashMap = persistentHashMapOf(-1 to "a", 0 to "b", 32 to "c") as PersistentHashMap val builder = map1.builder() From f082a928eeaf27e4b7867a2f7baec7981f6fe5f2 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Tue, 22 Apr 2025 11:41:51 +0400 Subject: [PATCH 44/58] 204: Rename tests with `` chars --- .../src/stress/map/PersistentHashMapBuilderTest.kt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt index c0a0f2b0..117f8d8f 100644 --- a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt +++ b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt @@ -276,7 +276,7 @@ class PersistentHashMapBuilderTest : ExecutionTimeMeasuringTest() { } @Test - fun removingTwiceOnIteratorsThrowsIllegalStateException() { + fun `removing twice on iterators throws IllegalStateException`() { val map: PersistentHashMap = persistentHashMapOf(1 to "a", 2 to "b", 3 to "c", 0 to "y", 32 to "z") as PersistentHashMap val builder = map.builder() @@ -295,7 +295,7 @@ class PersistentHashMapBuilderTest : ExecutionTimeMeasuringTest() { } @Test - fun removingElementsFromDifferentIteratorsThrowsConcurrentModificationException() { + fun `removing elements from different iterators throws ConcurrentModificationException`() { val map: PersistentHashMap = persistentHashMapOf(1 to "a", 2 to "b", 3 to "c", 0 to "y", 32 to "z") as PersistentHashMap val builder = map.builder() @@ -313,7 +313,7 @@ class PersistentHashMapBuilderTest : ExecutionTimeMeasuringTest() { } @Test - fun removingElementFromOneIteratorAndAccessingAnotherThrowsConcurrentModificationException() { + fun `removing element from one iterator and accessing another throws ConcurrentModificationException`() { val map = persistentHashMapOf(1 to "a", 2 to "b", 3 to "c") val builder = map.builder() val iterator1 = builder.entries.iterator() From 97350569821d17c2aa2b4f4c09397c1a85c28bb1 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Fri, 25 Apr 2025 16:41:32 +0400 Subject: [PATCH 45/58] 204: Remove PersistentOrderedMapTest --- .../stress/map/PersistentOrderedMapTest.kt | 52 ------------------- 1 file changed, 52 deletions(-) delete mode 100644 core/commonTest/src/stress/map/PersistentOrderedMapTest.kt diff --git a/core/commonTest/src/stress/map/PersistentOrderedMapTest.kt b/core/commonTest/src/stress/map/PersistentOrderedMapTest.kt deleted file mode 100644 index 43d4a318..00000000 --- a/core/commonTest/src/stress/map/PersistentOrderedMapTest.kt +++ /dev/null @@ -1,52 +0,0 @@ -/* - * Copyright 2016-2025 JetBrains s.r.o. - * Use of this source code is governed by the Apache 2.0 License that can be found in the LICENSE.txt file. - */ - -package tests.stress.map - -import kotlinx.collections.immutable.persistentMapOf -import kotlin.test.Test -import kotlin.test.assertEquals - -class PersistentOrderedMapTest { - - /** - * Test from issue: https://github.com/Kotlin/kotlinx.collections.immutable/issues/198 - */ - @Test - fun persistentMapEqualityAfterRemovingKeysWithHashCodeCollisionsTest() { - class ChosenHashCode( - private val hashCode: Int, - private val name: String, - ) { - override fun equals(other: Any?): Boolean { - return other is ChosenHashCode && (other.name == name) - } - - override fun hashCode(): Int { - return hashCode - } - - override fun toString(): String { - return name - } - } - - val a = ChosenHashCode(123, "A") - val b = ChosenHashCode(123, "B") - val c = ChosenHashCode(123, "C") - - val abc = persistentMapOf( - a to "x", - b to "y", - c to "z", - ) - - val minusAb = abc.minus(arrayOf(a, b)) - val cOnly = persistentMapOf(c to "z") - - assertEquals(minusAb.entries, cOnly.entries) - assertEquals(minusAb, cOnly) - } -} \ No newline at end of file From 21b6945a63daa88d8dfb579c06721623890841fb Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Fri, 25 Apr 2025 16:52:57 +0400 Subject: [PATCH 46/58] 204: Avoid Int boxing --- .../PersistentHashMapBuilderContentIterators.kt | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/commonMain/src/implementations/immutableMap/PersistentHashMapBuilderContentIterators.kt b/core/commonMain/src/implementations/immutableMap/PersistentHashMapBuilderContentIterators.kt index 921c3626..6915fba6 100644 --- a/core/commonMain/src/implementations/immutableMap/PersistentHashMapBuilderContentIterators.kt +++ b/core/commonMain/src/implementations/immutableMap/PersistentHashMapBuilderContentIterators.kt @@ -57,7 +57,7 @@ internal open class PersistentHashMapBuilderBaseIterator( val currentKey = currentKey() builder.remove(lastIteratedKey) - resetPath(currentKey.hashCode(), builder.node, currentKey, 0, lastIteratedKey.hashCode()) + resetPath(currentKey.hashCode(), builder.node, currentKey, 0, lastIteratedKey.hashCode(), afterRemove = true) } else { builder.remove(lastIteratedKey) } @@ -82,7 +82,7 @@ internal open class PersistentHashMapBuilderBaseIterator( expectedModCount = builder.modCount } - private fun resetPath(keyHash: Int, node: TrieNode<*, *>, key: K, pathIndex: Int, removedKeyHash: Int? = null) { + private fun resetPath(keyHash: Int, node: TrieNode<*, *>, key: K, pathIndex: Int, removedKeyHash: Int = 0, afterRemove: Boolean = false) { val shift = pathIndex * LOG_MAX_BRANCHING_FACTOR if (shift > MAX_SHIFT) { // collision @@ -99,7 +99,7 @@ internal open class PersistentHashMapBuilderBaseIterator( if (node.hasEntryAt(keyPositionMask)) { // key is directly in buffer val keyIndex = node.entryKeyIndex(keyPositionMask) - val removedKeyPositionMask = removedKeyHash?.let { 1 shl indexSegment(it, shift) } + val removedKeyPositionMask = if (afterRemove) 1 shl indexSegment(removedKeyHash, shift) else 0 if (keyPositionMask == removedKeyPositionMask && pathIndex < pathLastIndex) { path[pathLastIndex].reset(arrayOf(node.buffer[keyIndex], node.buffer[keyIndex + 1]), ENTRY_SIZE) @@ -118,7 +118,7 @@ internal open class PersistentHashMapBuilderBaseIterator( val nodeIndex = node.nodeIndex(keyPositionMask) val targetNode = node.nodeAtIndex(nodeIndex) path[pathIndex].reset(node.buffer, ENTRY_SIZE * node.entryCount(), nodeIndex) - resetPath(keyHash, targetNode, key, pathIndex + 1, removedKeyHash) + resetPath(keyHash, targetNode, key, pathIndex + 1, removedKeyHash, afterRemove) } private fun checkNextWasInvoked() { From 84d70214b5a23bd5c135256df15fde5ceefc844b Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Fri, 25 Apr 2025 17:04:34 +0400 Subject: [PATCH 47/58] 204: Add content check to iterationsAfterPromotionTest --- .../stress/map/PersistentHashMapBuilderTest.kt | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt index 117f8d8f..e34c1033 100644 --- a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt +++ b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt @@ -228,8 +228,9 @@ class PersistentHashMapBuilderTest : ExecutionTimeMeasuringTest() { @Test fun iterationsAfterPromotionTest() { + val removedKey = 0 val map: PersistentHashMap = - persistentHashMapOf(1 to "a", 2 to "b", 3 to "c", 0 to "y", 32 to "z") as PersistentHashMap + persistentHashMapOf(1 to "a", 2 to "b", 3 to "c", removedKey to "y", 32 to "z") as PersistentHashMap val builder = map.builder() val iterator = builder.entries.iterator() @@ -238,12 +239,22 @@ class PersistentHashMapBuilderTest : ExecutionTimeMeasuringTest() { while (iterator.hasNext()) { val (key, _) = iterator.next() - if (key == 0) { + if (key == removedKey) { iterator.remove() } actualCount++ } + val resultMap = builder.build() + for ((key, value) in map) { + if (key != removedKey) { + assertTrue(key in resultMap) + assertEquals(resultMap[key], value) + } else { + assertFalse(key in resultMap) + } + } + assertEquals(expectedCount, actualCount) } From 110cb36fa96e997dc6a2210be2966a98a0e451fa Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Fri, 25 Apr 2025 17:10:53 +0400 Subject: [PATCH 48/58] 204: Add validatePromotion call to iterationsAfterPromotionTest and iterationsAfterPromotionWithIntWrapperTest --- .../map/PersistentHashMapBuilderTest.kt | 49 ++++++++----------- 1 file changed, 20 insertions(+), 29 deletions(-) diff --git a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt index e34c1033..65360c76 100644 --- a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt +++ b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt @@ -230,45 +230,26 @@ class PersistentHashMapBuilderTest : ExecutionTimeMeasuringTest() { fun iterationsAfterPromotionTest() { val removedKey = 0 val map: PersistentHashMap = - persistentHashMapOf(1 to "a", 2 to "b", 3 to "c", removedKey to "y", 32 to "z") as PersistentHashMap - val builder = map.builder() - val iterator = builder.entries.iterator() - - val expectedCount = map.size - var actualCount = 0 - - while (iterator.hasNext()) { - val (key, _) = iterator.next() - if (key == removedKey) { - iterator.remove() - } - actualCount++ - } - - val resultMap = builder.build() - for ((key, value) in map) { - if (key != removedKey) { - assertTrue(key in resultMap) - assertEquals(resultMap[key], value) - } else { - assertFalse(key in resultMap) - } - } + persistentHashMapOf(1 to "a", 2 to "b", 3 to "c", removedKey to "y", 32 to "z") + as PersistentHashMap - assertEquals(expectedCount, actualCount) + validatePromotion(map, removedKey) } @Test fun iterationsAfterPromotionWithIntWrapperTest() { - val zeroKey = IntWrapper(0, 0) - + val removedKey = IntWrapper(0, 0) val map: PersistentHashMap = persistentHashMapOf( - zeroKey to "a", + removedKey to "a", IntWrapper(1, 0) to "b", IntWrapper(2, 32) to "c", IntWrapper(3, 32) to "d" ) as PersistentHashMap + validatePromotion(map, removedKey) + } + + private fun validatePromotion(map: PersistentHashMap, removedKey: K) { val builder = map.builder() val iterator = builder.entries.iterator() @@ -277,12 +258,22 @@ class PersistentHashMapBuilderTest : ExecutionTimeMeasuringTest() { while (iterator.hasNext()) { val (key, _) = iterator.next() - if (key == zeroKey) { + if (key == removedKey) { iterator.remove() } actualCount++ } + val resultMap = builder.build() + for ((key, value) in map) { + if (key != removedKey) { + assertTrue(key in resultMap) + assertEquals(resultMap[key], value) + } else { + assertFalse(key in resultMap) + } + } + assertEquals(expectedCount, actualCount) } From 47732264bb288189c4b54f6ccab5fadd67a71ff9 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Fri, 25 Apr 2025 18:04:50 +0400 Subject: [PATCH 49/58] 204: Rename tests --- .../commonTest/src/stress/map/PersistentHashMapBuilderTest.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt index 65360c76..83fd8d2f 100644 --- a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt +++ b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt @@ -227,7 +227,7 @@ class PersistentHashMapBuilderTest : ExecutionTimeMeasuringTest() { } @Test - fun iterationsAfterPromotionTest() { + fun `should correctly iterate after removing integer key and promotion colliding key during iteration`() { val removedKey = 0 val map: PersistentHashMap = persistentHashMapOf(1 to "a", 2 to "b", 3 to "c", removedKey to "y", 32 to "z") @@ -237,7 +237,7 @@ class PersistentHashMapBuilderTest : ExecutionTimeMeasuringTest() { } @Test - fun iterationsAfterPromotionWithIntWrapperTest() { + fun `should correctly iterate after removing IntWrapper key and promotion colliding key during iteration`() { val removedKey = IntWrapper(0, 0) val map: PersistentHashMap = persistentHashMapOf( removedKey to "a", From 6e0ebeddb7188d2b6bdde7cf591d1cf7695d1510 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Fri, 25 Apr 2025 18:10:42 +0400 Subject: [PATCH 50/58] 204: Rename tests --- core/commonTest/src/stress/map/PersistentHashMapTest.kt | 2 +- core/commonTest/src/stress/set/PersistentOrderedSetTest.kt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/core/commonTest/src/stress/map/PersistentHashMapTest.kt b/core/commonTest/src/stress/map/PersistentHashMapTest.kt index d8fe184d..0649bbbf 100644 --- a/core/commonTest/src/stress/map/PersistentHashMapTest.kt +++ b/core/commonTest/src/stress/map/PersistentHashMapTest.kt @@ -353,7 +353,7 @@ class PersistentHashMapTest : ExecutionTimeMeasuringTest() { } @Test - fun valuePromotionAfterMutableRemovingTest() { + fun `if the collision is of size 2 and one of the keys is removed, the remaining key must be promoted`() { val map1: PersistentHashMap = persistentHashMapOf(-1 to "a", 0 to "b", 32 to "c") as PersistentHashMap val builder = map1.builder() diff --git a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt index 3e648667..a7d9e014 100644 --- a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt +++ b/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt @@ -16,7 +16,7 @@ class PersistentOrderedSetTest { * Test from issue: https://github.com/Kotlin/kotlinx.collections.immutable/issues/204 */ @Test - fun persistentSetAndBuilderEqualityBeforeAndAfterModificationTest() { + fun `persistentSet and their builder should be equal before and after modification`() { val set1 = persistentSetOf(-486539264, 16777216, 0, 67108864) val builder = set1.builder() From c4cd5f3a4579b206232010b2e468c59a6053f779 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Fri, 25 Apr 2025 18:22:44 +0400 Subject: [PATCH 51/58] 204: Move `if the collision is of size 2 and one of the keys is removed, the remaining key must be promoted` test --- .../src/contract/map/PersistentHashMapTest.kt | 35 +++++++++++++++++++ .../src/stress/map/PersistentHashMapTest.kt | 21 ----------- 2 files changed, 35 insertions(+), 21 deletions(-) create mode 100644 core/commonTest/src/contract/map/PersistentHashMapTest.kt diff --git a/core/commonTest/src/contract/map/PersistentHashMapTest.kt b/core/commonTest/src/contract/map/PersistentHashMapTest.kt new file mode 100644 index 00000000..0c67f790 --- /dev/null +++ b/core/commonTest/src/contract/map/PersistentHashMapTest.kt @@ -0,0 +1,35 @@ +/* + * Copyright 2016-2025 JetBrains s.r.o. + * Use of this source code is governed by the Apache 2.0 License that can be found in the LICENSE.txt file. + */ + +package tests.contract.map + +import kotlinx.collections.immutable.implementations.immutableMap.PersistentHashMap +import kotlinx.collections.immutable.persistentHashMapOf +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +class PersistentHashMapTest { + + @Test + fun `if the collision is of size 2 and one of the keys is removed, the remaining key must be promoted`() { + val map1: PersistentHashMap = + persistentHashMapOf(-1 to "a", 0 to "b", 32 to "c") as PersistentHashMap + val builder = map1.builder() + val map2 = builder.build() + + assertTrue(map1.equals(builder)) + assertEquals(map1, map2.toMap()) + assertEquals(map1, map2) + + val map3 = map1.remove(0) + builder.remove(0) + val map4 = builder.build() + + assertTrue(map3.equals(builder)) + assertEquals(map3, map4.toMap()) + assertEquals(map3, map4) + } +} \ No newline at end of file diff --git a/core/commonTest/src/stress/map/PersistentHashMapTest.kt b/core/commonTest/src/stress/map/PersistentHashMapTest.kt index 0649bbbf..6eb0c08a 100644 --- a/core/commonTest/src/stress/map/PersistentHashMapTest.kt +++ b/core/commonTest/src/stress/map/PersistentHashMapTest.kt @@ -6,7 +6,6 @@ package tests.stress.map import kotlinx.collections.immutable.PersistentMap -import kotlinx.collections.immutable.implementations.immutableMap.PersistentHashMap import kotlinx.collections.immutable.persistentHashMapOf import tests.NForAlgorithmComplexity import tests.distinctStringValues @@ -352,26 +351,6 @@ class PersistentHashMapTest : ExecutionTimeMeasuringTest() { } } - @Test - fun `if the collision is of size 2 and one of the keys is removed, the remaining key must be promoted`() { - val map1: PersistentHashMap = - persistentHashMapOf(-1 to "a", 0 to "b", 32 to "c") as PersistentHashMap - val builder = map1.builder() - val map2 = builder.build() - - assertTrue(map1.equals(builder)) - assertEquals(map1, map2.toMap()) - assertEquals(map1, map2) - - val map3 = map1.remove(0) - builder.remove(0) - val map4 = builder.build() - - assertTrue(map3.equals(builder)) - assertEquals(map3, map4.toMap()) - assertEquals(map3, map4) - } - private fun testAfterOperation( expected: Map, actual: Map, From 5e6119c91c571ada99d7c4b0af6332752791a7cd Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Fri, 25 Apr 2025 18:25:26 +0400 Subject: [PATCH 52/58] 204: Move PersistentHashMapBuilder tests --- .../map/PersistentHashMapBuilderTest.kt | 121 ++++++++++++++++++ .../map/PersistentHashMapBuilderTest.kt | 103 --------------- 2 files changed, 121 insertions(+), 103 deletions(-) create mode 100644 core/commonTest/src/contract/map/PersistentHashMapBuilderTest.kt diff --git a/core/commonTest/src/contract/map/PersistentHashMapBuilderTest.kt b/core/commonTest/src/contract/map/PersistentHashMapBuilderTest.kt new file mode 100644 index 00000000..732eb81a --- /dev/null +++ b/core/commonTest/src/contract/map/PersistentHashMapBuilderTest.kt @@ -0,0 +1,121 @@ +/* + * Copyright 2016-2025 JetBrains s.r.o. + * Use of this source code is governed by the Apache 2.0 License that can be found in the LICENSE.txt file. + */ + +package tests.contract.map + +import kotlinx.collections.immutable.implementations.immutableMap.PersistentHashMap +import kotlinx.collections.immutable.persistentHashMapOf +import tests.stress.IntWrapper +import kotlin.collections.iterator +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +class PersistentHashMapBuilderTest { + + @Test + fun `should correctly iterate after removing integer key and promotion colliding key during iteration`() { + val removedKey = 0 + val map: PersistentHashMap = + persistentHashMapOf(1 to "a", 2 to "b", 3 to "c", removedKey to "y", 32 to "z") + as PersistentHashMap + + validatePromotion(map, removedKey) + } + + @Test + fun `should correctly iterate after removing IntWrapper key and promotion colliding key during iteration`() { + val removedKey = IntWrapper(0, 0) + val map: PersistentHashMap = persistentHashMapOf( + removedKey to "a", + IntWrapper(1, 0) to "b", + IntWrapper(2, 32) to "c", + IntWrapper(3, 32) to "d" + ) as PersistentHashMap + + validatePromotion(map, removedKey) + } + + private fun validatePromotion(map: PersistentHashMap, removedKey: K) { + val builder = map.builder() + val iterator = builder.entries.iterator() + + val expectedCount = map.size + var actualCount = 0 + + while (iterator.hasNext()) { + val (key, _) = iterator.next() + if (key == removedKey) { + iterator.remove() + } + actualCount++ + } + + val resultMap = builder.build() + for ((key, value) in map) { + if (key != removedKey) { + assertTrue(key in resultMap) + assertEquals(resultMap[key], value) + } else { + assertFalse(key in resultMap) + } + } + + assertEquals(expectedCount, actualCount) + } + + @Test + fun `removing twice on iterators throws IllegalStateException`() { + val map: PersistentHashMap = + persistentHashMapOf(1 to "a", 2 to "b", 3 to "c", 0 to "y", 32 to "z") as PersistentHashMap + val builder = map.builder() + val iterator = builder.entries.iterator() + + assertFailsWith { + while (iterator.hasNext()) { + val (key, _) = iterator.next() + if (key == 0) iterator.remove() + if (key == 0) { + iterator.remove() + iterator.remove() + } + } + } + } + + @Test + fun `removing elements from different iterators throws ConcurrentModificationException`() { + val map: PersistentHashMap = + persistentHashMapOf(1 to "a", 2 to "b", 3 to "c", 0 to "y", 32 to "z") as PersistentHashMap + val builder = map.builder() + val iterator1 = builder.entries.iterator() + val iterator2 = builder.entries.iterator() + + assertFailsWith { + while (iterator1.hasNext()) { + val (key, _) = iterator1.next() + iterator2.next() + if (key == 0) iterator1.remove() + if (key == 2) iterator2.remove() + } + } + } + + @Test + fun `removing element from one iterator and accessing another throws ConcurrentModificationException`() { + val map = persistentHashMapOf(1 to "a", 2 to "b", 3 to "c") + val builder = map.builder() + val iterator1 = builder.entries.iterator() + val iterator2 = builder.entries.iterator() + + assertFailsWith { + iterator1.next() + iterator1.remove() + iterator2.next() + } + } +} \ No newline at end of file diff --git a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt index 83fd8d2f..deb20e4b 100644 --- a/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt +++ b/core/commonTest/src/stress/map/PersistentHashMapBuilderTest.kt @@ -6,7 +6,6 @@ package tests.stress.map import kotlinx.collections.immutable.PersistentMap -import kotlinx.collections.immutable.implementations.immutableMap.PersistentHashMap import kotlinx.collections.immutable.persistentHashMapOf import tests.NForAlgorithmComplexity import tests.distinctStringValues @@ -226,108 +225,6 @@ class PersistentHashMapBuilderTest : ExecutionTimeMeasuringTest() { } } - @Test - fun `should correctly iterate after removing integer key and promotion colliding key during iteration`() { - val removedKey = 0 - val map: PersistentHashMap = - persistentHashMapOf(1 to "a", 2 to "b", 3 to "c", removedKey to "y", 32 to "z") - as PersistentHashMap - - validatePromotion(map, removedKey) - } - - @Test - fun `should correctly iterate after removing IntWrapper key and promotion colliding key during iteration`() { - val removedKey = IntWrapper(0, 0) - val map: PersistentHashMap = persistentHashMapOf( - removedKey to "a", - IntWrapper(1, 0) to "b", - IntWrapper(2, 32) to "c", - IntWrapper(3, 32) to "d" - ) as PersistentHashMap - - validatePromotion(map, removedKey) - } - - private fun validatePromotion(map: PersistentHashMap, removedKey: K) { - val builder = map.builder() - val iterator = builder.entries.iterator() - - val expectedCount = map.size - var actualCount = 0 - - while (iterator.hasNext()) { - val (key, _) = iterator.next() - if (key == removedKey) { - iterator.remove() - } - actualCount++ - } - - val resultMap = builder.build() - for ((key, value) in map) { - if (key != removedKey) { - assertTrue(key in resultMap) - assertEquals(resultMap[key], value) - } else { - assertFalse(key in resultMap) - } - } - - assertEquals(expectedCount, actualCount) - } - - @Test - fun `removing twice on iterators throws IllegalStateException`() { - val map: PersistentHashMap = - persistentHashMapOf(1 to "a", 2 to "b", 3 to "c", 0 to "y", 32 to "z") as PersistentHashMap - val builder = map.builder() - val iterator = builder.entries.iterator() - - assertFailsWith { - while (iterator.hasNext()) { - val (key, _) = iterator.next() - if (key == 0) iterator.remove() - if (key == 0) { - iterator.remove() - iterator.remove() - } - } - } - } - - @Test - fun `removing elements from different iterators throws ConcurrentModificationException`() { - val map: PersistentHashMap = - persistentHashMapOf(1 to "a", 2 to "b", 3 to "c", 0 to "y", 32 to "z") as PersistentHashMap - val builder = map.builder() - val iterator1 = builder.entries.iterator() - val iterator2 = builder.entries.iterator() - - assertFailsWith { - while (iterator1.hasNext()) { - val (key, _) = iterator1.next() - iterator2.next() - if (key == 0) iterator1.remove() - if (key == 2) iterator2.remove() - } - } - } - - @Test - fun `removing element from one iterator and accessing another throws ConcurrentModificationException`() { - val map = persistentHashMapOf(1 to "a", 2 to "b", 3 to "c") - val builder = map.builder() - val iterator1 = builder.entries.iterator() - val iterator2 = builder.entries.iterator() - - assertFailsWith { - iterator1.next() - iterator1.remove() - iterator2.next() - } - } - @Test fun removeTests() { val builder = persistentHashMapOf().builder() From 16f015b54789d76ef166203df8a6c7ed8e4eb6cd Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Fri, 25 Apr 2025 18:26:36 +0400 Subject: [PATCH 53/58] 204: Move `persistentSet and their builder should be equal before and after modification` test --- .../src/{stress => contract}/set/PersistentOrderedSetTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename core/commonTest/src/{stress => contract}/set/PersistentOrderedSetTest.kt (97%) diff --git a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt b/core/commonTest/src/contract/set/PersistentOrderedSetTest.kt similarity index 97% rename from core/commonTest/src/stress/set/PersistentOrderedSetTest.kt rename to core/commonTest/src/contract/set/PersistentOrderedSetTest.kt index a7d9e014..fc638f92 100644 --- a/core/commonTest/src/stress/set/PersistentOrderedSetTest.kt +++ b/core/commonTest/src/contract/set/PersistentOrderedSetTest.kt @@ -3,7 +3,7 @@ * Use of this source code is governed by the Apache 2.0 License that can be found in the LICENSE.txt file. */ -package tests.stress.set +package tests.contract.set import kotlinx.collections.immutable.persistentSetOf import kotlin.test.Test From 95980bb15b5a2c6f02baf0a359c8042c69a84174 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Fri, 25 Apr 2025 18:42:07 +0400 Subject: [PATCH 54/58] 204: `Add persistentHashSet and their builder should be equal before and after modification` test --- .../src/contract/set/PersistentHashSetTest.kt | 32 +++++++++++++++++++ .../contract/set/PersistentOrderedSetTest.kt | 2 +- 2 files changed, 33 insertions(+), 1 deletion(-) create mode 100644 core/commonTest/src/contract/set/PersistentHashSetTest.kt diff --git a/core/commonTest/src/contract/set/PersistentHashSetTest.kt b/core/commonTest/src/contract/set/PersistentHashSetTest.kt new file mode 100644 index 00000000..770b4a29 --- /dev/null +++ b/core/commonTest/src/contract/set/PersistentHashSetTest.kt @@ -0,0 +1,32 @@ +/* + * Copyright 2016-2025 JetBrains s.r.o. + * Use of this source code is governed by the Apache 2.0 License that can be found in the LICENSE.txt file. + */ + +package tests.contract.set + +import kotlinx.collections.immutable.implementations.immutableSet.PersistentHashSet +import kotlinx.collections.immutable.persistentHashSetOf +import kotlinx.collections.immutable.persistentSetOf +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +class PersistentHashSetTest { + + @Test + fun `persistentHashSet and their builder should be equal before and after modification`() { + val set1 = persistentHashSetOf(-1, 0, 32) + val builder = set1.builder() + + assertTrue(set1.equals(builder)) + assertEquals(set1, builder.build()) + assertEquals(set1, builder.build().toSet()) + + val set2 = set1.remove(0) + builder.remove(0) + + assertEquals(set2, builder.build().toSet()) + assertEquals(set2, builder.build()) + } +} \ No newline at end of file diff --git a/core/commonTest/src/contract/set/PersistentOrderedSetTest.kt b/core/commonTest/src/contract/set/PersistentOrderedSetTest.kt index fc638f92..a3d639d9 100644 --- a/core/commonTest/src/contract/set/PersistentOrderedSetTest.kt +++ b/core/commonTest/src/contract/set/PersistentOrderedSetTest.kt @@ -16,7 +16,7 @@ class PersistentOrderedSetTest { * Test from issue: https://github.com/Kotlin/kotlinx.collections.immutable/issues/204 */ @Test - fun `persistentSet and their builder should be equal before and after modification`() { + fun `persistentOrderedSet and their builder should be equal before and after modification`() { val set1 = persistentSetOf(-486539264, 16777216, 0, 67108864) val builder = set1.builder() From 61085fa43539119b17ed1d29b00b7cc53d3d9d12 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Fri, 25 Apr 2025 18:48:03 +0400 Subject: [PATCH 55/58] 204: Remove imports --- core/commonTest/src/contract/set/PersistentHashSetTest.kt | 2 -- 1 file changed, 2 deletions(-) diff --git a/core/commonTest/src/contract/set/PersistentHashSetTest.kt b/core/commonTest/src/contract/set/PersistentHashSetTest.kt index 770b4a29..d1a5c777 100644 --- a/core/commonTest/src/contract/set/PersistentHashSetTest.kt +++ b/core/commonTest/src/contract/set/PersistentHashSetTest.kt @@ -5,9 +5,7 @@ package tests.contract.set -import kotlinx.collections.immutable.implementations.immutableSet.PersistentHashSet import kotlinx.collections.immutable.persistentHashSetOf -import kotlinx.collections.immutable.persistentSetOf import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertTrue From a3392046dbd4e1ffe42b1c20c3eac40a000de9f1 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Fri, 25 Apr 2025 18:51:14 +0400 Subject: [PATCH 56/58] 204: Add PersistentHashSetBuilderTest --- .../set/PersistentHashSetBuilderTest.kt | 119 ++++++++++++++++++ 1 file changed, 119 insertions(+) create mode 100644 core/commonTest/src/contract/set/PersistentHashSetBuilderTest.kt diff --git a/core/commonTest/src/contract/set/PersistentHashSetBuilderTest.kt b/core/commonTest/src/contract/set/PersistentHashSetBuilderTest.kt new file mode 100644 index 00000000..df1df86a --- /dev/null +++ b/core/commonTest/src/contract/set/PersistentHashSetBuilderTest.kt @@ -0,0 +1,119 @@ +/* + * Copyright 2016-2025 JetBrains s.r.o. + * Use of this source code is governed by the Apache 2.0 License that can be found in the LICENSE.txt file. + */ + +package tests.contract.set + +import kotlinx.collections.immutable.implementations.immutableSet.PersistentHashSet +import kotlinx.collections.immutable.persistentHashSetOf +import tests.stress.IntWrapper +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +class PersistentHashSetBuilderTest { + + @Test + fun `should correctly iterate after removing integer element`() { + val removedElement = 0 + val set: PersistentHashSet = + persistentHashSetOf(1, 2, 3, removedElement, 32) + as PersistentHashSet + + validate(set, removedElement) + } + + @Test + fun `should correctly iterate after removing IntWrapper element`() { + val removedElement = IntWrapper(0, 0) + val set: PersistentHashSet = persistentHashSetOf( + removedElement, + IntWrapper(1, 0), + IntWrapper(2, 32), + IntWrapper(3, 32) + ) as PersistentHashSet + + validate(set, removedElement) + } + + private fun validate(set: PersistentHashSet, removedElement: E) { + val builder = set.builder() + val iterator = builder.iterator() + + val expectedCount = set.size + var actualCount = 0 + + while (iterator.hasNext()) { + val element = iterator.next() + if (element == removedElement) { + iterator.remove() + } + actualCount++ + } + + val resultSet = builder.build() + for (element in set) { + if (element != removedElement) { + assertTrue(element in resultSet) + } else { + assertFalse(element in resultSet) + } + } + + assertEquals(expectedCount, actualCount) + } + + @Test + fun `removing twice on iterators throws IllegalStateException`() { + val set: PersistentHashSet = + persistentHashSetOf(1, 2, 3, 0, 32) as PersistentHashSet + val builder = set.builder() + val iterator = builder.iterator() + + assertFailsWith { + while (iterator.hasNext()) { + val element = iterator.next() + if (element == 0) iterator.remove() + if (element == 0) { + iterator.remove() + iterator.remove() + } + } + } + } + + @Test + fun `removing elements from different iterators throws ConcurrentModificationException`() { + val set: PersistentHashSet = + persistentHashSetOf(1, 2, 3, 0, 32) as PersistentHashSet + val builder = set.builder() + val iterator1 = builder.iterator() + val iterator2 = builder.iterator() + + assertFailsWith { + while (iterator1.hasNext()) { + val element1 = iterator1.next() + iterator2.next() + if (element1 == 0) iterator1.remove() + if (element1 == 2) iterator2.remove() + } + } + } + + @Test + fun `removing element from one iterator and accessing another throws ConcurrentModificationException`() { + val set = persistentHashSetOf(1, 2, 3) + val builder = set.builder() + val iterator1 = builder.iterator() + val iterator2 = builder.iterator() + + assertFailsWith { + iterator1.next() + iterator1.remove() + iterator2.next() + } + } +} \ No newline at end of file From 7a3ea2ec535df94b0b167fe40ba8cd7c8338b411 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Fri, 25 Apr 2025 19:23:56 +0400 Subject: [PATCH 57/58] 204: Remove comma from test name --- core/commonTest/src/contract/map/PersistentHashMapTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/commonTest/src/contract/map/PersistentHashMapTest.kt b/core/commonTest/src/contract/map/PersistentHashMapTest.kt index 0c67f790..c1152f4a 100644 --- a/core/commonTest/src/contract/map/PersistentHashMapTest.kt +++ b/core/commonTest/src/contract/map/PersistentHashMapTest.kt @@ -14,7 +14,7 @@ import kotlin.test.assertTrue class PersistentHashMapTest { @Test - fun `if the collision is of size 2 and one of the keys is removed, the remaining key must be promoted`() { + fun `if the collision is of size 2 and one of the keys is removed the remaining key must be promoted`() { val map1: PersistentHashMap = persistentHashMapOf(-1 to "a", 0 to "b", 32 to "c") as PersistentHashMap val builder = map1.builder() From 16bd081f56ff5132bdb225f18d850c692383ae92 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Fri, 25 Apr 2025 19:42:04 +0400 Subject: [PATCH 58/58] 204: Add comments to describe how removedKeyPositionMask check works --- .../PersistentHashMapBuilderContentIterators.kt | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/core/commonMain/src/implementations/immutableMap/PersistentHashMapBuilderContentIterators.kt b/core/commonMain/src/implementations/immutableMap/PersistentHashMapBuilderContentIterators.kt index 6915fba6..ccc03ab5 100644 --- a/core/commonMain/src/implementations/immutableMap/PersistentHashMapBuilderContentIterators.kt +++ b/core/commonMain/src/implementations/immutableMap/PersistentHashMapBuilderContentIterators.kt @@ -99,9 +99,17 @@ internal open class PersistentHashMapBuilderBaseIterator( if (node.hasEntryAt(keyPositionMask)) { // key is directly in buffer val keyIndex = node.entryKeyIndex(keyPositionMask) + // After removing an element, we need to handle node promotion properly to maintain a correct iteration order. + // `removedKeyPositionMask` represents the bit position of the removed key's hash at the current level. + // This is needed to detect if the current key was potentially promoted from a deeper level. val removedKeyPositionMask = if (afterRemove) 1 shl indexSegment(removedKeyHash, shift) else 0 + // Check if the removed key is at the same position as the current key and was previously at a deeper level. + // This indicates a node promotion occurred during removal, + // and we need to handle it in a special way to prevent re-traversing already visited elements. if (keyPositionMask == removedKeyPositionMask && pathIndex < pathLastIndex) { + // Instead of traversing the normal way, we create a special path entry at the previous depth + // that points directly to the promoted entry, maintaining the original iteration sequence. path[pathLastIndex].reset(arrayOf(node.buffer[keyIndex], node.buffer[keyIndex + 1]), ENTRY_SIZE) return }