Deskolemize PatternTypeConstrainer #12506
Conversation
|
test performance with #exhaustivity please |
|
@liufengyun am I doing something wrong? Why won't the performance check run? |
Before this commit, constrainSimplePatternType did a bunch of manipulations and then ran a subtype check. It wrapped one of the types in a SkolemType and replaces type arguments of the other one with wildcards. The point of those manipulations was to make TypeComparer take a specific path and compare appropriate subcomponents of constrainSimplePatternType's arguments. This turned out to be a problem overall. To improve the situation, we instead manually compare the appropriate subcomponents of constrainSimplePatternType - the changes can be seen as inlining pieces of code from TypeComparer. Also, this commit adds restoring the GADT constraint to TypeComparer - this turned out to be more important now that constrainSimplePatternType now does multiple isSubType checks.
abgruszecki
force-pushed
the
deskolemize-gadts
branch
from
2875d19
to
fabd1ae
Compare
|
I added additional explanations to the first commit. The tests passed before and should also pass now. |
|
@abgruszecki You are not on the whitelist. I'm now adding you. Edited: it should work now. |
|
performance test scheduled: 1 job(s) in queue, 1 running. |
|
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/12506/ to see the changes. Benchmarks is based on merging with master (1f8e9b2) |
|
test performance please |
|
performance test scheduled: 4 job(s) in queue, 1 running. |
|
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/12506/ to see the changes. Benchmarks is based on merging with master (e48a268) |
| def apply(tp: Type) = tp.dealias match | ||
| case tp: TypeRef if !tp.symbol.isClass => | ||
| expandBounds(tp.info.bounds) | ||
| if alreadyExpanding contains tp then tp else |
There was a problem hiding this comment.
How is this connected to the rest of the changes? Did the rest trigger a SO here?
There was a problem hiding this comment.
Yes, this is the source of the SO that happened after restoring GADT constraints on failed subtype checks.
|
There was a worrisome jump in performance benchmarks, I still want to take a look at that. |
|
test performance please |
|
performance test scheduled: 2 job(s) in queue, 1 running. |
|
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/12506/ to see the changes. Benchmarks is based on merging with master (54929c2) |
|
@smarter I modified the comments, can you mark it as accepted? It seems that the second benchmark didn't show a performance decrease compared to current master, so I think we're good to go. |
Before this PR, constrainSimplePatternType did a bunch of manipulations on its arguments and then ran a subtype check. It wrapped one of the types in a SkolemType and replaced type arguments (if any) of the other one with wildcards. The point of those manipulations was to make TypeComparer take a specific path and compare appropriate subcomponents of
constrainSimplePatternType'sarguments. This turned out to be a problem overall. To improve the situation, we instead manually compare the appropriate subcomponents ofconstrainSimplePatternType's arguments - the changes can be seen as inlining pieces of code from TypeComparer.Also, this PR adds restoring the GADT constraint to TypeComparer - this turned out to be more important now that constrainSimplePatternType now does multiple isSubType checks.
Fixes #11103, #12476.