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

[11630] Iterator#flatMap#hasNext calls outer#hasNext 1 time, not 2-3 times #8220

Merged
merged 1 commit into from Jul 15, 2019

Conversation

joshlemer
Copy link
Member

@joshlemer joshlemer commented Jul 13, 2019

Fixes scala/bug#11630

When doing two calls in succession to Iterator#flatMap:

it.hasNext()
it.next()

the status quo is to have the flatmapped iterator call the sub-iterator's hasNext 3 times:

  • once during the "advance to next non-empty sub-iterator, if required, and return it" phase
  • a second time, to return that now-current sub-iterator's hasNext to the outside caller
  • a third time, when calling next()

This PR replaces that logic with a system where these 3 calls are replaced with a single call during the it.hasNext phase, which is achieved by caching the state of the iterator in a new private field _hasNext: Int which can either be:

  • 0 = known to be false (hasNext is known to be false)
  • 1 = known to be true (hasNext is known to be true true)
  • -1 = unknown (not known if hasNext would be true or false)

The resulting state of the iterator after a hasNext call is either 0 or 1. After a call to next() we won't know if we have exhausted the entire iterator or not, since we are not forcing any more work to be done, so we set our state to -1.

This partially was a port of the old 2.12 implementation, but that implementation still required 2 calls to cur.hasNext, whereas this PR represents the additional improvement to only call cur.hasNext once during a hasNext(); next(); call sequence.

TODO

  • Write some unit tests

@scala-jenkins scala-jenkins added this to the 2.13.1 milestone Jul 13, 2019
@joshlemer joshlemer added library:collections PRs involving changes to the standard collection library performance the need for speed. usually compiler performance, sometimes runtime performance. regression labels Jul 13, 2019
@joshlemer joshlemer changed the title [11630] Iterator#flatMap#hasNext calls outer#hasNext 1 times, not 2-3 times [WIP][nomerge][11630] Iterator#flatMap#hasNext calls outer#hasNext 1 times, not 2-3 times Jul 13, 2019
@hrhino
Copy link
Member

hrhino commented Jul 13, 2019

I would have expected the "regression" label to be a badge of shame attached to released PRs.

@joshlemer joshlemer changed the title [WIP][nomerge][11630] Iterator#flatMap#hasNext calls outer#hasNext 1 times, not 2-3 times [11630] Iterator#flatMap#hasNext calls outer#hasNext 1 times, not 2-3 times Jul 14, 2019
@joshlemer joshlemer changed the title [11630] Iterator#flatMap#hasNext calls outer#hasNext 1 times, not 2-3 times [11630] Iterator#flatMap#hasNext calls outer#hasNext 1 time, not 2-3 times Jul 14, 2019
@SethTisue SethTisue added the prio:blocker release blocker (used only by core team, only near release time) label Jul 14, 2019
Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

Thanks, @joshlemer!

@lrytz lrytz merged commit 0b0f0f0 into scala:2.13.x Jul 15, 2019
@joshlemer joshlemer deleted the issue/11630 branch July 15, 2019 13:29
@szeiger szeiger added the release-notes worth highlighting in next release notes label Sep 9, 2019
sim642 added a commit to sim642/adventofcode that referenced this pull request Sep 18, 2019
This reverts commit eb6a4bf.
The regression was fixed in Scala 2.13.1: scala/scala#8220.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
library:collections PRs involving changes to the standard collection library performance the need for speed. usually compiler performance, sometimes runtime performance. prio:blocker release blocker (used only by core team, only near release time) release-notes worth highlighting in next release notes
Projects
None yet
6 participants