From aec35b3deeb952c88f19b2f5f894993e664f0594 Mon Sep 17 00:00:00 2001 From: Som Snytt Date: Wed, 7 Feb 2024 14:49:14 -0800 Subject: [PATCH 1/2] Mild refactor for reading --- .../scala/collection/immutable/HashSet.scala | 53 ++++++++----------- 1 file changed, 22 insertions(+), 31 deletions(-) diff --git a/src/library/scala/collection/immutable/HashSet.scala b/src/library/scala/collection/immutable/HashSet.scala index 1db42c9ece47..e5f3518d1c8a 100644 --- a/src/library/scala/collection/immutable/HashSet.scala +++ b/src/library/scala/collection/immutable/HashSet.scala @@ -606,48 +606,37 @@ private final class BitmapIndexedSetNode[A]( if (element0 == element) { if (this.payloadArity == 2 && this.nodeArity == 0) { - /* - * Create new node with remaining pair. The new node will a) either become the new root - * returned, or b) unwrapped and inlined during returning. - */ + // Create new node with remaining pair. The new node will a) either become the new root + // returned, or b) unwrapped and inlined during returning. val newDataMap = if (shift == 0) (dataMap ^ bitpos) else bitposFrom(maskFrom(elementHash, 0)) - if (index == 0) - return new BitmapIndexedSetNode[A](newDataMap, 0, Array(getPayload(1)), Array(originalHashes(1)), size - 1, improve(originalHashes(1))) - else - return new BitmapIndexedSetNode[A](newDataMap, 0, Array(getPayload(0)), Array(originalHashes(0)), size - 1, improve(originalHashes(0))) + if (index == 0) new BitmapIndexedSetNode[A](newDataMap, 0, Array(getPayload(1)), Array(originalHashes(1)), size - 1, improve(originalHashes(1))) + else new BitmapIndexedSetNode[A](newDataMap, 0, Array(getPayload(0)), Array(originalHashes(0)), size - 1, improve(originalHashes(0))) } - else return copyAndRemoveValue(bitpos, elementHash) - } else return this + else copyAndRemoveValue(bitpos, elementHash) + } + else this } - - if ((nodeMap & bitpos) != 0) { + else if ((nodeMap & bitpos) != 0) { val index = indexFrom(nodeMap, mask, bitpos) val subNode = this.getNode(index) - val subNodeNew = subNode.removed(element, originalHash, elementHash, shift + BitPartitionSize) - if (subNodeNew eq subNode) return this - - // cache just in case subNodeNew is a hashCollision node, in which in which case a little arithmetic is avoided - // in Vector#length - val subNodeNewSize = subNodeNew.size - - if (subNodeNewSize == 1) { - if (this.size == subNode.size) { + if (subNodeNew eq subNode) this + // if subNodeNew is a hashCollision node, size has cost in Vector#length + else subNodeNew.size match { + case 1 => // subNode is the only child (no other data or node children of `this` exist) // escalate (singleton or empty) result - return subNodeNew.asInstanceOf[BitmapIndexedSetNode[A]] - } else { + if (this.size == subNode.size) subNodeNew.asInstanceOf[BitmapIndexedSetNode[A]] // inline value (move to front) - return copyAndMigrateFromNodeToInline(bitpos, elementHash, subNode, subNodeNew) - } - } else if (subNodeNewSize > 1) { - // modify current node (set replacement node) - return copyAndSetNode(bitpos, subNode, subNodeNew) + else copyAndMigrateFromNodeToInline(bitpos, elementHash, subNode, subNodeNew) + case subNodeNewSize if subNodeNewSize > 1 => + // modify current node (set replacement node) + copyAndSetNode(bitpos, subNode, subNodeNew) + case _ => this } } - - this + else this } /** Variant of `removed` which will perform mutation on only the top-level node (`this`), rather than return a new * node @@ -1428,6 +1417,8 @@ private final class BitmapIndexedSetNode[A]( override def hashCode(): Int = throw new UnsupportedOperationException("Trie nodes do not support hashing.") + override def toString: String = f"BitmapIndexedSetNode(size=$size, dataMap=$dataMap%x, nodeMap=$nodeMap%x)" // content=${scala.runtime.ScalaRunTime.stringOf(content)} + override def copy(): BitmapIndexedSetNode[A] = { val contentClone = content.clone() val contentLength = contentClone.length @@ -2077,7 +2068,7 @@ private[collection] final class HashSetBuilder[A] extends ReusableBuilder[A, Has ensureUnaliased() val h = elem.## val im = improve(h) - update(rootNode, elem, h, im, 0) + update(rootNode, elem, originalHash = h, elementHash = im, shift = 0) this } From 8a87e9ab5928cbc4838dce6840e58e2a031951e1 Mon Sep 17 00:00:00 2001 From: Som Snytt Date: Wed, 7 Feb 2024 14:59:55 -0800 Subject: [PATCH 2/2] Handle empty test and NxD in subsetOf --- .../scala/collection/immutable/HashSet.scala | 6 +++--- .../collection/immutable/HashSetTest.scala | 20 +++++++++++++++++-- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/src/library/scala/collection/immutable/HashSet.scala b/src/library/scala/collection/immutable/HashSet.scala index e5f3518d1c8a..aa6d4e786882 100644 --- a/src/library/scala/collection/immutable/HashSet.scala +++ b/src/library/scala/collection/immutable/HashSet.scala @@ -190,10 +190,10 @@ final class HashSet[A] private[immutable](private[immutable] val rootNode: Bitma * Stops iterating the first time that f returns `false`.*/ @`inline` private[collection] def foreachWithHashWhile(f: (A, Int) => Boolean): Unit = rootNode.foreachWithHashWhile(f) - def subsetOf(that: Set[A]): Boolean = if (that.isEmpty) true else that match { + def subsetOf(that: Set[A]): Boolean = isEmpty || !that.isEmpty && (that match { case set: HashSet[A] => rootNode.subsetOf(set.rootNode, 0) case _ => super.subsetOf(that) - } + }) override def equals(that: Any): Boolean = that match { @@ -988,7 +988,7 @@ private final class BitmapIndexedSetNode[A]( val elementHash = improve(elementUnimprovedHash) subNode.contains(payload, elementUnimprovedHash, elementHash, shift + BitPartitionSize) } - } else { + } else ((node.dataMap & bitpos) == 0) && { // Node x Node val subNode0 = this.getNode(indexFrom(this.nodeMap, bitpos)) val subNode1 = node.getNode(indexFrom(node.nodeMap, bitpos)) diff --git a/test/junit/scala/collection/immutable/HashSetTest.scala b/test/junit/scala/collection/immutable/HashSetTest.scala index 24760e537c82..1c16fffbb331 100644 --- a/test/junit/scala/collection/immutable/HashSetTest.scala +++ b/test/junit/scala/collection/immutable/HashSetTest.scala @@ -1,6 +1,6 @@ package scala.collection.immutable -import org.junit.Assert.{ assertThrows => _, _ } +import org.junit.Assert.{assertEquals, assertFalse, assertSame, assertTrue} import org.junit.{Ignore, Test} import org.junit.runner.RunWith import org.junit.runners.JUnit4 @@ -9,7 +9,7 @@ import scala.tools.testkit.AllocationTest import scala.tools.testkit.AssertUtil._ @RunWith(classOf[JUnit4]) -class HashSetTest extends AllocationTest { +class HashSetAllocationTest extends AllocationTest { @Test def t11551(): Unit = { @@ -304,3 +304,19 @@ class HashSetTest extends AllocationTest { } } } +class HashSetTest { + @Test def `t12944 subset case NxD is false`: Unit = { + + // Bryan Cranston, John Stuart Mill + val sets = Seq( + (HashSet("ian", "cra"), HashSet("john", "michael", "mills")), + (HashSet("ian", "cras"), HashSet("john", "michael", "mills")), + (HashSet("ian", "crasto"), HashSet("john", "michael", "mills")), + (HashSet("ian", "craston"), HashSet("john", "michael", "mills")), + ) + + sets.foreach { case (s1, s2) => assertFalse(s"$s1 <= $s2", s1.subsetOf(s2)) } + } + @Test def `nonempty subset of empty is false`: Unit = + assertFalse(HashSet("bryan", "cranston", "john", "stuart", "mill").subsetOf(HashSet.empty[String])) +}