8196106: Support nested infinite or recursive flat mapped streams #18625
Conversation
|
👋 Welcome back vklang! A progress list of the required criteria for merging this PR into |
|
@viktorklang-ora This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 174 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
@viktorklang-ora The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
|
@PaulSandoz @AlanBateman I've added a commit to this PR which removes the use of Gatherer for Stream::flatMap, but instead implements flatMap for all of the pipelines using the same encoding which Gatherer would use. It seems very competitive performance-wise, and resolves at least one open JBS-issue with flatMap (will look to see if it resolves more than that) |
That's a rather clever use of Did you observe performance improvements using |
Thank you :)
I'm currently exploring some different venues for optimization. Stay tuned :) |
Webrevs
|
|
@AlanBateman @PaulSandoz Moving this to a candidate PR, see the updated description for performance numbers, the sequential improvements are very encouraging, as well as the improvements which addresses https://bugs.openjdk.org/browse/JDK-8196106 |
| DoubleConsumer downstreamAsDouble = downstream::accept; | ||
| class FlatMap implements Sink.OfDouble, DoublePredicate { | ||
| boolean cancel; | ||
| private final boolean shorts = isShortCircuitingPipeline(); |
There was a problem hiding this comment.
If there's a known, short-circuiting, operation in the pipeline, we have to use allMatch but in the case where we know that there's nothing which could short-circuit we can go down a fast-path to forEach.
src/java.base/share/classes/java/util/stream/DoublePipeline.java
Outdated
Show resolved
Hide resolved
viktorklang-ora
force-pushed
the
8196106
branch
from
bedbab8
to
a3f1e51
Compare
|
@viktorklang-ora Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
|
@PaulSandoz @AlanBateman This should be ready for review now, if you find some time :) |
src/java.base/share/classes/java/util/stream/AbstractPipeline.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/java/util/stream/AbstractPipeline.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/java/util/stream/DoublePipeline.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/java/util/stream/ReferencePipeline.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/java/util/stream/ReferencePipeline.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/java/util/stream/ReferencePipeline.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/java/util/stream/ReferencePipeline.java
Outdated
Show resolved
Hide resolved
|
Hello, i will not pretend I fully understand the code, but i think it's important to keep a constistent code style. |
|
Just pushed another commit addressing feedback from @forax. (FYI @PaulSandoz & @AlanBateman) |
|
/integrate |
|
Going to push as commit 8a5b86c.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@viktorklang-ora Pushed as commit 8a5b86c. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
|
I just noticed this improved performance in Java 23. However, it still doesn't fit my needs because this doesn't work: Stream.of(1)
.flatMap(c -> Stream.generate(() -> 1).flatMap(x -> Stream.generate(() -> x)))
.iterator()
.next()Should it work? And how else can one use some stream elements in a non-terminal way? |
This PR implements Gatherer-inspired encoding of
flatMapthat shows that it is both competitive performance-wise as well as improve correctness.Below is the performance of
Stream::flatMap(for reference types):Before this PR (on my MacBook, aarch64):
After this PR (on my MacBook, aarch64):
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18625/head:pull/18625$ git checkout pull/18625Update a local copy of the PR:
$ git checkout pull/18625$ git pull https://git.openjdk.org/jdk.git pull/18625/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 18625View PR using the GUI difftool:
$ git pr show -t 18625Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18625.diff
Webrev
Link to Webrev Comment