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.

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

Jump to bottom

[backport] Avoid retaining refs to previously added items after VectorBuilder.clear #10019

Merged
merged 3 commits into from May 2, 2022
Merged

[backport] Avoid retaining refs to previously added items after VectorBuilder.clear #10019

merged 3 commits into from May 2, 2022

Conversation

He-Pin
Copy link
Contributor

@He-Pin He-Pin commented Apr 27, 2022

@scala-jenkins scala-jenkins added this to the 2.12.17 milestone Apr 27, 2022
@He-Pin He-Pin mentioned this pull request Apr 27, 2022
Closed
@He-Pin
Copy link
Contributor Author

He-Pin commented Apr 27, 2022 •
edited

@SethTisue I updated it, @jrudolph cc

@SethTisue is it possible to reschedule this to 2.12.16?

@lrytz
Copy link
Member

lrytz commented Apr 27, 2022

This is basically a backport of #6779. I assume it could be a 1-1 backport (i.e., use preClean), but I this patch LGTM too.

Note that 2.13.0 Vector is different form current 2.13 Vector since 2.13.2, there was a complete rewrite (#8534). The 2.13 patch (#6779) was for the 2.13.0 version of Vector.

lrytz
lrytz approved these changes Apr 27, 2022
View reviewed changes
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.

👍

@SethTisue SethTisue modified the milestones: 2.12.17, 2.12.16 Apr 27, 2022
@SethTisue SethTisue requested a review from a team April 27, 2022 13:25
@SethTisue SethTisue added the library:collections PRs involving changes to the standard collection library label Apr 27, 2022
@SethTisue SethTisue changed the title =col Clear VectorBuilder more. [backport] Avoid retaining refs to previously added items after VectorBuilder.clear Apr 27, 2022
NthPortal
NthPortal approved these changes May 1, 2022
View reviewed changes
Copy link
Contributor

@NthPortal NthPortal left a comment

Choose a reason for hiding this comment

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

might be nice to have a test for this. there are a few GC tests in junit you can potentially look at for examples, such as LazyListGCTest

@som-snytt
Copy link
Contributor

I'd like to point out that on the original ticket, retronym called it a "clear bug" and added "(ha!)" so future researchers would not miss it.

@som-snytt
Copy link
Contributor

I pushed a test, I hope I did it right, I mean the git part. I didn't track down *GCTest but there is a helper for testing reachability. I noticed some unused imports and spurious warnings to quash.

@lrytz lrytz merged commit 7dbc139 into scala:2.12.x May 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lrytz lrytz lrytz approved these changes

@NthPortal NthPortal NthPortal approved these changes

Assignees
No one assigned
Labels
library:collections PRs involving changes to the standard collection library
Projects
None yet
Milestone
2.12.16
6 participants
@He-Pin @lrytz @som-snytt @NthPortal @SethTisue @scala-jenkins