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

ensures given iterable is interacted once per subscriber #3297

Merged
merged 7 commits into from Nov 28, 2022

Conversation

OlegDokuka
Copy link
Contributor

close #3295

Signed-off-by: Oleh Dokuka odokuka@vmware.com

Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
@OlegDokuka OlegDokuka requested a review from a team as a code owner November 24, 2022 16:08
@OlegDokuka OlegDokuka added this to the 3.5.1 milestone Nov 24, 2022
@OlegDokuka OlegDokuka added the type/enhancement A general enhancement label Nov 24, 2022
@OlegDokuka OlegDokuka self-assigned this Nov 24, 2022
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>

try {
b = it.hasNext();
size = knownToBeFinite ? it.estimateSize() : -1;
Copy link
Member

Choose a reason for hiding this comment

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

I'd love to see a comment explaining why this is feasible, e.g. "If the Spliterator is finite and we know it has not been traversed, the estimate is accurate." - but this raises a question - should we check whether the Spliterator has not been traversed yet? Is there a way to do so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is irrelevant. At this stage, we just retrieved a new spliterator instance. So does it make sense to care about "check whether the Spliterator has not been traversed yet"?

Also, the code it selves is taken from the method Spliterator#getExactSizeIfKnown() so this avoids extra work since we knowToBeFinite

Copy link
Member

Choose a reason for hiding this comment

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

Right, I failed to record the fact the spliterator was retrieved in the constructor. Makes me think though: what happens in case of -1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is not used explicitly. I tend to think that indeed we need to replace that with simplified code. e.g.

hasValues = !knownToBeFinite || it.estimateSize() != 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

using the above we can say: ok if it is unknown to be finite - we can do nothing, assume has values, if it is known to be finite and estimated size is not 0 then it has values

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chemicL good to go?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, thanks.

Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
instanceof check brings significant overhead. All JMH benchmarks with observations are available at the following link https://gist.github.com/OlegDokuka/2a5dc81ba89d1f3b8f81153a78a4dd96

Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
@OlegDokuka
Copy link
Contributor Author

removed instanceof Collection check in checkFinite method

instanceof check brings significant overhead. All JMH benchmarks with observations are available at the following link https://gist.github.com/OlegDokuka/2a5dc81ba89d1f3b8f81153a78a4dd96

@reactorbot
Copy link

@OlegDokuka this PR seems to have been merged on a maintenance branch, please ensure the change is merge-forwarded to intermediate maintenance branches and up to main 🙇

@OlegDokuka OlegDokuka deleted the bugfix/3.4.x-#3295 branch November 28, 2022 10:38
OlegDokuka added a commit that referenced this pull request Nov 28, 2022
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
Signed-off-by: OlegDokuka <odokuka@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants