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

Clear temp var for captured var expr to permit GC #14205

Merged
merged 2 commits into from
Jan 10, 2022

Conversation

som-snytt
Copy link
Contributor

Fixes #14198

@som-snytt som-snytt marked this pull request as draft January 3, 2022 16:43
@som-snytt
Copy link
Contributor Author

som-snytt commented Jan 3, 2022

That was just non-trivial enough to learn something. Running the suite locally shows errors, so converting to draft while I chase those.

Still to answer, what does a tree inherit by virtue of cpy (position?), and why transformFollowing is called twice.

Update is that resetting the temporary requires null.asInstanceOf[T]. Checker complained where T is Thing[], but I haven't clarified all the post-erasure aspects.

That was in MegaPhase where var nx: Array[MiniPhase] is set by range.foreach { nx = ??? }. I think in Scala 2 the closure is eliminated, because there was a comment

// OPT: hand rolled version of sum

but the var sum is captured by range.foreach, as spuriously mentioned by -Wperformance. Obviously this level of "code switching" (so to speak) could prove quite stressful.

Hardest part still is trying to figure out how to run tests under dotty.

@anatoliykmetyuk
Copy link
Contributor

@som-snytt what's the status on this one? Is it review-ready?

@som-snytt som-snytt marked this pull request as ready for review January 10, 2022 15:25
@som-snytt
Copy link
Contributor Author

Possibly the extra nullifier could be emitted only selectively.

@smarter smarter assigned som-snytt and unassigned smarter Jan 10, 2022
@som-snytt
Copy link
Contributor Author

Documented the change, aligned the stars, and omit nullify for x = null.

Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

Thank you!

@smarter smarter merged commit db34814 into scala:master Jan 10, 2022
@som-snytt som-snytt deleted the issue/14198 branch January 10, 2022 21:27
@Kordyjan Kordyjan modified the milestones: 3.2.0-RC1, 3.1.2 Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Local is not cleared, causing an object not to be GC'd
4 participants