Fix #6635: Improve subtype tests for aliases and singleton types #8443
Conversation
1. backtrack in more cases when we we fail after dealiasing 2. fall back to atoms comparisons when comparing two singleton types The widened criterion for (1) is still provisional. We need to come up with a criterion that is sound and has a low risk of leading to combinatorial explosion when comparing two long alias chains.
A better criterion for avoiding backtracking after comparing a dealiased type.
|
test performance please |
|
performance test scheduled: 2 job(s) in queue, 1 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/8443/ to see the changes. Benchmarks is based on merging with master (91e5f35) |
Cache canDopAlias calls in TypeRefs. Performance tests pointed to a ~1% showdown before, which is really in the noise. However, stats showed that the number of iterations in the exists call of `canDropAlias` is significant (a bit more than total member operations). With this optimization, the number of calls gets reduced to 1% of what it was.
|
test performance please |
|
performance test scheduled: 5 job(s) in queue, 1 running. |
|
Cool that Dotty can fix this! Needing backtracking is unfortunate but easy, and the alternative I had imagined (union-find) requires much more work, so that's cool... |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/8443/ to see the changes. Benchmarks is based on merging with master (c5a3b5c) |
|
@Blaisorblade Yes, I fear that union-find would be difficult to make fast in these situations. |
| } | ||
| isNewSubType(tp1.underlying.widenExpr) || comparePaths | ||
| case _ => false | ||
| comparePaths || isNewSubType(tp1.underlying.widenExpr) |
There was a problem hiding this comment.
Discussed this change in-person w/ Martin: changing the order of arguments here is solely a performance optimization, not an actual change to logic.
There was a problem hiding this comment.
@hepin1989 thank you for your astute observation! I was thinking about things such as the old RHS assuming the old LHS already handled some type shape, or that we do some evil tricks when loading code from Tasty and the check on new LHS is necessary so that the new RHS doesn't blow up, but I suppose it never hurts to be reminded of elementary laws of Boolean algebra.
There was a problem hiding this comment.
Or otherwise, a || b and b || a differ when a and b are not pure :-)
|
@AleksanderBG what's the status on this? This one is the only blocker for the release. |
|
@anatoliykmetyuk I thought that Martin also requested Olivier's review. If not, you can just merge this. |
| && tp1.signature == tp2.signature | ||
| && !(sym1.isClass && sym2.isClass) // class types don't subtype each other | ||
| ) || | ||
| thirdTryNamed(tp2) |
There was a problem hiding this comment.
introduce an explicit boolean return?
|
| implicit val ctx: Context = this.ctx | ||
| tp2.info match { | ||
| val info2 = tp2.info |
There was a problem hiding this comment.
This binding doesn't seem necessary
Uh oh!
There was an error while loading. Please reload this page.