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

Conversation

tmccarthy
Copy link
Contributor

@tmccarthy tmccarthy commented Jun 19, 2020

This PR adds support for immutable and mutable ArraySeqs for Chunk. Conversions in both directions are provided. This PR follows on from #1899.

Constructing a Chunk from ArraySeq

The Chunk.arraySeq factory method constructs a Chunk by wrapping the underlying array. This reproduces the behaviour previously implemented in ArrayBackedSeq.

import scala.collection

Chunk.arraySeq(mutable.ArraySeq(1, 2, 3))   // Create Chunk from mutable ArraySeq
Chunk.arraySeq(immutable.ArraySeq(1, 2, 3)) // Create Chunk from immutable ArraySeq
Chunk.iterable(immutable.ArraySeq(1, 2, 3)) // Delegates to the factory methods above

Constructing an ArraySeq from a Chunk

import scala.collection.immutable.ArraySeq

val chunk = Chunk(1, 2, 3)

chunk.toArraySeq         // Uses implicit ClassTag to specialise ArraySeq
chunk.toArraySeqUntagged // Creates untagged ArraySeq

Why provide specialised support for ArraySeq?

The immutable ArraySeq is newly added in Scala 2.13. It is an immutable wrapper over a bare array with specialised implementations for primitives. As such it is well suited to the kinds of performance-sensitive uses for which fs2 is commonly used.

Comment on lines 596 to 597
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).

Comment on lines +11 to +12
copyToArray(array)
ArraySeq.unsafeWrapArray[O2](array)
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.


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.

@tmccarthy
Copy link
Contributor Author

Removing the ArrayBackedSeq matcher has broken the build for 2.12. I'll need to re-think the cross-version approach for the .iterable method 😕.

import scala.collection.immutable
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!

@tmccarthy tmccarthy marked this pull request as ready for review June 21, 2020 00:21
@tmccarthy
Copy link
Contributor Author

Thanks @mpilquist! This one should be good to go once the build completes.

@mpilquist mpilquist merged commit 6f215b7 into typelevel:main Jun 22, 2020
@mpilquist mpilquist added this to the 2.4.3 milestone Aug 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants