Skip to content

Avoid leaking constraints in typer states #12333

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

Merged
merged 7 commits into from
May 6, 2021

Conversation

odersky
Copy link
Contributor

@odersky odersky commented May 5, 2021

The set of fixes here together makes sure that the constraint of the typerstate at the end of each phase is empty. This is now tested under -Ycheck.

odersky added 6 commits May 5, 2021 10:26
owningState weak reference can be reset to null. This was observed
when compiling scalatest.
When we replace a type lambda in a constraint by another,
the type variables associated with the old lambda changed their origin
to point to the new lambda. But that means that the same type
variables in predecessor typestates do not point to their associated
lambda anymore. To fix this, we have to do the same renaming
of type lambdas also in predecessor typestates.
I verified that with the previous commit all compilation tests except two neg
tests pass this check.
Besides the constraint, we also need to reset any instantiated type variables.
This is necessary for correctness and also to avoid uncollected garbage in constraints
when an entry is never dropped since the associated type variables are no longer
owned by the constraint.
It is not enough to reset the constraint only, since type variables might have
gotten instantiated during matchtype reduction.

With that change we now test in -Ycheck that constraints are empty after each phase.
@odersky odersky changed the title Maintain TyperState invariants after mergeConstraints Avoid leaking constraints in typer states May 6, 2021
@odersky odersky requested review from smarter and liufengyun May 6, 2021 13:15
@odersky
Copy link
Contributor Author

odersky commented May 6, 2021

test performance please

@dottybot
Copy link
Member

dottybot commented May 6, 2021

performance test scheduled: 1 job(s) in queue, 1 running.

@odersky
Copy link
Contributor Author

odersky commented May 6, 2021

I also ran the scalatest, stdlib213, and shapeless builds from CB. The maximal constraint was in each case smaller than 30 entries.

Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@dottybot
Copy link
Member

dottybot commented May 6, 2021

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/12333/ to see the changes.

Benchmarks is based on merging with master (69c800b)

@liufengyun liufengyun merged commit f4d139d into scala:master May 6, 2021
@liufengyun liufengyun deleted the fix-merge-gc branch May 6, 2021 17:02
@Kordyjan Kordyjan added this to the 3.0.1 milestone 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.

4 participants