fs2 icon indicating copy to clipboard operation
fs2 copied to clipboard

Published 20 hours ago • verified publisher icon typelevel

Update evalTapChunk

Open kamilkloch opened this issue 1 year ago • 7 comments

Fixes #3351.

Benchmark: https://gist.github.com/kamilkloch/7edfe8cab15ebf0a75332c40b316ad18

kamilkloch avatar Nov 28 '23 23:11 kamilkloch

This looks similar to @diesalbla's https://github.com/typelevel/fs2/pull/3197.

peterneyens avatar Dec 11 '23 10:12 peterneyens

This looks similar to @diesalbla's #3197.

We have a benchmark here :)

kamilkloch avatar Dec 11 '23 11:12 kamilkloch

It's similar, but @kamilkloch commit is more focused.

diesalbla avatar Dec 11 '23 18:12 diesalbla

@kamilkloch I remembered that the evalMap could not be directly written to an unconsFlatMap. There are some differences between the flatMap and the unconsFlatMap, related to how the latter treats interruption.

In the PR I had, the code I was using was this:

    underlying.unconsFlatMap(Pull.output1).scoped.flatMapOutput(onChunk).streamNoScope

This is more complicated, but needed to preserve that semantics. Could you try using that line on your PR and see if the benchmarks still show it worth it, please?

diesalbla avatar Dec 11 '23 21:12 diesalbla 

    underlying.unconsFlatMap(Pull.output1).scoped.flatMapOutput(onChunk).streamNoScope

Does it compile? I get stuck at .scoped.

kamilkloch avatar Dec 12 '23 09:12 kamilkloch

There are some differences between the flatMap and the unconsFlatMap, related to how the latter treats interruption.

Can we add a test for evalTapChunk covering this case? It's clearly subtle and easy to make a regression.

armanbilge avatar Dec 12 '23 17:12 armanbilge

Can we add a test for evalTapChunk covering this case? It's clearly subtle and easy to make a regression.

By all means, but I am afraid I would need some rope here (scopes are still an urchartered territory for me). @diesalbla, would you perhaps have and idea for a test?

kamilkloch avatar Dec 13 '23 10:12 kamilkloch