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

Revert PR #9818 (GroupedIterator improvements) #9859

Merged
merged 1 commit into from
Dec 17, 2021

Conversation

SethTisue
Copy link
Member

@SethTisue SethTisue commented Dec 16, 2021

This reverts commit d047407.

Because it caused community build failures; see scala/community-build#1519 for details

This reverts commit d047407.

Reverts PR scala#9818; see scala/community-build#1519 for details
on the community build failures
@SethTisue SethTisue added this to the 2.13.8 milestone Dec 16, 2021
@scala-jenkins scala-jenkins modified the milestones: 2.13.8, 2.13.9 Dec 16, 2021
@SethTisue SethTisue added the library:collections PRs involving changes to the standard collection library label Dec 16, 2021
@SethTisue SethTisue modified the milestones: 2.13.9, 2.13.8 Dec 16, 2021
@SethTisue SethTisue added the prio:blocker release blocker (used only by core team, only near release time) label Dec 16, 2021
@SethTisue SethTisue changed the title Revert PR #9818 ("GroupedIterator improvements") Revert PR #9818 (GroupedIterator improvements) Dec 16, 2021
@SethTisue SethTisue added internal not resulting in user-visible changes (build changes, tests, internal cleanups) and removed library:collections PRs involving changes to the standard collection library labels Dec 16, 2021
@som-snytt
Copy link
Contributor

I commented on the PR that the community build just uncovers a bad test or assumption about the API. Make that two bad tests. I only looked at twitter-util which was straightforward.

@SethTisue
Copy link
Member Author

SethTisue commented Dec 17, 2021

Regardless, I'm still much more comfortable reverting the PR for 2.13.8, in keeping with the informal alternation we tend to have between big releases with months of changes (e.g. 2.13.7) and modest releases focused on addressing regressions (e.g. 2.13.8).

@lrytz
Copy link
Member

lrytz commented Dec 17, 2021

... I agree.

Copy link
Member

@joroKr21 joroKr21 left a comment

Choose a reason for hiding this comment

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

I agree with the revert. It's reasonable to expect that smallCollection.grouped(hugeNumber) won't actually allocate much memory. It's an edge case but a valid edge case IMO (same as collection.take(negativeNumber)). I'm now thinking about another approach where we reuse the initial buffer for everything. That should both reduce allocations overall and allow us to let it grow since it would be a one-time cost. I don't see a theoretical reason why we can't do that.

@lrytz lrytz merged commit 4b2151c into scala:2.13.x Dec 17, 2021
@SethTisue SethTisue removed the prio:blocker release blocker (used only by core team, only near release time) label Dec 17, 2021
@SethTisue SethTisue deleted the revert-grouped-pr-9818 branch December 17, 2021 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal not resulting in user-visible changes (build changes, tests, internal cleanups)
Projects
None yet
5 participants