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] 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 = true
as.unsafeArray.getClass == classOf[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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
anything that creates a vector from a generic context with varargs will be an ArraySeq
wrapping an 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.
Ping @joshlemer, wdyt?
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.
@@ -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 = { |
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.
t
?
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not it.copyToArray
to benefit from fast array copying for array-based collections?
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.
Merging this now so we can get on with the 2.13.1 release. I see some room for optimization but we have #8276 for that and everything here looks safe.
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.