Skip to content

Fixed NPE in VectorStepper due to dirty display vector. #117

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

Merged
merged 1 commit into from
Feb 20, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,19 @@
public class CollectionInternals {
public static <A> Object[] getTable(scala.collection.mutable.FlatHashTable<A> fht) { return fht.hashTableContents().table(); }
public static <A, E extends scala.collection.mutable.HashEntry<A,E>> scala.collection.mutable.HashEntry<A, E>[] getTable(scala.collection.mutable.HashTable<A,E> ht) { return ht.hashTableContents().table(); }
public static <A> boolean getDirt(scala.collection.immutable.Vector<A> v) { return v.dirty(); }
public static <A> Object[] getDisplay0(scala.collection.immutable.Vector<A> v) { return v.display0(); }
public static <A> Object[] getDisplay0(scala.collection.immutable.VectorIterator<A> p) { return p.display0(); }
public static <A> Object[] getDisplay1(scala.collection.immutable.Vector<A> v) { return v.display1(); }
public static <A> Object[] getDisplay1(scala.collection.immutable.VectorIterator<A> p) { return p.display1(); }
public static <A> Object[] getDisplay2(scala.collection.immutable.Vector<A> v) { return v.display2(); }
public static <A> Object[] getDisplay2(scala.collection.immutable.VectorIterator<A> p) { return p.display2(); }
public static <A> Object[] getDisplay3(scala.collection.immutable.Vector<A> v) { return v.display3(); }
public static <A> Object[] getDisplay3(scala.collection.immutable.VectorIterator<A> p) { return p.display3(); }
public static <A> Object[] getDisplay4(scala.collection.immutable.Vector<A> v) { return v.display4(); }
public static <A> Object[] getDisplay4(scala.collection.immutable.VectorIterator<A> p) { return p.display4(); }
public static <A> Object[] getDisplay5(scala.collection.immutable.Vector<A> v) { return v.display5(); }
public static <A> Object[] getDisplay5(scala.collection.immutable.VectorIterator<A> p) { return p.display5(); }

Choose a reason for hiding this comment

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

Could all these duplicates just take a scala.collection.immutable.VectorPointer<A>?

Choose a reason for hiding this comment

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

oh I see

// (Manually specialized this way for speed.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it cuts off about 20% of the runtime in my ad-hoc testing.

public static <A> scala.Tuple2< scala.Tuple2< scala.collection.Iterator<A>, Object >, scala.collection.Iterator<A> > trieIteratorSplit(scala.collection.Iterator<A> it) {
if (it instanceof scala.collection.immutable.TrieIterator) {
scala.collection.immutable.TrieIterator<A> trie = (scala.collection.immutable.TrieIterator<A>)it;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,16 +32,19 @@ private[java8] abstract class StepsLikeIndexed[A, STA >: Null <: StepsLikeIndexe
private[java8] abstract class StepsDoubleLikeIndexed[STD >: Null <: StepsDoubleLikeIndexed[_]](_i0: Int, _iN: Int)
extends AbstractStepsLikeIndexed[DoubleStepper, STD](_i0, _iN)
with DoubleStepper
with java.util.Spliterator.OfDouble // Compiler wants this for mixin forwarder
{}

/** Abstracts the operation of stepping over an indexable collection of Ints */
private[java8] abstract class StepsIntLikeIndexed[STI >: Null <: StepsIntLikeIndexed[_]](_i0: Int, _iN: Int)
extends AbstractStepsLikeIndexed[IntStepper, STI](_i0, _iN)
with IntStepper
with java.util.Spliterator.OfInt // Compiler wants this for mixin forwarder
{}

/** Abstracts the operation of stepping over an indexable collection of Longs */
private[java8] abstract class StepsLongLikeIndexed[STL >: Null <: StepsLongLikeIndexed[_]](_i0: Int, _iN: Int)
extends AbstractStepsLikeIndexed[LongStepper, STL](_i0, _iN)
with LongStepper
with java.util.Spliterator.OfLong // Compiler wants this for mixin forwarder
{}
58 changes: 52 additions & 6 deletions src/main/scala/scala/compat/java8/converterImpl/StepsVector.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package scala.compat.java8.converterImpl

import scala.annotation.switch

import scala.collection.immutable.VectorIterator

import scala.compat.java8.collectionImpl._
import scala.compat.java8.runtime._

Expand All @@ -13,20 +15,27 @@ import Stepper._

private[java8] trait StepsVectorLike[A] {
protected def myVector: Vector[A]
protected def myVectorIterator: VectorIterator[A]
protected def myVectorLength: Int
protected var index: Int = 32
protected var data: Array[AnyRef] = null
protected var index1: Int = 32
protected var data1: Array[AnyRef] = null
protected final def advanceData(iX: Int): Unit = {
index1 += 1
if (index >= 32) initTo(iX)
if (index >= 32) {
if (myVector != null) initTo(iX)
else initVpTo(iX)
}
else {
data = data1(index1).asInstanceOf[Array[AnyRef]]
index = 0
}
}
protected final def initTo(iX: Int): Unit = {
myVector.length match {
// WARNING--initVpTo is an exact copy of this except for the type! If you change one you must change the other!
// (Manually specialized this way for speed.)
myVectorLength match {
case x if x <= 0x20 =>
index = iX
data = CollectionInternals.getDisplay0(myVector)
Expand All @@ -52,12 +61,43 @@ private[java8] trait StepsVectorLike[A] {
data = data1(index1).asInstanceOf[Array[AnyRef]]
}
}
protected final def initVpTo(iX: Int): Unit = {
// WARNING--this is an exact copy of initTo! If you change one you must change the other!
// (Manually specialized this way for speed.)
myVectorLength match {
case x if x <= 0x20 =>
index = iX
data = CollectionInternals.getDisplay0(myVectorIterator)
case x if x <= 0x400 =>
index1 = iX >>> 5
data1 = CollectionInternals.getDisplay1(myVectorIterator)
index = iX & 0x1F
data = data1(index1).asInstanceOf[Array[AnyRef]]
case x =>
var N = 0
var dataN: Array[AnyRef] =
if (x <= 0x8000) { N = 2; CollectionInternals.getDisplay2(myVectorIterator) }
else if (x <= 0x100000) { N = 3; CollectionInternals.getDisplay3(myVectorIterator) }
else if (x <= 0x2000000) { N = 4; CollectionInternals.getDisplay4(myVectorIterator) }
else /*x <= 0x40000000*/{ N = 5; CollectionInternals.getDisplay5(myVectorIterator) }
while (N > 2) {
dataN = dataN((iX >>> (5*N))&0x1F).asInstanceOf[Array[AnyRef]]
N -= 1
}
index1 = (iX >>> 5) & 0x1F
data1 = dataN((iX >>> 10) & 0x1F).asInstanceOf[Array[AnyRef]]
index = iX & 0x1F
data = data1(index1).asInstanceOf[Array[AnyRef]]
}
}
}

private[java8] class StepsAnyVector[A](underlying: Vector[A], _i0: Int, _iN: Int)
extends StepsLikeIndexed[A, StepsAnyVector[A]](_i0, _iN)
with StepsVectorLike[A] {
protected def myVector = underlying
protected val myVector = if (CollectionInternals.getDirt(underlying)) null else underlying
protected val myVectorIterator = if (myVector == null) underlying.iterator else null
protected val myVectorLength = underlying.length
def next() = if (hasNext()) {
index += 1
if (index >= 32) advanceData(i0)
Expand All @@ -76,7 +116,9 @@ with StepsVectorLike[A] {
private[java8] class StepsDoubleVector(underlying: Vector[Double], _i0: Int, _iN: Int)
extends StepsDoubleLikeIndexed[StepsDoubleVector](_i0, _iN)
with StepsVectorLike[Double] {
protected def myVector = underlying
protected val myVector = if (CollectionInternals.getDirt(underlying)) null else underlying
protected val myVectorIterator = if (myVector == null) underlying.iterator else null
protected val myVectorLength = underlying.length
def nextDouble() = if (hasNext()) {
index += 1
if (index >= 32) advanceData(i0)
Expand All @@ -95,7 +137,9 @@ with StepsVectorLike[Double] {
private[java8] class StepsIntVector(underlying: Vector[Int], _i0: Int, _iN: Int)
extends StepsIntLikeIndexed[StepsIntVector](_i0, _iN)
with StepsVectorLike[Int] {
protected def myVector = underlying
protected val myVector = if (CollectionInternals.getDirt(underlying)) null else underlying
protected val myVectorIterator = if (myVector == null) underlying.iterator else null
protected val myVectorLength = underlying.length
def nextInt() = if (hasNext()) {
index += 1
if (index >= 32) advanceData(i0)
Expand All @@ -114,7 +158,9 @@ with StepsVectorLike[Int] {
private[java8] class StepsLongVector(underlying: Vector[Long], _i0: Int, _iN: Int)
extends StepsLongLikeIndexed[StepsLongVector](_i0, _iN)
with StepsVectorLike[Long] {
protected def myVector = underlying
protected val myVector = if (CollectionInternals.getDirt(underlying)) null else underlying
protected val myVectorIterator = if (myVector == null) underlying.iterator else null
protected val myVectorLength = underlying.length
def nextLong() = if (hasNext()) {
index += 1
if (index >= 32) advanceData(i0)
Expand Down
28 changes: 28 additions & 0 deletions src/test/scala/scala/compat/java8/StreamConvertersTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -276,4 +276,32 @@ class StreamConvertersTest {
val stepper2 = steppize2(coll2).stepper
assertTrue(stepper2.getClass.getName.contains("StepsIntVector"))
}

@Test
def issue_87(): Unit = {
// Vectors that are generated from other vectors tend _not_ to
// have all their display vectors consistent; the cached vectors
// are correct, but the higher-level vector does _not_ contain
// the cached vector in the correct place (for efficiency)! This
// is called being "dirty", and needs to be handled specially.
val dirtyDisplayVector = Vector.fill(120)("a").slice(0, 40)
val shouldNotNPE =
dirtyDisplayVector.seqStream.collect(Collectors.toList())
assertEq(shouldNotNPE.toArray(new Array[String](0)).toVector, dirtyDisplayVector, "Vector[Any].seqStream (with dirty display)")

val dirtyDisplayVectorInt = Vector.fill(120)(999).slice(0, 40)
val shouldNotNPEInt =
dirtyDisplayVectorInt.seqStream.sum()
assertEq(shouldNotNPEInt, dirtyDisplayVectorInt.sum, "Vector[Int].seqStream (with dirty display)")

val dirtyDisplayVectorLong = Vector.fill(120)(99999999999L).slice(0, 40)
val shouldNotNPELong =
dirtyDisplayVectorLong.seqStream.sum()
assertEq(shouldNotNPELong, dirtyDisplayVectorLong.sum, "Vector[Long].seqStream (with dirty display)")

val dirtyDisplayVectorDouble = Vector.fill(120)(0.1).slice(0, 40)
val shouldNotNPEDouble =
math.rint(dirtyDisplayVectorDouble.seqStream.sum() * 10)
assertEq(shouldNotNPEDouble, math.rint(dirtyDisplayVectorDouble.sum * 10), "Vector[Double].seqStream (with dirty display)")
}
}