Skip to content
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

Chunk support for ArraySeq #1905

Merged
merged 5 commits into from
Jun 22, 2020
Merged
Show file tree
Hide file tree
Changes from 3 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
9 changes: 9 additions & 0 deletions core/shared/src/main/scala-2.12/fs2/ChunkPlatform.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package fs2

trait ChunkPlatform[+O] { self: Chunk[O] =>

}

trait ChunkCompanionPlatform { self: Chunk.type =>

}
10 changes: 0 additions & 10 deletions core/shared/src/main/scala-2.12/fs2/internal/ArrayBackedSeq.scala

This file was deleted.

41 changes: 41 additions & 0 deletions core/shared/src/main/scala-2.13/fs2/ChunkPlatform.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package fs2

import scala.collection.immutable.ArraySeq
import scala.collection.{immutable, mutable}
import scala.reflect.ClassTag

trait ChunkPlatform[+O] { self: Chunk[O] =>
Copy link
Member

Choose a reason for hiding this comment

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

Can you make this trait and it’s companion private[fs2] both here and in 2.13 file? Otherwise looks ready for merge. Thanks!


def toArraySeq[O2 >: O: ClassTag]: ArraySeq[O2] = {
val array: Array[O2] = new Array[O2](size)
copyToArray(array)
ArraySeq.unsafeWrapArray[O2](array)
Comment on lines +11 to +12
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to be the most efficient way to construct an ArraySeq, given it will often take advantage of System.arraycopy. I might be missing something, though, so I'm open to feedback if there are better ways to do this.

}

def toArraySeqUntagged: ArraySeq[O] =
self match {
case knownType: Chunk.KnownElementType[O] => toArraySeq[O](knownType.elementClassTag)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We attempt to specialise the ArraySeq even if there is no ClassTag available.

case _ =>
val buf = ArraySeq.untagged.newBuilder[O]
buf.sizeHint(size)
var i = 0
while (i < size) {
buf += apply(i)
i += 1
}
buf.result()
}

}

trait ChunkCompanionPlatform { self: Chunk.type =>

// The array casts below are safe because the array constructor will check for primative vs boxed arrays
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment replaces the now-deleted comment here in Chunk.


def arraySeq[O](arraySeq: immutable.ArraySeq[O]): Chunk[O] =
array(arraySeq.unsafeArray.asInstanceOf[Array[O]])

def arraySeq[O](arraySeq: mutable.ArraySeq[O]): Chunk[O] =
array(arraySeq.array.asInstanceOf[Array[O]])

}
10 changes: 0 additions & 10 deletions core/shared/src/main/scala-2.13/fs2/internal/ArrayBackedSeq.scala

This file was deleted.

15 changes: 6 additions & 9 deletions core/shared/src/main/scala/fs2/Chunk.scala
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import java.nio.{
import cats.{Alternative, Applicative, Eq, Eval, Monad, Traverse, TraverseFilter}
import cats.data.{Chain, NonEmptyList}
import cats.implicits._
import fs2.internal.ArrayBackedSeq

/**
* Strict, finite sequence of values that allows index-based random access of elements.
Expand All @@ -33,7 +32,7 @@ import fs2.internal.ArrayBackedSeq
* The operations on `Chunk` are all defined strictly. For example, `c.map(f).map(g).map(h)` results in
* intermediate chunks being created (1 per call to `map`).
*/
abstract class Chunk[+O] extends Serializable { self =>
abstract class Chunk[+O] extends Serializable with ChunkPlatform[O] { self =>

/** Returns the number of elements in this chunk. */
def size: Int
Expand Down Expand Up @@ -495,7 +494,7 @@ abstract class Chunk[+O] extends Serializable { self =>
iterator.mkString("Chunk(", ", ", ")")
}

object Chunk extends CollectorK[Chunk] {
object Chunk extends CollectorK[Chunk] with ChunkCompanionPlatform {

/** Optional mix-in that provides the class tag of the element type in a chunk. */
trait KnownElementType[A] { self: Chunk[A] =>
Expand Down Expand Up @@ -594,12 +593,10 @@ object Chunk extends CollectorK[Chunk] {
/** Creates a chunk from a `scala.collection.Iterable`. */
def iterable[O](i: collection.Iterable[O]): Chunk[O] =
i match {
case ArrayBackedSeq(arr) =>
// arr is either a primitive array or a boxed array
// cast is safe b/c the array constructor will check for primitive vs boxed arrays
array(arr.asInstanceOf[Array[O]])
case v: Vector[O] => vector(v)
case b: collection.mutable.Buffer[O] => buffer(b)
case a: collection.mutable.ArraySeq[O] => arraySeq(a)
case a: collection.immutable.ArraySeq[O] => arraySeq(a)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have replaced the previous implementation of this behaviour via ArrayBackedSeq (now deleted).

case v: Vector[O] => vector(v)
case b: collection.mutable.Buffer[O] => buffer(b)
case l: List[O] =>
if (l.isEmpty) empty
else if (l.tail.isEmpty) singleton(l.head)
Expand Down
106 changes: 106 additions & 0 deletions core/shared/src/test/scala-2.13/fs2/ChunkPlatformSpec.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
package fs2

import org.scalatest.prop.{CommonGenerators, Generator}

import scala.collection.immutable.ArraySeq
import scala.collection.{immutable, mutable}
import scala.reflect.ClassTag

class ChunkPlatformSpec extends Fs2Spec {

private implicit val genUnit: Generator[Unit] = CommonGenerators.specificValue(())
private implicit def genArraySeq[A: Generator: ClassTag]: Generator[ArraySeq[A]] =
Generator.vectorGenerator[A].map(ArraySeq.from)
private implicit def genMutableArraySeq[A: Generator: ClassTag]: Generator[mutable.ArraySeq[A]] =
implicitly[Generator[ArraySeq[A]]].map(_.to(mutable.ArraySeq))

private def testChunkToArraySeq[A](
testName: String,
shouldSpecialise: Boolean,
f: Chunk[A] => ArraySeq[A]
)(implicit generator: Generator[Chunk[A]]): Unit =
s"Chunk toArraySeq $testName" - {
"values" in forAll { chunk: Chunk[A] =>
assert(f(chunk).toVector == chunk.toVector)
}

if (shouldSpecialise)
"specialised" in forAll { chunk: Chunk[A] =>
f(chunk) match {
case _: ArraySeq.ofRef[A] =>
fail("Expected specialised ArraySeq but was not specialised")
case _ => succeed
}
}
}

private def testChunkToArraySeqTagged[A: ClassTag](testName: String, shouldSpecialise: Boolean)(
implicit generator: Generator[Chunk[A]]
): Unit =
testChunkToArraySeq[A](testName, shouldSpecialise, f = _.toArraySeq[A])

private def testChunkToArraySeqUntagged[A](
testName: String
)(implicit generator: Generator[Chunk[A]]): Unit =
testChunkToArraySeq[A](testName, shouldSpecialise = false, f = _.toArraySeqUntagged)

testChunkToArraySeqTagged[Int]("Int tagged", shouldSpecialise = true)
testChunkToArraySeqTagged[Double]("Double tagged", shouldSpecialise = true)
testChunkToArraySeqTagged[Long]("Long tagged", shouldSpecialise = true)
testChunkToArraySeqTagged[Float]("Float tagged", shouldSpecialise = true)
testChunkToArraySeqTagged[Char]("Char tagged", shouldSpecialise = true)
testChunkToArraySeqTagged[Byte]("Byte tagged", shouldSpecialise = true)
testChunkToArraySeqTagged[Short]("Short tagged", shouldSpecialise = true)
testChunkToArraySeqTagged[Boolean]("Boolean tagged", shouldSpecialise = true)
testChunkToArraySeqTagged[String]("String tagged", shouldSpecialise = false)
testChunkToArraySeqTagged[Unit]("Unit tagged", shouldSpecialise = false)

testChunkToArraySeqUntagged[Int]("Int untagged")
testChunkToArraySeqUntagged[Double]("Double untagged")
testChunkToArraySeqUntagged[Long]("Long untagged")
testChunkToArraySeqUntagged[Float]("Float untagged")
testChunkToArraySeqUntagged[Char]("Char untagged")
testChunkToArraySeqUntagged[Byte]("Byte untagged")
testChunkToArraySeqUntagged[Short]("Short untagged")
testChunkToArraySeqUntagged[Boolean]("Boolean untagged")
testChunkToArraySeqUntagged[Unit]("Unit untagged")
testChunkToArraySeqUntagged[String]("String untagged")

private def testChunkFromArraySeq[A: ClassTag: Generator]: Unit = {
val testTypeName: String = implicitly[ClassTag[A]].runtimeClass.getSimpleName

s"fromArraySeq ArraySeq[$testTypeName]" - {
"mutable" in forAll { arraySeq: mutable.ArraySeq[A] =>
assert(Chunk.arraySeq(arraySeq).toVector == arraySeq.toVector)
}

"immutable" in forAll { arraySeq: immutable.ArraySeq[A] =>
assert(Chunk.arraySeq(arraySeq).toVector == arraySeq.toVector)
}
}
}

"Chunk from ArraySeq" - {
testChunkFromArraySeq[Int]
testChunkFromArraySeq[Double]
testChunkFromArraySeq[Long]
testChunkFromArraySeq[Float]
testChunkFromArraySeq[Char]
testChunkFromArraySeq[Byte]
testChunkFromArraySeq[Short]
testChunkFromArraySeq[Boolean]
testChunkFromArraySeq[Unit]
testChunkFromArraySeq[String]
}

"Chunk iterable" - {
"mutable ArraySeq" in forAll { a: mutable.ArraySeq[String] =>
assert(Chunk.iterable(a).toVector == a.toVector)
}

"immutable ArraySeq" in forAll { a: immutable.ArraySeq[String] =>
assert(Chunk.iterable(a).toVector == a.toVector)
}
}

}