Cache skolem types #14909
Merged
Cache skolem types #14909
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, 1 running. |
|
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/14909/ to see the changes. Benchmarks is based on merging with main (73df472) |
smarter
added a commit
to dotty-staging/dotty
that referenced
this pull request
Apr 20, 2022
Previously, when reducing `a.T` we checked if the type of `a` was a subtype of `RefinedType(.., T, TypeAlias(...))`, now we extend this check to handle refinements where the `info` is a `TypeBounds` where both bounds are equal. This solves two big issues at once: - We can restore tests/pos/13491.scala to its original form from before scala#13780. The check for abstract types introduced by scala#13780 for soundness reasons is no longer hit because the type selection is reduced before we get to that point. This is important because parboiled2 relies on this and is therefore currently broken on 3.1.3-RC1 and main (sirthias/parboiled2#365). - This fixes scala#14904 (slow compilation issue affecting parboiled2) without caching skolems (as in the alternative fix scala#14909). Again, this is due to the type containing skolem being reducible to a simpler type and therefore cacheable.
smarter
added a commit
to dotty-staging/dotty
that referenced
this pull request
Apr 20, 2022
Previously, when reducing `a.T` we checked if the type of `a` was a subtype of `RefinedType(.., T, TypeAlias(...))`, now we extend this check to handle refinements where the `info` is a `TypeBounds` where both bounds are equal. This solves two big issues at once: - We can restore tests/pos/13491.scala to its original form from before scala#13780. The check for abstract types introduced by scala#13780 for soundness reasons is no longer hit because the type selection is reduced before we get to that point. This is important because parboiled2 relies on this and is therefore currently broken on 3.1.3-RC1 and main (sirthias/parboiled2#365). - This fixes scala#14903 (slow compilation issue affecting parboiled2) without caching skolems (as in the alternative fix scala#14909). Again, this is due to the type containing skolem being reducible to a simpler type and therefore cacheable.
|
I think it's still worth to merge this, to prevent creating too many type copies in other situations |
smarter
reviewed
Apr 21, 2022
| override def underlying(using Context): Type = info | ||
| def derivedSkolemType(info: Type)(using Context): SkolemType = | ||
| if (info eq this.info) this else SkolemType(info) | ||
| override def hashCode: Int = System.identityHashCode(this) | ||
|
|
||
| override def computeHash(bs: Binders): Int = identityHash(bs) |
There was a problem hiding this comment.
Shouldn't this return NotCached if info itself is not cache-able?
There was a problem hiding this comment.
No, a skolem is in any case unique. We never merge two skolems. So it does not matter whether we
cache the info or not.
tests/pos/i14903.scala
Outdated
| @@ -0,0 +1,22 @@ | |||
| trait Wrapper[T] { | |||
There was a problem hiding this comment.
This test is already present as https://github.com/lampepfl/dotty/blob/main/tests/pos/i14903a.scala, and without any test nor any hash table statistics it's hard for me to say if this change is a net positive.
There was a problem hiding this comment.
I think we can argue by analogy to type variables. Both skolem types and type variables are locally generated, and both are usually part of more complicated types. Type variables are cached, so it makes sense to cache skolem types as well.
There was a problem hiding this comment.
OK, I've edited the commit message to mention this and dropped the duplicate test.
michelou
pushed a commit
to michelou/scala3
that referenced
this pull request
Apr 25, 2022
Previously, when reducing `a.T` we checked if the type of `a` was a subtype of `RefinedType(.., T, TypeAlias(...))`, now we extend this check to handle refinements where the `info` is a `TypeBounds` where both bounds are equal. This solves two big issues at once: - We can restore tests/pos/13491.scala to its original form from before scala#13780. The check for abstract types introduced by scala#13780 for soundness reasons is no longer hit because the type selection is reduced before we get to that point. This is important because parboiled2 relies on this and is therefore currently broken on 3.1.3-RC1 and main (sirthias/parboiled2#365). - This fixes scala#14903 (slow compilation issue affecting parboiled2) without caching skolems (as in the alternative fix scala#14909). Again, this is due to the type containing skolem being reducible to a simpler type and therefore cacheable.
Skolem types were not cached, which means that any type containing a skolem type was not cached either. This meant that the same match type with a skolem type as selector was created many times instead of once, so its reduction was not cached either. We now cache skolem types. It's a bet that in practice few skolem types are created and that therefore the hashtable pollution with skolemtypes is less of a problem than the potential problem of losing identity of types containing skolem types. This was originally motivated by scala#14903 but that ended up being fixed in a different way. It still seems like a good idea to do this since it matches what we do for type variables.
smarter
approved these changes
Apr 26, 2022
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.
Skolem types were not cached, which means that any type containing a skolem type
was not cached either. This meant that the same match type with a skolem type as
selector was created many times instead of once, so its reduction was not cached
either. We now cache skolem types. It's a bet that in practice few skolem types are
created and that therefore the hashtable pollution with skolemtypes is less of
a problem than the potential problem of losing identity of types containing
skolem types.
Fixes #14903