-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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.
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
[11600] Vectors safely clone display0 arrays on updates #8276
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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 | ||
|
@@ -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) | ||
|
@@ -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] = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The change |
||
val x = array(index) | ||
array(index) = null | ||
if (destination == null) x.clone() | ||
|
@@ -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] = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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 | ||
|
@@ -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() | ||
|
@@ -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() | ||
|
@@ -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() | ||
|
@@ -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 | ||
|
There was a problem hiding this comment.
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 copyingThere was a problem hiding this comment.
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
, sodef toArray[B >: E]
won't work.There was a problem hiding this comment.
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]]
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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]
orArray.from[Any]
There was a problem hiding this comment.
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:
Array
that allows it to conform toIterableOnce
.iterator
There was a problem hiding this comment.
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.