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

Update evalTapChunk #3350

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

kamilkloch
Copy link
Contributor

@kamilkloch kamilkloch commented Nov 28, 2023

@kamilkloch kamilkloch marked this pull request as ready for review November 29, 2023 10:17
@peterneyens
Copy link
Contributor

This looks similar to @diesalbla's #3197.

@kamilkloch
Copy link
Contributor Author

This looks similar to @diesalbla's #3197.

We have a benchmark here :)

diesalbla
diesalbla previously approved these changes Dec 11, 2023
@diesalbla
Copy link
Contributor

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

@diesalbla
Copy link
Contributor

@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?

@kamilkloch
Copy link
Contributor Author

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

Does it compile? I get stuck at .scoped.

@armanbilge
Copy link
Member

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.

@kamilkloch
Copy link
Contributor Author

kamilkloch commented Dec 13, 2023

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?

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.

Optimize Stream#evalTapChunk
4 participants