-
Notifications
You must be signed in to change notification settings - Fork 604
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
Avoid double evaluation of pattern matchers in Chunk.collect #3364
Conversation
7df3227
to
1c3ed3d
Compare
@@ -87,7 +87,8 @@ abstract class Chunk[+O] extends Serializable with ChunkPlatform[O] with ChunkRu | |||
def collect[O2](pf: PartialFunction[O, O2]): Chunk[O2] = { | |||
val b = makeArrayBuilder[Any] | |||
b.sizeHint(size) | |||
foreach(o => if (pf.isDefinedAt(o)) b += pf(o)) | |||
val f = pf.runWith(b += _) | |||
foreach { o => f(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.
Well, this looks rather ugly. pf.runWith
returns O => Boolean
so we have to transform it to O => Unit
to avoid warnings. This could be rewritten as follows (at the cost of extra lambda call per Chunk item):
foreach(pf.runWith(b += _).andThen(_ => ()))
@armanbilge, which one would you prefer?
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.
Ugly but performant is the right choice 😅
Here is some benchmark results. 1. Masterforeach(o => if (pf.isDefinedAt(o)) b += pf(o))
2. This PR val f = pf.runWith(b += _)
foreach { o => f(o); () }
3. This PR (prettified)foreach(pf.runWith(b += _).andThen(_ => ()))
Option 2 (this PR) shows slightly better score than the other two options. |
1c3ed3d
to
4e146de
Compare
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.
Thank you for the very thorough PR, this is very nice work 🙏
@armanbilge Thank you for the prompt and kind feedback :) |
fs2.Chunk#collect
evaluates pattern matchers and guards of a passed partial function twice because it callspf.isDefinedAt
and thenpf.apply
:Scala collections use
pf.applyOrElse
/pf.runWith
(depending on scala version) instead to avoid double evaluation. This PR implements the same approach in aChunk
.It should be noted that this may change observable behavior for those who use a partial function with side-effecting matchers/guards. But hey, side-effecting matchers/guards is considered a bad practice anyway, and I doubt that there's an fs2 user relying on that double evaluation "feature."