-
Notifications
You must be signed in to change notification settings - Fork 603
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
Create a Chunk from an IterableOnce #2722
Conversation
else { | ||
val head = itr.next() | ||
if (itr.hasNext) { | ||
val bldr = collection.mutable.Buffer.newBuilder[O] |
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.
According to this
https://github.com/typelevel/fs2/pull/2720/files#diff-e8fdf3ccd3d2479432355b3abf34a16cd548373afeddeaf231b8d8a1147b1d48R115
ArrayBuilder shows x2 performance compared to mutable.Buffer, shouldn't we change a type of builder used here?
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.
In those cases we have the (expected) size, we don't in this case.
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.
ArrayBuilder should still beat mutable buffer though, even without a size hint.
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.
Using ArrayBuilder
requires us to add a ClassTag
constraint, I don't think that is something that we want?
That would mean that we couldn't use it in iterable
and chain
.
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.
Merge the main branch in to this PR and then use makeArrayBuilder[Any]
. We'll have to cast the resulting chunk back to a Chunk[A]
just like we do in methods like filter
.
@@ -596,18 +596,24 @@ object Chunk | |||
case ix: GIndexedSeq[O] => indexedSeq(ix) | |||
case _ => | |||
if (i.isEmpty) empty |
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.
emptiness gets checked in iterableOnce
, can we remove this check from here and L626
?
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.
Now the implementation has changed and it's an optimization. Thus, the comment is resolved.
I already forgot |
Thanks! |
The implementation is basically the one from
iterable
andchain
.I replaced the implementations in
iterable
andchain
with a call toiterableOnce
. This does add anIterator#isEmpty
call.