Skip to content

Commit

Permalink
[11600] Vectors safely clone display0 arrays on updates
Browse files Browse the repository at this point in the history
  • Loading branch information
Josh Lemer authored and Josh Lemer committed Jul 31, 2019
1 parent 4854c5d commit 5a00a14
Show file tree
Hide file tree
Showing 2 changed files with 84 additions and 20 deletions.
61 changes: 41 additions & 20 deletions src/library/scala/collection/immutable/Vector.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ package scala
package collection
package immutable

import java.util

import scala.collection.Stepper.EfficientSplit
import scala.collection.generic.DefaultSerializable
import scala.collection.mutable.ReusableBuilder
Expand All @@ -30,11 +32,21 @@ object Vector extends StrictOptimizedSeqFactory[Vector] {

def from[E](it: collection.IterableOnce[E]): Vector[E] =
it match {
case as: ArraySeq[E] if as.length <= 32 && as.unsafeArray.isInstanceOf[Array[AnyRef]] =>
case as: ArraySeq[E] if as.length <= 32 =>
if (as.isEmpty) NIL
else {
val v = new Vector(0, as.length, 0)
v.display0 = as.unsafeArray.asInstanceOf[Array[AnyRef]]
v.display0 = as.unsafeArray match {
case array: Array[AnyRef] => array
case array =>
val display0 = new Array[AnyRef](array.length)
var i = 0
while (i < array.length) {
display0(i) = array(i).asInstanceOf[AnyRef]
i += 1
}
display0
}
v.depth = 1
releaseFence()
v
Expand Down Expand Up @@ -545,7 +557,7 @@ final class Vector[+A] private[immutable] (private[collection] val startIndex: I
// low-level implementation (needs cleanup, maybe move to util class)

private def shiftTopLevel(oldLeft: Int, newLeft: Int) = (depth - 1) match {
case 0 => display0 = copyRange(display0, oldLeft, newLeft)
case 0 => display0 = copyRange(display0, classOf[AnyRef], oldLeft, newLeft)
case 1 => display1 = copyRange(display1, oldLeft, newLeft)
case 2 => display2 = copyRange(display2, oldLeft, newLeft)
case 3 => display3 = copyRange(display3, oldLeft, newLeft)
Expand Down Expand Up @@ -1146,7 +1158,7 @@ private[immutable] trait VectorPointer[+T] {
* 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[T0 <: AnyRef](array: Array[Array[T0]], index: Int, destination: Array[T0] = null): Array[T0] = {
val x = array(index)
array(index) = null
if (destination == null) x.clone()
Expand All @@ -1156,7 +1168,14 @@ private[immutable] trait VectorPointer[+T] {
}
}

// make sure there is no aliasing
private final def nullSlotAndCopy0(array: Array[Array[AnyRef]], index: Int, destination: Array[AnyRef] = null): Array[AnyRef] = {
val x = array(index)
array(index) = null
val dest = if (destination == null) new Array[AnyRef](x.length) else destination
System.arraycopy(x, 0, dest, 0, x.length)
dest
}

// requires structure is at pos index
// ensures structure is clean and at pos index and writable at all levels except 0

Expand Down Expand Up @@ -1213,45 +1232,45 @@ 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)
display0 = nullSlotAndCopy0(display1, (newIndex >>> 5) & 31)
case 4 =>
display4 = display4.clone()
display3 = nullSlotAndCopy(display4, (newIndex >>> 20) & 31)
display2 = nullSlotAndCopy(display3, (newIndex >>> 15) & 31)
display1 = nullSlotAndCopy(display2, (newIndex >>> 10) & 31)
display0 = nullSlotAndCopy(display1, (newIndex >>> 5) & 31)
display0 = nullSlotAndCopy0(display1, (newIndex >>> 5) & 31)
case 3 =>
display3 = display3.clone()
display2 = nullSlotAndCopy(display3, (newIndex >>> 15) & 31)
display1 = nullSlotAndCopy(display2, (newIndex >>> 10) & 31)
display0 = nullSlotAndCopy(display1, (newIndex >>> 5) & 31)
display0 = nullSlotAndCopy0(display1, (newIndex >>> 5) & 31)
case 2 =>
display2 = display2.clone()
display1 = nullSlotAndCopy(display2, (newIndex >>> 10) & 31)
display0 = nullSlotAndCopy(display1, (newIndex >>> 5) & 31)
display0 = nullSlotAndCopy0(display1, (newIndex >>> 5) & 31)
case 1 =>
display1 = display1.clone()
display0 = nullSlotAndCopy(display1, (newIndex >>> 5) & 31)
display0 = nullSlotAndCopy0(display1, (newIndex >>> 5) & 31)
case 0 =>
display0 = display0.clone()
display0 = util.Arrays.copyOf(display0, display0.length, classOf[Array[AnyRef]])
}

// 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 = {
if (xor < (1 << 5)) { // level = 0
display0 = display0.clone()
display0 = util.Arrays.copyOf(display0, display0.length, classOf[Array[AnyRef]])
} else if (xor < (1 << 10)) { // level = 1
display1 = display1.clone()
display1((oldIndex >>> 5) & 31) = display0
display0 = nullSlotAndCopy(display1, (newIndex >>> 5) & 31)
display0 = nullSlotAndCopy0(display1, (newIndex >>> 5) & 31)
} else if (xor < (1 << 15)) { // level = 2
display1 = display1.clone()
display2 = display2.clone()
display1((oldIndex >>> 5) & 31) = display0
display2((oldIndex >>> 10) & 31) = display1
display1 = nullSlotAndCopy(display2, (newIndex >>> 10) & 31)
display0 = nullSlotAndCopy(display1, (newIndex >>> 5) & 31, reuseDisplay0)
display0 = nullSlotAndCopy0(display1, (newIndex >>> 5) & 31, reuseDisplay0)
} else if (xor < (1 << 20)) { // level = 3
display1 = display1.clone()
display2 = display2.clone()
Expand All @@ -1261,7 +1280,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 = nullSlotAndCopy0(display1, (newIndex >>> 5) & 31, reuseDisplay0)
} else if (xor < (1 << 25)) { // level = 4
display1 = display1.clone()
display2 = display2.clone()
Expand All @@ -1274,7 +1293,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 = nullSlotAndCopy0(display1, (newIndex >>> 5) & 31, reuseDisplay0)
} else if (xor < (1 << 30)) { // level = 5
display1 = display1.clone()
display2 = display2.clone()
Expand All @@ -1290,21 +1309,23 @@ 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 = nullSlotAndCopy0(display1, (newIndex >>> 5) & 31, reuseDisplay0)
} else { // level = 6
throw new IllegalArgumentException()
}
}


// USED IN DROP

private[immutable] final def copyRange[T <: AnyRef](array: Array[T], oldLeft: Int, newLeft: Int) = {
val elems = java.lang.reflect.Array.newInstance(array.getClass.getComponentType, 32).asInstanceOf[Array[T]]
private[immutable] final def copyRange[T <: AnyRef](array: Array[T], componentType: Class[_], oldLeft: Int, newLeft: Int): Array[T] = {
val elems = java.lang.reflect.Array.newInstance(componentType, 32).asInstanceOf[Array[T]]
java.lang.System.arraycopy(array, oldLeft, elems, newLeft, 32 - Math.max(newLeft, oldLeft))
elems
}

private[immutable] final def copyRange[T <: AnyRef](array: Array[T], oldLeft: Int, newLeft: Int): Array[T] =
copyRange(array, array.getClass.getComponentType, oldLeft, newLeft)


// USED IN APPEND
// create a new block at the bottom level (and possibly nodes on its path) and prepares for writing
Expand Down
43 changes: 43 additions & 0 deletions test/junit/scala/collection/immutable/VectorTest.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,4 +219,47 @@ class VectorTest {
assertArrayEquals(s"<${v2.length}>.slice($j, $k)", v2.toArray.slice(j, k), v2.iterator.slice(j, k).toArray)
}
}
@Test
def t11600(): Unit = {
locally {
abstract class Base
class Derived1 extends Base
class Derived2 extends Base
val d1 = new Derived1
val d2 = new Derived2

locally {
val arraySeq = ArraySeq(d1)
val vector = Vector(arraySeq: _*)
assertEquals(arraySeq, ArraySeq(d1)) // ensure arraySeq is not mutated
assertEquals(vector.updated(0, d2), Vector(d2))
}

locally {
val list = List(d1)
val vector = Vector.from(list)
assertEquals(list, vector)
assertEquals(List(d2), vector.updated(0, d2))
}
}

locally {
// ensure boxing logic works:
val arraySeq = ArraySeq(1,2,3,4,5)
val vector = Vector(arraySeq: _*)

assertEquals(1 to 5, vector)
assertEquals(vector.updated(0, 20), Vector(20,2,3,4,5))
assertEquals(vector.updated(0, ""), Vector("",2,3,4,5))
assertEquals(1 to 5, arraySeq) // ensure arraySeq is not mutated
}
locally {
// ensure boxing logic works:
val arr = Array(1)
val vector = Vector.from(arr)
assertEquals(arr.toList, vector)
assertEquals(List(20), vector.updated(0, 20))
assertEquals(List(""), vector.updated(0, ""))
}
}
}

0 comments on commit 5a00a14

Please sign in to comment.