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

[11600] Vectors safely clone display0 arrays on updates #8276

Closed
wants to merge 1 commit into from
Closed

[11600] Vectors safely clone display0 arrays on updates #8276

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
60 changes: 41 additions & 19 deletions src/library/scala/collection/immutable/Vector.scala
View file
Open in desktop
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

probably simpler to call toArray rather than manually copying

case _ => as.toArray[AnyRef]

Copy link
Member Author

Choose a reason for hiding this comment

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

sadly that won't compile because we don't have that E <: AnyRef, so def toArray[B >: E] won't work.

Copy link
Contributor

@NthPortal NthPortal Jul 29, 2019 •
edited

Choose a reason for hiding this comment

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

as.toArray[Any].asInstanceOf[Array[AnyRef]]

Copy link
Member Author

@joshlemer joshlemer Jul 29, 2019 •
edited

Choose a reason for hiding this comment

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

Hmm looked into that, seems it goes through a lot many intermediate method calls, with additional bounds checks and math checks, to eventually just do this exact thing, so I reckon this is faster but I could benchmark

Copy link
Contributor

Choose a reason for hiding this comment

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

you might also be able to do Array.from[AnyRef] or Array.from[Any]

Copy link
Member Author

Choose a reason for hiding this comment

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

That code path would make two additional allocs:

  • the implicitly created wrapper around the Array that allows it to conform to IterableOnce
  • the wrapper's .iterator

Copy link
Member

Choose a reason for hiding this comment

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

Array.copyAs is the right method for this task. Maybe the manually inlined version here is a bit faster.

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] = {
Copy link
Member Author

Choose a reason for hiding this comment

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

The change T -> T0 is simply to avoid shadowing which is going on between this T and the T in VectorPointer[T]

val x = array(index)
array(index) = null
if (destination == null) x.clone()
Expand All @@ -1156,6 +1168,14 @@ private[immutable] trait VectorPointer[+T] {
}
}

private final def nullSlotAndCopy0(array: Array[Array[AnyRef]], index: Int, destination: Array[AnyRef] = null): Array[AnyRef] = {
Copy link
Member Author

@joshlemer joshlemer Aug 29, 2019 •
edited

Choose a reason for hiding this comment

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

TODO: Include Scaladocs/Comments to explain that this is a specialized form of nullSlotAndCopy specialized for display0 which exists solely for avoiding use of array.clone() which may be a shared array with element type != java.lang.Object

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
}

// make sure there is no aliasing
// 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 +1233,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 +1281,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 +1294,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 +1310,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]]
joshlemer marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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, ""))
}
}
}