[#11600] Vector.from(ArraySeq) copies elems rather than reusing unsafeArray #8194
Conversation
| @@ -30,11 +30,23 @@ 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]] => | |||
There was a problem hiding this comment.
we should be able to reuse the same array without copying if it's exactly Array[AnyRef]]
scala> new Array[String](0).getClass == classOf[Array[AnyRef]]
res2: Boolean = false
scala> new Array[AnyRef](0).getClass == classOf[Array[AnyRef]]
res3: Boolean = trueas.unsafeArray.getClass == classOf[Array[AnyRef]]There was a problem hiding this comment.
I thought about this, but thought this is a pretty niche scenario, who goes around calling Vector.apply[AnyRef](...)? An alternate approach would be to special case the underlying display0.clone() logic (which is hit by updated) to be display0.copyToArray(new Array[AnyRef](32)). This would allow us to keep reusing the unsafeArray. (and maybe even in the case of primitives?)
There was a problem hiding this comment.
anything that creates a vector from a generic context with varargs will be an ArraySeq wrapping an Array[AnyRef]
| @@ -219,4 +219,48 @@ class VectorTest { | |||
| assertArrayEquals(s"<${v2.length}>.slice($j, $k)", v2.toArray.slice(j, k), v2.iterator.slice(j, k).toArray) | |||
| } | |||
| } | |||
|
|
|||
| @Test | |||
| def i11600(): Unit = { | |||
unsafeArray There were issues with reusing the ArraySeq unsafeArray which arose in subsequent calls to Vector#updated which would then clone underlying arrays, and insert into them the new updated element. However the underlying array.clone() would return a new array with the same componentType as the passed varargs, rather than an Array[AnyRef] which is required in the general case.
diesalbla
added
the
library:collections
SethTisue
added
the
prio:blocker
|
@joshlemer With the subtle Vector problems that led to the other remaining open Vector PR in mind, I think we should merge this one and leave the more ambitious version for 2.13.2 rather than rushing it now. WDYT? |
| i += 1 | ||
| } | ||
| val display0 = new Array[Any](knownSize) | ||
| it.iterator.copyToArray(display0) |
There was a problem hiding this comment.
why not it.copyToArray to benefit from fast array copying for array-based collections?
Closes scala/bug#11600
Fixes scala/bug#11600
There were issues with reusing the ArraySeq unsafeArray which arose in
subsequent calls to Vector#updated which would then clone underlying
arrays, and insert into them the new updated element. However the
underlying array.clone() would return a new array with the same
componentType as the passed varargs, rather than an Array[AnyRef] which
is required in the general case.