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

Create chunks in Stream.fromIterator (#2010) #2013

Merged
merged 2 commits into from
Aug 28, 2020

Conversation

susuro
Copy link
Contributor

@susuro susuro commented Aug 28, 2020

Improves performance of Stream.fromIterator and Stream.fromBlockingIterator by creating chunks of requested size, as described in #2010.

@mpilquist
Copy link
Member

Looks like mima configuration is messed up: [info] fs2-core: mimaPreviousArtifacts is empty, not analyzing binary compatibility.

If we fix that so it correctly checks binary compatibility, the build will fail on this change. Can you try making it an overload instead?

@mpilquist mpilquist merged commit c10bd88 into typelevel:main Aug 28, 2020
@mpilquist
Copy link
Member

Mima check is failing with:

[error] fs2-core: Failed binary compatibility check against co.fs2:fs2-core_2.12:2.0.0! Found 2 potential problems (filtered 662)
[error]  * extension method apply$extension(Boolean,scala.concurrent.ExecutionContext,scala.collection.Iterator,cats.effect.Sync,cats.effect.ContextShift)fs2.internal.FreeC in object fs2.Stream#PartiallyAppliedFromBlockingIterator does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("fs2.Stream#PartiallyAppliedFromBlockingIterator.apply$extension")
[error]  * extension method apply$extension(Boolean,scala.collection.Iterator,cats.effect.Sync)fs2.internal.FreeC in object fs2.Stream#PartiallyAppliedFromIterator does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("fs2.Stream#PartiallyAppliedFromIterator.apply$extension")
[error] stack trace is suppressed; run last mimaReportBinaryIssues for the full output

@susuro
Copy link
Contributor Author

susuro commented Aug 28, 2020

I have no clue why this has broken the binary compatibility and how to cope with it. Any idea?

@susuro
Copy link
Contributor Author

susuro commented Aug 28, 2020

I don't see how this can really break the compatibility. I would suggest excluding it from compatibility checks:

ProblemFilters.exclude[DirectMissingMethodProblem]("fs2.Stream#PartiallyAppliedFromIterator.apply$extension"),
ProblemFilters.exclude[DirectMissingMethodProblem]("fs2.Stream#PartiallyAppliedFromBlockingIterator.apply$extension")

@mpilquist
Copy link
Member

Can you try compiling some code against 2.4.4 that uses fromIterator and then running that compiled code against the current head?

@susuro
Copy link
Contributor Author

susuro commented Aug 29, 2020

I did two tests on Scala 2.13, calling fromIterator directly and through a library. Both, the application and the library were built against 2.4.4 and first run with this version on classpath, then with the version from head. Each execution was correct. The head version was built with publishLocal after slight modification of build.sbt to get the library for Scala 2.13 (only 0.26 was published originally).
To check if the methodology is correct, I built also a modified head version with default chunk size parameter instead of an overload. This time I got indeed an error Exception in thread "main" java.lang.NoSuchMethodError: 'fs2.internal.FreeC fs2.Stream$PartiallyAppliedFromIterator$.apply$extension(boolean, scala.collection.Iterator, cats.effect.Sync)'.
I am not sure if this proves the compatibility enough, though.

@mpilquist
Copy link
Member

OK cool, I guess it's a false report by mima. Let's add the exclusions.

@susuro
Copy link
Contributor Author

susuro commented Aug 29, 2020

I see you have already added it. Thanks.

@susuro susuro deleted the iterator-chunk branch August 29, 2020 18:07
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