Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Vector: Fix 2.13.2 performance regression: Restore special cases for small operands in {append,prepend}edAll #9036

Merged
merged 1 commit into from Jun 12, 2020
Merged

Vector: Fix 2.13.2 performance regression: Restore special cases for small operands in {append,prepend}edAll #9036

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions build.sbt
View file
Open in desktop
Expand Up @@ -327,6 +327,8 @@ val mimaFilterSettings = Seq {
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.RedBlackTree#TreeIterator.ordering"),
ProblemFilters.exclude[ReversedMissingMethodProblem]("scala.collection.SortedSet.scala$collection$SortedSet$$super=uals"),
ProblemFilters.exclude[ReversedMissingMethodProblem]("scala.collection.SortedMap.scala$collection$SortedMap$$super=uals"),

ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.Vector.prependedAll0"),
),
}

Expand Down
87 changes: 86 additions & 1 deletion src/library/scala/collection/immutable/Vector.scala
View file
Open in desktop
Expand Up @@ -191,14 +191,33 @@ sealed abstract class Vector[+A] private[immutable] (private[immutable] final va
override def updated[B >: A](index: Int, elem: B): Vector[B] = super.updated(index, elem)
override def appended[B >: A](elem: B): Vector[B] = super.appended(elem)
override def prepended[B >: A](elem: B): Vector[B] = super.prepended(elem)
override def prependedAll[B >: A](prefix: collection.IterableOnce[B]): Vector[B] = super.prependedAll(prefix)
override def prependedAll[B >: A](prefix: collection.IterableOnce[B]): Vector[B] = {
val k = prefix.knownSize
if(k == 0) this
else prependedAll0(prefix, k)
}

override final def appendedAll[B >: A](suffix: collection.IterableOnce[B]): Vector[B] = {
val k = suffix.knownSize
if(k == 0) this
else appendedAll0(suffix, k)
}

protected[this] def prependedAll0[B >: A](prefix: collection.IterableOnce[B], k: Int): Vector[B] = {
val tinyAppendLimit = 4 + vectorSliceCount
if (k < tinyAppendLimit /*|| k < (this.size >>> Log2ConcatFaster)*/) {
Copy link
Contributor

Choose a reason for hiding this comment

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

incorrect assumption that k is positive

Copy link
Contributor

Choose a reason for hiding this comment

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

I have no idea if this is the source of scala/bug#12564, but it might be

var v: Vector[B] = this
val it = IndexedSeq.from(prefix).reverseIterator
while (it.hasNext) v = it.next() +: v
v
} else if (this.size < (k >>> Log2ConcatFaster) && prefix.isInstanceOf[Vector[_]]) {
var v = prefix.asInstanceOf[Vector[B]]
val it = this.iterator
while (it.hasNext) v = v :+ it.next()
v
} else super.prependedAll(prefix)
}

protected[this] def appendedAll0[B >: A](suffix: collection.IterableOnce[B], k: Int): Vector[B] = {
val tinyAppendLimit = 4 + vectorSliceCount
if(k > 0 && k < tinyAppendLimit) {
Expand All @@ -208,6 +227,11 @@ sealed abstract class Vector[+A] private[immutable] (private[immutable] final va
case _ => suffix.iterator.foreach(x => v = v.appended(x))
}
v
} else if (this.size < (k >>> Log2ConcatFaster) && suffix.isInstanceOf[Vector[_]]) {
var v = suffix.asInstanceOf[Vector[B]]
val ri = this.reverseIterator
while (ri.hasNext) v = v.prepended(ri.next())
v
} else new VectorBuilder[B].initFrom(this).addAll(suffix).result()
}

Expand Down Expand Up @@ -337,6 +361,9 @@ private object Vector0 extends BigVector[Nothing](empty1, empty1, 0) {
}
}

override protected[this]def prependedAll0[B >: Nothing](prefix: collection.IterableOnce[B], k: Int): Vector[B] =
Vector.from(prefix)

override protected[this]def appendedAll0[B >: Nothing](suffix: collection.IterableOnce[B], k: Int): Vector[B] =
Vector.from(suffix)

Expand Down Expand Up @@ -388,6 +415,12 @@ private final class Vector1[+A](_data1: Arr1) extends VectorImpl[A](_data1) {
protected[immutable] def vectorSlice(idx: Int): Array[_ <: AnyRef] = prefix1
protected[immutable] def vectorSlicePrefixLength(idx: Int): Int = prefix1.length

override protected[this] def prependedAll0[B >: A](prefix: collection.IterableOnce[B], k: Int): Vector[B] = {
val data1b = prepend1IfSpace(prefix1, prefix)
if(data1b ne null) new Vector1(data1b)
else super.prependedAll0(prefix, k)
}

override protected[this] def appendedAll0[B >: A](suffix: collection.IterableOnce[B], k: Int): Vector[B] = {
val data1b = append1IfSpace(prefix1, suffix)
if(data1b ne null) new Vector1(data1b)
Expand Down Expand Up @@ -477,6 +510,11 @@ private final class Vector2[+A](_prefix1: Arr1, private[immutable] val len1: Int
case 2 => length0
}

override protected[this] def prependedAll0[B >: A](prefix: collection.IterableOnce[B], k: Int): Vector[B] = {
val prefix1b = prepend1IfSpace(prefix1, prefix)
if(prefix1b ne null) copy(prefix1 = prefix1b, len1 = len1-prefix1.length+prefix1b.length, length0 = length0-prefix1.length+prefix1b.length)
else super.prependedAll0(prefix, k)
}
override protected[this] def appendedAll0[B >: A](suffix: collection.IterableOnce[B], k: Int): Vector[B] = {
val suffix1b = append1IfSpace(suffix1, suffix)
if(suffix1b ne null) copy(suffix1 = suffix1b, length0 = length0-suffix1.length+suffix1b.length)
Expand Down Expand Up @@ -588,6 +626,11 @@ private final class Vector3[+A](_prefix1: Arr1, private[immutable] val len1: Int
case 4 => length0
}

override protected[this] def prependedAll0[B >: A](prefix: collection.IterableOnce[B], k: Int): Vector[B] = {
val prefix1b = prepend1IfSpace(prefix1, prefix)
if(prefix1b ne null) copy(prefix1 = prefix1b, len1 = len1-prefix1.length+prefix1b.length, length0 = length0-prefix1.length+prefix1b.length)
else super.prependedAll0(prefix, k)
}
override protected[this] def appendedAll0[B >: A](suffix: collection.IterableOnce[B], k: Int): Vector[B] = {
val suffix1b = append1IfSpace(suffix1, suffix)
if(suffix1b ne null) copy(suffix1 = suffix1b, length0 = length0-suffix1.length+suffix1b.length)
Expand Down Expand Up @@ -719,6 +762,12 @@ private final class Vector4[+A](_prefix1: Arr1, private[immutable] val len1: Int
case 6 => length0
}

override protected[this] def prependedAll0[B >: A](prefix: collection.IterableOnce[B], k: Int): Vector[B] = {
val prefix1b = prepend1IfSpace(prefix1, prefix)
if(prefix1b ne null) copy(prefix1 = prefix1b, len1 = len1-prefix1.length+prefix1b.length, length0 = length0-prefix1.length+prefix1b.length)
else super.prependedAll0(prefix, k)
}

override protected[this] def appendedAll0[B >: A](suffix: collection.IterableOnce[B], k: Int): Vector[B] = {
val suffix1b = append1IfSpace(suffix1, suffix)
if(suffix1b ne null) copy(suffix1 = suffix1b, length0 = length0-suffix1.length+suffix1b.length)
Expand Down Expand Up @@ -870,6 +919,12 @@ private final class Vector5[+A](_prefix1: Arr1, private[immutable] val len1: Int
case 8 => length0
}

override protected[this] def prependedAll0[B >: A](prefix: collection.IterableOnce[B], k: Int): Vector[B] = {
val prefix1b = prepend1IfSpace(prefix1, prefix)
if(prefix1b ne null) copy(prefix1 = prefix1b, len1 = len1-prefix1.length+prefix1b.length, length0 = length0-prefix1.length+prefix1b.length)
else super.prependedAll0(prefix, k)
}

override protected[this] def appendedAll0[B >: A](suffix: collection.IterableOnce[B], k: Int): Vector[B] = {
val suffix1b = append1IfSpace(suffix1, suffix)
if(suffix1b ne null) copy(suffix1 = suffix1b, length0 = length0-suffix1.length+suffix1b.length)
Expand Down Expand Up @@ -1041,6 +1096,12 @@ private final class Vector6[+A](_prefix1: Arr1, private[immutable] val len1: Int
case 10 => length0
}

override protected[this] def prependedAll0[B >: A](prefix: collection.IterableOnce[B], k: Int): Vector[B] = {
val prefix1b = prepend1IfSpace(prefix1, prefix)
if(prefix1b ne null) copy(prefix1 = prefix1b, len1 = len1-prefix1.length+prefix1b.length, length0 = length0-prefix1.length+prefix1b.length)
else super.prependedAll0(prefix, k)
}

override protected[this] def appendedAll0[B >: A](suffix: collection.IterableOnce[B], k: Int): Vector[B] = {
val suffix1b = append1IfSpace(suffix1, suffix)
if(suffix1b ne null) copy(suffix1 = suffix1b, length0 = length0-suffix1.length+suffix1b.length)
Expand Down Expand Up @@ -1674,6 +1735,7 @@ private[immutable] object VectorInline {
final val BITS6 = BITS * 6
final val WIDTH6 = 1 << BITS6
final val LASTWIDTH = WIDTH << 1 // 1 extra bit in the last level to go up to Int.MaxValue (2^31-1) instead of 2^30:
final val Log2ConcatFaster = 5

type Arr1 = Array[AnyRef]
type Arr2 = Array[Array[AnyRef]]
Expand Down Expand Up @@ -1855,6 +1917,29 @@ private object VectorStatics {
ac.asInstanceOf[Array[T]]
}

final def prepend1IfSpace(prefix1: Arr1, xs: IterableOnce[_]): Arr1 = xs match {
case it: Iterable[_] =>
if(it.sizeCompare(WIDTH-prefix1.length) <= 0) {
it.size match {
case 0 => null
case 1 => copyPrepend(it.head.asInstanceOf[AnyRef], prefix1)
case s =>
val prefix1b = new Arr1(prefix1.length + s)
System.arraycopy(prefix1, 0, prefix1b, s, prefix1.length)
it.copyToArray(prefix1b.asInstanceOf[Array[Any]], 0)
prefix1b
}
} else null
case it =>
val s = it.knownSize
if(s > 0 && s <= WIDTH-prefix1.length) {
val prefix1b = new Arr1(prefix1.length + s)
System.arraycopy(prefix1, 0, prefix1b, s, prefix1.length)
it.iterator.copyToArray(prefix1b.asInstanceOf[Array[Any]], 0)
prefix1b
} else null
}

final def append1IfSpace(suffix1: Arr1, xs: IterableOnce[_]): Arr1 = xs match {
case it: Iterable[_] =>
if(it.sizeCompare(WIDTH-suffix1.length) <= 0) {
Expand Down
13 changes: 11 additions & 2 deletions test/junit/scala/collection/immutable/VectorTest.scala
View file
Open in desktop
Expand Up @@ -34,16 +34,18 @@ class VectorTest {
}

@Test
def hasCorrectPrependedAll(): Unit = {
def hasCorrectAppenedAndPrependedAll(): Unit = {
val els = Vector(1 to 1000: _*)

for (i <- 0 until els.size) {
val (prefix, suffix) = els.splitAt(i)
validateDebug(prefix)
validateDebug(suffix)
validateDebug(prefix ++: suffix)
assertEquals(els, prefix ++: suffix)
assertEquals((prefix, suffix).toString, els, prefix ++: suffix)
assertEquals((prefix, suffix).toString, els, prefix :++ suffix)
assertEquals(els, prefix.toList ++: suffix)
assertEquals(els, prefix.toList :++ suffix)
}
}

Expand Down Expand Up @@ -412,6 +414,13 @@ class VectorTest {
assertEquals(0 until (size + appendSize), v3)
}
}

// scala/bug#12027
// This took forever in 2.13.2
@Test def testAppendWithTinyLhs(): Unit = {
(1 to 1000000).foldLeft(Vector.empty[Int]) { (v, i) => Vector(i) ++ v }
(1 to 1000000).foldLeft(Vector.empty[Int]) { (v, i) => v.prependedAll(Vector(i)) }
}
}

object VectorUtils {
Expand Down