Merge pull request #8410 from szeiger/wip/revert-7588

Revert "VectorBuilder avoids copying data if possible"

build.sbt

@@ -132,6 +132,23 @@ val mimaFilterSettings = Seq {
     ProblemFilters.exclude[DirectMissingMethodProblem]("scala.reflect.runtime.SynchronizedSymbols#SynchronizedSymbol.typeConstructor"),
     ProblemFilters.exclude[IncompatibleMethTypeProblem]("scala.reflect.io.ZipArchive.getDir"),
     ProblemFilters.exclude[IncompatibleResultTypeProblem]("scala.reflect.io.FileZipArchive.allDirs"),
+    ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.VectorBuilder.nullSlotAndCopy"),
+    ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.VectorBuilder.gotoPosWritable1"),
+    ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.VectorBuilder.gotoPosWritable1$default$4"),
+    ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.VectorBuilder.nullSlotAndCopy$default$3"),
+    ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.VectorIterator.nullSlotAndCopy"),
+    ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.VectorIterator.gotoPosWritable1"),
+    ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.VectorIterator.gotoPosWritable1$default$4"),
+    ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.VectorIterator.nullSlotAndCopy$default$3"),
+    ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.Vector.nullSlotAndCopy"),
+    ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.Vector.focus"),
+    ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.Vector.gotoPosWritable1"),
+    ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.Vector.gotoPosWritable1$default$4"),
+    ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.Vector.nullSlotAndCopy$default$3"),
+    ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.VectorPointer.nullSlotAndCopy"),
+    ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.VectorPointer.gotoPosWritable1"),
+    ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.VectorPointer.gotoPosWritable1$default$4"),
+    ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.VectorPointer.nullSlotAndCopy$default$3"),
   ),
 }
 

src/library/scala/collection/immutable/Vector.scala

@@ -290,51 +290,30 @@ final class Vector[+A] private[immutable] (private[collection] val startIndex: I
     dropRight(1)
   }
 
-  override def appendedAll[B >: A](suffix: collection.IterableOnce[B]): Vector[B] =
-    suffix match {
-      case v: Vector[B] =>
-        val thisLength = this.length
-        val thatLength = v.length
-        if (thisLength == 0) v
-        else if (thatLength == 0) this
-        else if (thatLength <= Vector.TinyAppendFaster) {
-          // Often it's better to append small numbers of elements (or prepend if RHS is a vector)
-          var v0: Vector[B] = this
-          var i = 0
-          while (i < thatLength) {
-            v0 = v0.appended(v(i))
-            i += 1
-          }
-          v0
-        } else {
-          if (thisLength < (thatLength >>> Vector.Log2ConcatFaster)) {
-            var v0 = v
-            val iter = this.reverseIterator
-            while(iter.hasNext) {
-              v0 = iter.next() +: v0
-            }
-            v0
-          } else {
-            new VectorBuilder[B]().addAll(this).addAll(suffix).result()
-          }
-        }
-      case _ =>
-        val thatKnownSize = suffix.knownSize
-        if (thatKnownSize == 0) this
-        else if (thatKnownSize > 0 && thatKnownSize <= Vector.TinyAppendFaster) {
-          var v0: Vector[B] = this
-          val iter = suffix.iterator
-          while (iter.hasNext) {
-            v0 = v0.appended(iter.next())
+  // appendAll (suboptimal but avoids worst performance gotchas)
+  override def appendedAll[B >: A](suffix: collection.IterableOnce[B]): Vector[B] = {
+    import Vector.{Log2ConcatFaster, TinyAppendFaster}
+    if (suffix.iterator.isEmpty) this
+    else {
+      suffix match {
+        case suffix: collection.Iterable[B] =>
+          suffix.size match {
+            // Often it's better to append small numbers of elements (or prepend if RHS is a vector)
+            case n if n <= TinyAppendFaster || n < (this.size >>> Log2ConcatFaster) =>
+              var v: Vector[B] = this
+              for (x <- suffix) v = v :+ x
+              v
+            case n if this.size < (n >>> Log2ConcatFaster) && suffix.isInstanceOf[Vector[_]] =>
+              var v = suffix.asInstanceOf[Vector[B]]
+              val ri = this.reverseIterator
+              while (ri.hasNext) v = ri.next() +: v
+              v
+            case _ => super.appendedAll(suffix)
           }
-          v0
-        } else {
-          val iter = suffix.iterator
-          if (iter.hasNext) {
-            new VectorBuilder[B]().addAll(this).addAll(suffix).result()
-          } else this
-        }
+        case _ => super.appendedAll(suffix)
+      }
     }
+  }
 
   override def prependedAll[B >: A](prefix: collection.IterableOnce[B]): Vector[B] = {
     // Implementation similar to `appendAll`: when of the collections to concatenate (either `this` or `prefix`)
@@ -851,21 +830,10 @@ final class VectorBuilder[A]() extends ReusableBuilder[A, Vector[A]] with Vector
   display0 = new Array[AnyRef](32)
   depth = 1
 
-  /** The index within the final Vector of `this.display0(0)` */
   private[this] var blockIndex = 0
-  /** The index within `this.display0` which is the next available index to write to.
-    * This value may be equal to display0.length, in which case before writing, a new block
-    * should be created (see advanceToNextBlockIfNecessary)*/
   private[this] var lo = 0
-  /** Indicates an offset of the final vector from the actual underlying array elements. This is
-    * used for example in `drop(1)` where instead of copying the entire Vector, only the startIndex is changed.
-    *
-    * This is present in the Builder because we may be able to share structure with a Vector that is `addAll`'d to this.
-    * In which case we must track that Vector's startIndex offset.
-    * */
-  private[this] var startIndex = 0
-
-  def size: Int = (blockIndex & ~31) + lo - startIndex
+
+  def size: Int = blockIndex + lo
   def isEmpty: Boolean = size == 0
   def nonEmpty: Boolean = size != 0
 
@@ -888,49 +856,10 @@ final class VectorBuilder[A]() extends ReusableBuilder[A, Vector[A]] with Vector
   }
 
   override def addAll(xs: IterableOnce[A]): this.type = {
-
-    xs match {
-      case v: Vector[A] if this.isEmpty && v.length >= 32 =>
-        depth = v.depth
-        blockIndex = (v.endIndex - 1) & ~31
-        lo = v.endIndex - blockIndex
-        startIndex = v.startIndex
-
-        /** `initFrom` will overwrite display0. Keep reference to it so we can reuse the array.*/
-        val initialDisplay0 = display0
-        initFrom(v)
-        stabilize(v.focus)
-        gotoPosWritable1(v.focus, blockIndex, v.focus ^ blockIndex, initialDisplay0)
-
-        depth match {
-          case 2 =>
-            display1((blockIndex >>> 5) & 31) = display0
-          case 3 =>
-            display1((blockIndex >>> 5) & 31) = display0
-            display2((blockIndex >>> 10) & 31) = display1
-          case 4 =>
-            display1((blockIndex >>> 5) & 31) = display0
-            display2((blockIndex >>> 10) & 31) = display1
-            display3((blockIndex >>> 15) & 31) = display2
-          case 5 =>
-            display1((blockIndex >>> 5) & 31) = display0
-            display2((blockIndex >>> 10) & 31) = display1
-            display3((blockIndex >>> 15) & 31) = display2
-            display4((blockIndex >>> 20) & 31) = display3
-          case 6 =>
-            display1((blockIndex >>> 5) & 31) = display0
-            display2((blockIndex >>> 10) & 31) = display1
-            display3((blockIndex >>> 15) & 31) = display2
-            display4((blockIndex >>> 20) & 31) = display3
-            display5((blockIndex >>> 25) & 31) = display4
-          case _ => ()
-        }
-      case _ =>
-        val it = (xs.iterator : Iterator[A]).asInstanceOf[Iterator[AnyRef]]
-        while (it.hasNext) {
-          advanceToNextBlockIfNecessary()
-          lo += it.copyToArray(xs = display0, start = lo, len = display0.length - lo)
-        }
+    val it = (xs.iterator : Iterator[A]).asInstanceOf[Iterator[AnyRef]]
+    while (it.hasNext) {
+      advanceToNextBlockIfNecessary()
+      lo += it.copyToArray(xs = display0, start = lo, len = display0.length - lo)
     }
     this
   }
@@ -939,9 +868,9 @@ final class VectorBuilder[A]() extends ReusableBuilder[A, Vector[A]] with Vector
     val size = this.size
     if (size == 0)
       return Vector.empty
-    val s = new Vector[A](startIndex, blockIndex + lo, 0) // should focus front or back?
+    val s = new Vector[A](0, size, 0) // should focus front or back?
     s.initFrom(this)
-    if (depth > 1) s.gotoPos(startIndex, blockIndex + lo - 1) // we're currently focused to size - 1, not size!
+    if (depth > 1) s.gotoPos(0, size - 1) // we're currently focused to size - 1, not size!
     releaseFence()
     s
   }
@@ -951,7 +880,6 @@ final class VectorBuilder[A]() extends ReusableBuilder[A, Vector[A]] with Vector
     display0 = new Array[AnyRef](32)
     blockIndex = 0
     lo = 0
-    startIndex = 0
   }
 }
 
@@ -1147,19 +1075,10 @@ private[immutable] trait VectorPointer[+T] {
 
     // STUFF BELOW USED BY APPEND / UPDATE
 
-  /** Sets array(index) to null and returns an array with same contents as what was previously at array(index)
-    *
-    * If `destination` array is not null, original contents of array(index) will be copied to it, and it will be returned.
-    * Otherwise array(index).clone() is returned
-    */
-    private[immutable] final def nullSlotAndCopy[T <: AnyRef](array: Array[Array[T]], index: Int, destination: Array[T] = null): Array[T] = {
+    private[immutable] final def nullSlotAndCopy[T <: AnyRef](array: Array[Array[T]], index: Int): Array[T] = {
       val x = array(index)
       array(index) = null
-      if (destination == null) x.clone()
-      else {
-        x.copyToArray(destination, 0)
-        destination
-      }
+      x.clone()
     }
 
     // make sure there is no aliasing
@@ -1242,9 +1161,10 @@ private[immutable] trait VectorPointer[+T] {
         display0 = display0.clone()
     }
 
+
     // requires structure is dirty and at pos oldIndex,
     // ensures structure is dirty and at pos newIndex and writable at level 0
-    private[immutable] final def gotoPosWritable1(oldIndex: Int, newIndex: Int, xor: Int, reuseDisplay0: Array[AnyRef] = null): Unit = {
+    private[immutable] final def gotoPosWritable1(oldIndex: Int, newIndex: Int, xor: Int): Unit = {
       if        (xor < (1 <<  5)) { // level = 0
         display0 = display0.clone()
       } else if (xor < (1 << 10)) { // level = 1
@@ -1257,7 +1177,7 @@ private[immutable] trait VectorPointer[+T] {
         display1((oldIndex >>>  5) & 31) = display0
         display2((oldIndex >>> 10) & 31) = display1
         display1 = nullSlotAndCopy(display2, (newIndex >>> 10) & 31)
-        display0 = nullSlotAndCopy(display1, (newIndex >>>  5) & 31, reuseDisplay0)
+        display0 = nullSlotAndCopy(display1, (newIndex >>>  5) & 31)
       } else if (xor < (1 << 20)) { // level = 3
         display1 = display1.clone()
         display2 = display2.clone()
@@ -1267,7 +1187,7 @@ private[immutable] trait VectorPointer[+T] {
         display3((oldIndex >>> 15) & 31) = display2
         display2 = nullSlotAndCopy(display3, (newIndex >>> 15) & 31)
         display1 = nullSlotAndCopy(display2, (newIndex >>> 10) & 31)
-        display0 = nullSlotAndCopy(display1, (newIndex >>>  5) & 31, reuseDisplay0)
+        display0 = nullSlotAndCopy(display1, (newIndex >>>  5) & 31)
       } else if (xor < (1 << 25)) { // level = 4
         display1 = display1.clone()
         display2 = display2.clone()
@@ -1280,7 +1200,7 @@ private[immutable] trait VectorPointer[+T] {
         display3 = nullSlotAndCopy(display4, (newIndex >>> 20) & 31)
         display2 = nullSlotAndCopy(display3, (newIndex >>> 15) & 31)
         display1 = nullSlotAndCopy(display2, (newIndex >>> 10) & 31)
-        display0 = nullSlotAndCopy(display1, (newIndex >>>  5) & 31, reuseDisplay0)
+        display0 = nullSlotAndCopy(display1, (newIndex >>>  5) & 31)
       } else if (xor < (1 << 30)) { // level = 5
         display1 = display1.clone()
         display2 = display2.clone()
@@ -1296,7 +1216,7 @@ private[immutable] trait VectorPointer[+T] {
         display3 = nullSlotAndCopy(display4, (newIndex >>> 20) & 31)
         display2 = nullSlotAndCopy(display3, (newIndex >>> 15) & 31)
         display1 = nullSlotAndCopy(display2, (newIndex >>> 10) & 31)
-        display0 = nullSlotAndCopy(display1, (newIndex >>>  5) & 31, reuseDisplay0)
+        display0 = nullSlotAndCopy(display1, (newIndex >>>  5) & 31)
       } else {                      // level = 6
         throw new IllegalArgumentException()
       }

test/junit/scala/collection/immutable/VectorTest.scala

@@ -263,4 +263,30 @@ class VectorTest {
       assertEquals(List(""), vector.updated(0, ""))
     }
   }
+
+  def t11636(): Unit = {
+    val a: Vector[String] = "O" +: Iterator.continually("E").take(2101).foldLeft(Vector.empty[String])((v, e) => v :+ e) :+ "C"
+    val a0: ArraySeq[String] = ArraySeq("O") ++ Iterator.continually("E").take(2101) ++ ArraySeq("C")
+
+    val b: Vector[String] = "O" +: Iterator.continually("E").take(223) .foldLeft(Vector.empty[String])((v, e) => v :+ e) :+ "C"
+    val b0: ArraySeq[String] = ArraySeq("O") ++ Iterator.continually("E").take(223) ++ ArraySeq("C")
+
+    val c: Vector[String] = "O" +: Iterator.continually("E").take(135) .foldLeft(Vector.empty[String])((v, e) => v :+ e) :+ "C"
+    val c0: ArraySeq[String] = ArraySeq("O") ++ Iterator.continually("E").take(135) ++ ArraySeq("C")
+
+    val d: Vector[String] = "O" +: Iterator.continually("E").take(0)   .foldLeft(Vector.empty[String])((v, e) => v :+ e) :+ "C"
+    val d0: ArraySeq[String] = ArraySeq("O", "C")
+
+    val e: Vector[String] = "O" +: Iterator.continually("E").take(376) .foldLeft(Vector.empty[String])((v, e) => v :+ e) :+ "C"
+    val e0: ArraySeq[String] = ArraySeq("O") ++ Iterator.continually("E").take(376) ++ ArraySeq("C")
+
+    val f: Vector[String] = "O" +: Iterator.continually("E").take(365) .foldLeft(Vector.empty[String])((v, e) => v :+ e) :+ "C"
+    val f0: ArraySeq[String] = ArraySeq("O") ++ Iterator.continually("E").take(365) ++ ArraySeq("C")
+
+    assertEquals(a0 ++ b0, a ++ b)
+    assertEquals(a0 ++ b0 ++ c0, a ++ b ++ c)
+    assertEquals(a0 ++ b0 ++ c0 ++ d0, a ++ b ++ c ++ d)
+    assertEquals(a0 ++ b0 ++ c0 ++ d0 ++ e0, a ++ b ++ c ++ d ++ e)
+    assertEquals(a0 ++ b0 ++ c0 ++ d0 ++ e0 ++ f0, a ++ b ++ c ++ d ++ e ++ f)
+  }
 }