Fix GADT-related memory leak #3233
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 0 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/3233/ to see the changes. Benchmarks is based on merging with master (96b0ae4) |
smarter
reviewed
Oct 3, 2017
| @@ -532,7 +532,7 @@ object Contexts { | |||
| moreProperties = Map.empty | |||
| typeComparer = new TypeComparer(this) | |||
| searchHistory = new SearchHistory(0, Map()) | |||
| gadt = new GADTMap(SimpleMap.Empty) | |||
| gadt = new GADTMap(SimpleMap.Empty) // EmptyGADTMap | |||
There was a problem hiding this comment.
Was this supposed to be = EmptyGADTMap ?
smarter
reviewed
Oct 3, 2017
| @@ -289,6 +281,8 @@ class PostTyper extends MacroTransform with SymTransformer { thisTransformer => | |||
| case _ => | |||
| } | |||
| super.transform(tree) | |||
| case Typed(Ident(nme.WILDCARD), _) => | |||
| tree // skip checking pattern type | |||
There was a problem hiding this comment.
Because type bounds in a pattern need not conform to selector bounds. I.e. you have
type Tree[T >: Null <: Type]
You are still allowed to write
case x: Tree[_]
(which translates to)
case x: (_: Tree[_])
There was a problem hiding this comment.
I am going to refine & explain the logic better
smarter
reviewed
Oct 4, 2017
| @@ -256,7 +256,7 @@ class PostTyper extends MacroTransform with IdentityDenotTransformer { thisTrans | |||
| case tree @ Annotated(annotated, annot) => | |||
| cpy.Annotated(tree)(transform(annotated), transformAnnot(annot)) | |||
| case tree: AppliedTypeTree => | |||
| Checking.checkAppliedType(tree) | |||
| Checking.checkAppliedType(tree, boundsCheck = !ctx.mode.is(Mode.Pattern)) | |||
There was a problem hiding this comment.
Now I'm confusing about why we would write this instead of if (!ctx.mode.is(Mode.Pattern)) Checking.checkAppliedType(tree) :)
There was a problem hiding this comment.
Because checkAppliedType also does other things besides bounds checking.
There was a confusion which led to the wrong gadt map being used for pattern-bound variables if there was no other GADT variable in the enclosing method. This led to the outermost gadt map in the initial context being populated with type bounds which never went away.
If -Xprompt is set, pause a dotty.tools.dotc.Bench task between compiler runs. This is useful for, e.g. taking a memory snapshot at a predictable time.
smarter
approved these changes
Oct 4, 2017
smarter
added a commit
to dotty-staging/dotty
that referenced
this pull request
Feb 12, 2018
We no longer convert GADT bounds into normal bounds since scala#3233 was merged.
smarter
added a commit
to dotty-staging/dotty
that referenced
this pull request
Feb 12, 2018
We no longer convert GADT bounds into normal bounds since scala#3233 was merged.
smarter
added a commit
to dotty-staging/dotty
that referenced
this pull request
Feb 13, 2018
We no longer convert GADT bounds into normal bounds since scala#3233 was merged.
smarter
added a commit
to dotty-staging/dotty
that referenced
this pull request
Feb 14, 2018
We no longer convert GADT bounds into normal bounds since scala#3233 was merged.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a confusion which led to the wrong gadt map being used
for pattern-bound variables if there was no other GADT variable
in the enclosing method.
This led to the outermost gadt map in the initial context being
populated with type bounds which never went away.