Skip to content

FlatMapPipelinedCursor starts background calculations of pipelined inner cursors #3072

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.

Sign up for GitHub

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

FlatMapPipelinedCursor starts background calculations of pipelined inner cursors #3072

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0be931c
Start calculating inner's first item concurrently.
hatyo Jan 29, 2025
b4322bb
address comment.
hatyo Jan 29, 2025
aa38bd0
comment out now incorrect assertions.
hatyo Jan 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions ...ayer-core/src/main/java/com/apple/foundationdb/record/cursors/FlatMapPipelinedCursor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,7 @@ private synchronized void addEntryToPipeline(PipelineQueueEntry pipelineQueueEnt
if (closed) {
pipelineQueueEntry.close();
}
pipelineQueueEntry.getNextInnerPipelineFuture();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need both this change and the change in the PipelineQueueEntry constructor? If I'm reasoning about this correctly, it seems like we only need one of the two (that this is extraneous if the next future is fired off during the constructor, and the constructor call is extraneous if the future is created here)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, this was an oversight, let me remove that.

pipeline.add(pipelineQueueEntry);
}

Expand Down Expand Up @@ -295,20 +296,26 @@ public PipelineQueueEntry(RecordCursor<V> innerCursor,
this.priorOuterContinuation = priorOuterContinuation;
this.outerResult = outerResult;
this.outerCheckValue = outerCheckValue;
// start calculating the next result in the background.
setInnerFuture();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a matter of style, I think it would be better not to fire off this future in the constructor. There are some security implications in general if this can throw an error (see: https://www.baeldung.com/java-constructors-exceptions, see "Security concerns"), but also, it's counter-intuitive for users of the API, who don't expect constructing an object to have side-effects like this. If we really want to always fire off this future when we make a new element, I think we should make a static initializer, which removes the security concern and is less likely to surprise API consumers (which, granted, is just us because this is a private class)

}

@Nonnull
public CompletableFuture<PipelineQueueEntry> getNextInnerPipelineFuture() {
if (innerFuture == null) {
if (innerCursor == null) {
innerFuture = CompletableFuture.completedFuture(RecordCursorResult.exhausted());
} else {
innerFuture = innerCursor.onNext();
}
setInnerFuture();
}
return innerFuture.thenApply(vignore -> this);
}

private void setInnerFuture() {
if (innerCursor == null) {
innerFuture = CompletableFuture.completedFuture(RecordCursorResult.exhausted());
} else {
innerFuture = innerCursor.onNext();
}
}

public boolean doesNotHaveReturnableResult() {
if (innerCursor == null || // Hit sentinel, so we have a returnable result
innerFuture == null || // Inner future hasn't been started yet.
Expand Down