Merge pull request #9819 from NthPortal/topic/ArrayBuffer-sizing/PR

[bug#12464] Fix resizing for `ArrayBuffer` and `PriorityQueue`

project/MimaFilters.scala

@@ -30,6 +30,12 @@ object MimaFilters extends AutoPlugin {
     ProblemFilters.exclude[DirectMissingMethodProblem]("scala.Predef#SeqCharSequence.isEmpty"),
     ProblemFilters.exclude[DirectMissingMethodProblem]("scala.Predef#ArrayCharSequence.isEmpty"),
     ProblemFilters.exclude[DirectMissingMethodProblem]("scala.runtime.ArrayCharSequence.isEmpty"),
+
+    // scala/scala#9819
+    ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.mutable.PriorityQueue#ResizableArrayAccess.p_ensureAdditionalSize"), // private[PriorityQueue]
+    ProblemFilters.exclude[MissingClassProblem]("scala.runtime.PStatics"),  // private[scala]
+    ProblemFilters.exclude[MissingClassProblem]("scala.runtime.PStatics$"), // private[scala]
+
   )
 
   override val buildSettings = Seq(

src/library/scala/collection/IterableOnce.scala

@@ -275,6 +275,13 @@ object IterableOnce {
       case src: Iterable[A] => src.copyToArray[B](xs, start, len)
       case src              => src.iterator.copyToArray[B](xs, start, len)
     }
+
+  @inline private[collection] def checkArraySizeWithinVMLimit(size: Int): Unit = {
+    import scala.runtime.PStatics.VM_MaxArraySize
+    if (size > VM_MaxArraySize) {
+      throw new Exception(s"Size of array-backed collection exceeds VM array size limit of ${VM_MaxArraySize}")
+    }
+  }
 }
 
 /** This implementation trait can be mixed into an `IterableOnce` to get the basic methods that are shared between

src/library/scala/collection/mutable/ArrayBuffer.scala

@@ -19,7 +19,6 @@ import java.util.Arrays
 import scala.annotation.nowarn
 import scala.collection.Stepper.EfficientSplit
 import scala.collection.generic.DefaultSerializable
-import scala.util.chaining._
 
 /** An implementation of the `Buffer` class using an array to
   *  represent the assembled sequence internally. Append, update and random
@@ -54,6 +53,7 @@ class ArrayBuffer[A] private (initialElements: Array[AnyRef], initialSize: Int)
 
   @transient private[this] var mutationCount: Int = 0
 
+  // needs to be `private[collection]` or `protected[collection]` for parallel-collections
   protected[collection] var array: Array[AnyRef] = initialElements
   protected var size0 = initialSize
 
@@ -69,6 +69,13 @@ class ArrayBuffer[A] private (initialElements: Array[AnyRef], initialSize: Int)
     array = ArrayBuffer.ensureSize(array, size0, n)
   }
 
+  // TODO 3.T: should be `protected`, perhaps `protected[this]`
+  /** Ensure that the internal array has at least `n` additional cells more than `size0`. */
+  private[mutable] def ensureAdditionalSize(n: Int): Unit = {
+    // `.toLong` to ensure `Long` arithmetic is used and prevent `Int` overflow
+    array = ArrayBuffer.ensureSize(array, size0, size0.toLong + n)
+  }
+
   def sizeHint(size: Int): Unit =
     if(size > length && size >= 1) ensureSize(size)
 
@@ -135,8 +142,8 @@ class ArrayBuffer[A] private (initialElements: Array[AnyRef], initialSize: Int)
 
   def addOne(elem: A): this.type = {
     mutationCount += 1
+    ensureAdditionalSize(1)
     val oldSize = size0
-    ensureSize(oldSize + 1)
     size0 = oldSize + 1
     this(oldSize) = elem
     this
@@ -149,7 +156,7 @@ class ArrayBuffer[A] private (initialElements: Array[AnyRef], initialSize: Int)
         val elemsLength = elems.size0
         if (elemsLength > 0) {
           mutationCount += 1
-          ensureSize(length + elemsLength)
+          ensureAdditionalSize(elemsLength)
           Array.copy(elems.array, 0, array, length, elemsLength)
           size0 = length + elemsLength
         }
@@ -161,7 +168,7 @@ class ArrayBuffer[A] private (initialElements: Array[AnyRef], initialSize: Int)
   def insert(@deprecatedName("n", "2.13.0") index: Int, elem: A): Unit = {
     checkWithinBounds(index, index)
     mutationCount += 1
-    ensureSize(size0 + 1)
+    ensureAdditionalSize(1)
     Array.copy(array, index, array, index + 1, size0 - index)
     size0 += 1
     this(index) = elem
@@ -179,9 +186,8 @@ class ArrayBuffer[A] private (initialElements: Array[AnyRef], initialSize: Int)
         val elemsLength = elems.size
         if (elemsLength > 0) {
           mutationCount += 1
+          ensureAdditionalSize(elemsLength)
           val len = size0
-          val newSize = len + elemsLength
-          ensureSize(newSize)
           Array.copy(array, index, array, index + elemsLength, len - index)
           // if `elems eq this`, this copy is safe because
           //   - `elems.array eq this.array`
@@ -191,7 +197,7 @@ class ArrayBuffer[A] private (initialElements: Array[AnyRef], initialSize: Int)
           //   - `System.arraycopy` will effectively "read" all the values before
           //     overwriting any of them when two arrays are the the same reference
           IterableOnce.copyElemsToArray(elems, array.asInstanceOf[Array[Any]], index, elemsLength)
-          size0 = newSize // update size AFTER the copy, in case we're inserting a proxy
+          size0 = len + elemsLength // update size AFTER the copy, in case we're inserting a proxy
         }
       case _ => insertAll(index, ArrayBuffer.from(elems))
     }
@@ -265,12 +271,13 @@ class ArrayBuffer[A] private (initialElements: Array[AnyRef], initialSize: Int)
 @SerialVersionUID(3L)
 object ArrayBuffer extends StrictOptimizedSeqFactory[ArrayBuffer] {
   final val DefaultInitialSize = 16
+  private[this] val emptyArray = new Array[AnyRef](0)
 
-  // Avoid reallocation of buffer if length is known.
   def from[B](coll: collection.IterableOnce[B]): ArrayBuffer[B] = {
     val k = coll.knownSize
     if (k >= 0) {
-      val array = new Array[AnyRef](k max DefaultInitialSize)
+      // Avoid reallocation of buffer if length is known
+      val array = ensureSize(emptyArray, 0, k) // don't duplicate sizing logic, and check VM array size limit
       IterableOnce.copyElemsToArray(coll, array.asInstanceOf[Array[Any]])
       new ArrayBuffer[B](array, k)
     }
@@ -284,33 +291,49 @@ object ArrayBuffer extends StrictOptimizedSeqFactory[ArrayBuffer] {
 
   def empty[A]: ArrayBuffer[A] = new ArrayBuffer[A]()
 
-  // if necessary, copy (n elements of) the array to a new array of capacity n.
-  // Should use Array.copyOf(array, resizeEnsuring(array.length, n))?
-  //
-  private def ensureSize(array: Array[AnyRef], end: Int, n: Int): Array[AnyRef] = {
-    def resizeEnsuring(length: Int, end: Int, n: Int): Int = {
-      var newSize: Long = length
-      newSize = math.max(newSize * 2, DefaultInitialSize)
-      while (newSize < n) newSize *= 2
-      if (newSize <= Int.MaxValue) newSize.toInt
-      else if (end == Int.MaxValue) throw new Exception(s"Collections can not have more than ${Int.MaxValue} elements")
-      else Int.MaxValue
+  /**
+   * @param arrayLen  the length of the backing array
+   * @param targetLen the minimum length to resize up to
+   * @return -1 if no resizing is needed, or the size for the new array otherwise
+   */
+  private def resizeUp(arrayLen: Long, targetLen: Long): Int = {
+    if (targetLen <= arrayLen) -1
+    else {
+      if (targetLen > Int.MaxValue) throw new Exception(s"Collections cannot have more than ${Int.MaxValue} elements")
+      IterableOnce.checkArraySizeWithinVMLimit(targetLen.toInt) // safe because `targetSize <= Int.MaxValue`
+
+      val newLen = math.max(targetLen, math.max(arrayLen * 2, DefaultInitialSize))
+      math.min(newLen, scala.runtime.PStatics.VM_MaxArraySize).toInt
     }
-    if (n <= array.length) array
-    else new Array[AnyRef](resizeEnsuring(array.length, end, n)).tap(Array.copy(array, 0, _, 0, end))
   }
-  private def downsize(array: Array[AnyRef], requiredLength: Int): Array[AnyRef] = {
-    def resizeDown(length: Int, requiredLength: Int): Int = {
-      var newSize: Long = length
-      // ensure that newSize is a power of 2 (this tweak is not motivated)
-      if (newSize == Int.MaxValue) newSize += 1
-      val minLength = math.max(requiredLength, DefaultInitialSize)
-      while (newSize / 2 >= minLength) newSize /= 2
-      if (newSize <= Int.MaxValue) newSize.toInt
-      else Int.MaxValue
+  // if necessary, copy (curSize elements of) the array to a new array of capacity n.
+  // Should use Array.copyOf(array, resizeEnsuring(array.length))?
+  private def ensureSize(array: Array[AnyRef], curSize: Int, targetSize: Long): Array[AnyRef] = {
+    val newLen = resizeUp(array.length, targetSize)
+    if (newLen < 0) array
+    else {
+      val res = new Array[AnyRef](newLen)
+      System.arraycopy(array, 0, res, 0, curSize)
+      res
+    }
+  }
+
+  /**
+   * @param arrayLen  the length of the backing array
+   * @param targetLen the length to resize down to, if smaller than `arrayLen`
+   * @return -1 if no resizing is needed, or the size for the new array otherwise
+   */
+  private def resizeDown(arrayLen: Int, targetLen: Int): Int =
+    if (targetLen >= arrayLen) -1 else math.max(targetLen, 0)
+  private def downsize(array: Array[AnyRef], targetSize: Int): Array[AnyRef] = {
+    val newLen = resizeDown(array.length, targetSize)
+    if (newLen < 0) array
+    else if (newLen == 0) emptyArray
+    else {
+      val res = new Array[AnyRef](newLen)
+      System.arraycopy(array, 0, res, 0, targetSize)
+      res
     }
-    if (requiredLength >= array.length) array
-    else new Array[AnyRef](resizeDown(array.length, requiredLength)).tap(Array.copy(array, 0, _, 0, requiredLength))
   }
 }
 

src/library/scala/collection/mutable/PriorityQueue.scala

@@ -89,6 +89,7 @@ sealed class PriorityQueue[A](implicit val ord: Ordering[A])
     def p_size0_=(s: Int) = size0 = s
     def p_array = array
     def p_ensureSize(n: Int) = super.ensureSize(n)
+    def p_ensureAdditionalSize(n: Int) = super.ensureAdditionalSize(n)
     def p_swap(a: Int, b: Int): Unit = {
       val h = array(a)
       array(a) = array(b)
@@ -153,7 +154,7 @@ sealed class PriorityQueue[A](implicit val ord: Ordering[A])
     *  @return             this $coll.
     */
   def addOne(elem: A): this.type = {
-    resarr.p_ensureSize(resarr.p_size0 + 1)
+    resarr.p_ensureAdditionalSize(1)
     resarr.p_array(resarr.p_size0) = elem.asInstanceOf[AnyRef]
     fixUp(resarr.p_array, resarr.p_size0)
     resarr.p_size0 += 1
@@ -170,7 +171,7 @@ sealed class PriorityQueue[A](implicit val ord: Ordering[A])
   private def unsafeAdd(elem: A): Unit = {
     // like += but skips fixUp, which breaks the ordering invariant
     // a series of unsafeAdds MUST be followed by heapify
-    resarr.p_ensureSize(resarr.p_size0 + 1)
+    resarr.p_ensureAdditionalSize(1)
     resarr.p_array(resarr.p_size0) = elem.asInstanceOf[AnyRef]
     resarr.p_size0 += 1
   }

src/library/scala/runtime/PStatics.scala

@@ -0,0 +1,19 @@
+/*
+ * Scala (https://www.scala-lang.org)
+ *
+ * Copyright EPFL and Lightbend, Inc.
+ *
+ * Licensed under Apache License 2.0
+ * (http://www.apache.org/licenses/LICENSE-2.0).
+ *
+ * See the NOTICE file distributed with this work for
+ * additional information regarding copyright ownership.
+ */
+
+package scala.runtime
+
+// things that should be in `Statics`, but can't be yet for bincompat reasons
+// TODO 3.T: move to `Statics`
+private[scala] object PStatics {
+  final val VM_MaxArraySize = 2147483645 // == `Int.MaxValue - 2`, hotspot limit
+}

test/junit/scala/collection/mutable/ArrayBufferTest.scala

@@ -3,6 +3,7 @@ package scala.collection.mutable
 import org.junit.Test
 import org.junit.Assert.{assertEquals, assertTrue}
 
+import java.lang.reflect.InvocationTargetException
 import scala.annotation.nowarn
 import scala.tools.testkit.AssertUtil.{assertSameElements, assertThrows, fail}
 import scala.tools.testkit.ReflectUtil.{getMethodAccessible, _}
@@ -358,7 +359,7 @@ class ArrayBufferTest {
     assertEquals(1024, b.array.length)
     b.remove(200, 803)
     b.trimToSize()
-    assertEquals(256, b.array.length)
+    assertEquals(200, b.array.length)
   }
 
   @Test def t11482_allowNegativeInitialSize(): Unit =
@@ -383,16 +384,33 @@ class ArrayBufferTest {
     iiobe( newAB.remove(2, 2), "3 is out of bounds (min 0, max 2)" )
   }
 
-  @Test def `t7880 ensureSize must terminate`(): Unit = {
-    val sut = getMethodAccessible[ArrayBuffer.type]("resizeEnsuring")
-    def resizeEnsuring(length: Int, end: Int, n: Int): Int = sut.invoke(ArrayBuffer, length, end, n).asInstanceOf[Int]
+  // scala/bug#7880 and scala/bug#12464
+  @Test def `ensureSize must terminate and have limits`(): Unit = {
+    val sut = getMethodAccessible[ArrayBuffer.type]("resizeUp")
+    def resizeUp(arrayLen: Long, targetLen: Long): Int = sut.invoke(ArrayBuffer, arrayLen, targetLen).asInstanceOf[Int]
+
+    // check termination and correctness
     assertTrue(7 < ArrayBuffer.DefaultInitialSize)  // condition of test
-    assertEquals(ArrayBuffer.DefaultInitialSize, resizeEnsuring(7, 3, 10))
-    assertEquals(Int.MaxValue - 1, resizeEnsuring(Int.MaxValue / 2, 3, Int.MaxValue / 2 + 1))  // was: ok
-    assertEquals(Int.MaxValue - 1, resizeEnsuring(Int.MaxValue / 2, 3, Int.MaxValue / 2 + 2))  // was: ok
-    assertEquals(Int.MaxValue, resizeEnsuring(Int.MaxValue / 2 + 1, 3, Int.MaxValue / 2 + 1))  // was: wrong
-    assertEquals(Int.MaxValue, resizeEnsuring(Int.MaxValue / 2 + 1, 3, Int.MaxValue / 2 + 2))  // was: hang
-    assertEquals(Int.MaxValue, resizeEnsuring(Int.MaxValue / 2, 3, Int.MaxValue))  // was: hang
+    assertEquals(ArrayBuffer.DefaultInitialSize, resizeUp(7, 10))
+    assertEquals(Int.MaxValue - 2, resizeUp(Int.MaxValue / 2, Int.MaxValue / 2 + 1))  // was: ok
+    assertEquals(Int.MaxValue - 2, resizeUp(Int.MaxValue / 2, Int.MaxValue / 2 + 2))  // was: ok
+    assertEquals(-1, resizeUp(Int.MaxValue / 2 + 1, Int.MaxValue / 2 + 1))  // was: wrong
+    assertEquals(Int.MaxValue - 2, resizeUp(Int.MaxValue / 2 + 1, Int.MaxValue / 2 + 2))  // was: hang
+    assertEquals(Int.MaxValue - 2, resizeUp(Int.MaxValue / 2, Int.MaxValue - 2))
+
+    // check limits
+    def rethrow(op: => Any): Unit =
+      try op catch { case e: InvocationTargetException => throw e.getCause }
+    def checkExceedsMaxInt(targetLen: Long): Unit =
+      assertThrows[Exception](rethrow(resizeUp(0, targetLen)),
+                              _ == "Collections cannot have more than 2147483647 elements")
+    def checkExceedsVMArrayLimit(targetLen: Long): Unit =
+      assertThrows[Exception](rethrow(resizeUp(0, targetLen)),
+                              _ == "Size of array-backed collection exceeds VM array size limit of 2147483645")
+
+    checkExceedsMaxInt(Int.MaxValue + 1L)
+    checkExceedsVMArrayLimit(Int.MaxValue)
+    checkExceedsVMArrayLimit(Int.MaxValue - 1)
   }
 
   @Test def `array capacity must follow sizing`(): Unit = {
@@ -404,48 +422,62 @@ class ArrayBufferTest {
       val buf = new ArrayBuffer(42)
       assertEquals(42, array(buf).length)
     }
-    // hint grows to a power of 2
+    // hint over 2x grows to exact
     locally {
-      val buf = ArrayBuffer(42)
+      val buf = new ArrayBuffer(42)
       buf.sizeHint(100)
-      assertEquals(128, array(buf).length)
+      assertEquals(100, array(buf).length)
+    }
+    // hint under 2x grows by 2x
+    locally {
+      val buf = new ArrayBuffer(42)
+      buf.sizeHint(64)
+      assertEquals(84, array(buf).length)
     }
     locally {
       val buf = new ArrayBuffer[Int](42)
       Iterator.continually(42).take(42).foreach(buf.addOne)
       buf.remove(buf.size - 1)
       assertEquals(42, array(buf).length)
     }
-    // not a power of 2, just double capacity
+    // double capacity
     locally {
       val buf = new ArrayBuffer[Int](42)
       Iterator.continually(42).take(42).foreach(buf.addOne)
       buf.addOne(17)
       assertEquals(84, array(buf).length)
     }
+    // exact capacity larger than double
+    locally {
+      val buf = new ArrayBuffer[Int](42)
+      Iterator.continually(42).take(42).foreach(buf.addOne)
+      buf.addAll(ArrayBuffer.from(1 to 43))
+      assertEquals(85, array(buf).length)
+    }
     locally {
       val buf = new ArrayBuffer[Int](42)
       Iterator.continually(42).take(42).foreach(buf.addOne)
       buf.clearAndShrink(42)
       assertEquals(42, array(buf).length)
     }
-    // not a power of 2, just halved capacity
+    // exact shrink
     locally {
       val buf = new ArrayBuffer[Int](42)
       Iterator.continually(42).take(42).foreach(buf.addOne)
       buf.clearAndShrink(17)
-      assertEquals(21, array(buf).length)
+      assertEquals(17, array(buf).length)
     }
   }
 
   @Test def `downsizing must size down`(): Unit = {
     val sut = getMethodAccessible[ArrayBuffer.type]("resizeDown")
-    def resizeDown(length: Int, n: Int): Int = sut.invoke(ArrayBuffer, length, n).asInstanceOf[Int]
+    def resizeDown(arrayLen: Int, targetLen: Int): Int = sut.invoke(ArrayBuffer, arrayLen, targetLen).asInstanceOf[Int]
     assertTrue(17 > ArrayBuffer.DefaultInitialSize)  // condition of test
-    //assertEquals(ArrayBuffer.DefaultInitialSize, resizeDown(17, 3))  // unexpectedly 17 not 16
-    assertEquals(17, resizeDown(17, 3))
-    assertEquals(32, resizeDown(64, 30))
-    assertEquals(21, resizeDown(42, 17))
+    assertEquals(3, resizeDown(17, 3))
+    assertEquals(30, resizeDown(64, 30))
+    assertEquals(17, resizeDown(42, 17))
+    assertEquals(-1, resizeDown(42, 50))
+    assertEquals(0, resizeDown(42, -1))
   }
 
   // scala/bug#12121