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

joshlemer · 2019-07-28T16:39:32Z

This is an alternative approach to #8194 which fixes scala/bug#11600.
This approach is different because in this PR we do not abandon the reusing of ArraySeq#unsafeArray's, we instead continue to reuse ArraySeq#unsafeArray's, and then at the time of updating, we special-case the cloning of display0 so that instead of calling display0.clone() we manually copy the elements into an Array[AnyRef] which will be safe to write any element into.

NthPortal · 2019-07-28T19:54:38Z

src/library/scala/collection/immutable/Vector.scala

+          v.display0 = as.unsafeArray match {
+            case array: Array[AnyRef] => array
+            case array =>
+              val display0 = new Array[AnyRef](array.length)


probably simpler to call toArray rather than manually copying

case _ => as.toArray[AnyRef]

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

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

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

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

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

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

src/library/scala/collection/immutable/Vector.scala

szeiger · 2019-08-29T15:16:09Z

I'm not too familiar with the details of the Vector problem but as far as I can tell this is pretty much ready to go, except for the MiMa errors which can be safely whitelisted.

@joshlemer Should we merge this for 2.13.1 or merge #8194 for now and keep this one around for later?

joshlemer · 2019-08-29T19:33:53Z

src/library/scala/collection/immutable/Vector.scala

@@ -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] = {


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

joshlemer · 2019-08-29T19:35:30Z

src/library/scala/collection/immutable/Vector.scala

@@ -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] = {


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

joshlemer · 2019-09-03T13:37:02Z

@szeiger I do think that this is the better solution, between the two, but definitely think someone with an eye for the Vector impl should give the code a look-over before merging.

szeiger

@Ichoran Can you take a look at this PR?

szeiger · 2019-09-04T13:06:54Z

src/library/scala/collection/immutable/Vector.scala

+          v.display0 = as.unsafeArray match {
+            case array: Array[AnyRef] => array
+            case array =>
+              val display0 = new Array[AnyRef](array.length)


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

szeiger · 2019-09-04T14:19:59Z

MiMa errors look safe for whitelisting:

[error]  * method copyRange(Array[java.lang.Object],java.lang.Class,Int,Int)Array[java.lang.Object] in class scala.collection.immutable.VectorBuilder does not have a correspondent in other version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.VectorBuilder.copyRange")
[error]  * method copyRange(Array[java.lang.Object],java.lang.Class,Int,Int)Array[java.lang.Object] in class scala.collection.immutable.VectorIterator does not have a correspondent in other version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.VectorIterator.copyRange")
[error]  * method copyRange(Array[java.lang.Object],java.lang.Class,Int,Int)Array[java.lang.Object] in class scala.collection.immutable.Vector does not have a correspondent in other version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.Vector.copyRange")
[error]  * method copyRange(Array[java.lang.Object],java.lang.Class,Int,Int)Array[java.lang.Object] in interface scala.collection.immutable.VectorPointer does not have a correspondent in other version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.immutable.VectorPointer.copyRange")

joshlemer · 2019-12-12T14:03:29Z

Superseded by #8534

scala-jenkins added this to the 2.13.1 milestone Jul 28, 2019

joshlemer mentioned this pull request Jul 28, 2019

[#11600] Vector.from(ArraySeq) copies elems rather than reusing unsafeArray #8194

Merged

diesalbla added the library:collections PRs involving changes to the standard collection library label Jul 28, 2019

NthPortal reviewed Jul 28, 2019

View reviewed changes

src/library/scala/collection/immutable/Vector.scala Show resolved Hide resolved

NthPortal reviewed Jul 28, 2019

View reviewed changes

src/library/scala/collection/immutable/Vector.scala Outdated Show resolved Hide resolved

NthPortal reviewed Jul 28, 2019

View reviewed changes

src/library/scala/collection/immutable/Vector.scala Outdated Show resolved Hide resolved

joshlemer added the bug fix label Jul 28, 2019

szeiger reviewed Jul 31, 2019

View reviewed changes

src/library/scala/collection/immutable/Vector.scala Outdated Show resolved Hide resolved

joshlemer force-pushed the vector-updated-fix branch 2 times, most recently from 3295233 to 5a00a14 Compare July 31, 2019 21:05

[11600] Vectors safely clone display0 arrays on updates

7ea8315

joshlemer force-pushed the vector-updated-fix branch from 5a00a14 to 7ea8315 Compare July 31, 2019 21:06

joshlemer commented Aug 29, 2019

View reviewed changes

szeiger modified the milestones: 2.13.1, 2.13.2 Aug 30, 2019

joshlemer marked this pull request as ready for review September 3, 2019 13:41

szeiger reviewed Sep 4, 2019

View reviewed changes

joshlemer closed this Dec 12, 2019

SethTisue removed this from the 2.13.2 milestone Dec 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

joshlemer commented Jul 28, 2019

NthPortal Jul 28, 2019

joshlemer Jul 28, 2019

NthPortal Jul 29, 2019 •

edited

joshlemer Jul 29, 2019 •

edited

NthPortal Jul 29, 2019

joshlemer Jul 31, 2019

szeiger Sep 4, 2019

szeiger commented Aug 29, 2019

joshlemer Aug 29, 2019

joshlemer Aug 29, 2019 •

edited

joshlemer commented Sep 3, 2019

szeiger left a comment

szeiger Sep 4, 2019

szeiger commented Sep 4, 2019

joshlemer commented Dec 12, 2019

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

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

Conversation

joshlemer commented Jul 28, 2019

NthPortal Jul 28, 2019

Choose a reason for hiding this comment

joshlemer Jul 28, 2019

Choose a reason for hiding this comment

NthPortal Jul 29, 2019 • edited

Choose a reason for hiding this comment

joshlemer Jul 29, 2019 • edited

Choose a reason for hiding this comment

NthPortal Jul 29, 2019

Choose a reason for hiding this comment

joshlemer Jul 31, 2019

Choose a reason for hiding this comment

szeiger Sep 4, 2019

Choose a reason for hiding this comment

szeiger commented Aug 29, 2019

joshlemer Aug 29, 2019

Choose a reason for hiding this comment

joshlemer Aug 29, 2019 • edited

Choose a reason for hiding this comment

joshlemer commented Sep 3, 2019

szeiger left a comment

Choose a reason for hiding this comment

szeiger Sep 4, 2019

Choose a reason for hiding this comment

szeiger commented Sep 4, 2019

joshlemer commented Dec 12, 2019

NthPortal Jul 29, 2019 •

edited

joshlemer Jul 29, 2019 •

edited

joshlemer Aug 29, 2019 •

edited