Fix inductive implicits performance regression #6329
Conversation
|
test performance please |
|
test performance please |
|
@milessabin Looks like you're not in the whitelist: https://github.com/liufengyun/bench/blob/f8f08f6c31f561f9b831c02b3281609a46650417/bin/process#L22, you can make a PR to fix that. |
|
test performance please |
|
performance test scheduled: 3 job(s) in queue, 1 running. |
|
@smarter done here: liufengyun/bench#723 liufengyun/bench seems to be fresher than lampepfl/bench ... how come? |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/6329/ to see the changes. Benchmarks is based on merging with master (f09550d) |
|
That seems to have done the trick :-) |
|
Just add @milessabin and @anatoliykmetyuk . Hot maintenance happens in liufengyun/bench , so it is fresher. |
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 0 running. |
| case tp: TermRef => | ||
| apply(n, tp.underlying) | ||
| case tp: TypeParamRef => | ||
| ctx.typerState.constraint.entry(tp) match { |
There was a problem hiding this comment.
I believe this is equivalent to
apply(n, ctx.typeComparer.bounds(tp))
But that also points to a problem: What if the type is F-bounded? Would that not give an infinite recursion?
| apply(n, tp.underlying) | ||
| case tp: TypeParamRef => | ||
| ctx.typerState.constraint.entry(tp) match { | ||
| case tb: TypeBounds => foldOver(n, tb) |
There was a problem hiding this comment.
This case seems to be redundant. Is this not covered by the third case?
Also, what if the type is F-bounded? Would that not give an infinite recursion?
There was a problem hiding this comment.
The type being F-bounded is a problem. But we do need to look into those bounds. What are the options here?
| case tp: TypeBounds => | ||
| foldOver(cs, tp) | ||
| case other => | ||
| case tp: TypeRef if tp.prefix.isValueType => |
There was a problem hiding this comment.
What does the prefix.isValueType condition accomplish?
There was a problem hiding this comment.
It was a half-baked attempt at tp.typeSymbol.isClass.
| case tp: TermRef => | ||
| apply(cs, tp.underlying) | ||
| case tp: TypeParamRef => | ||
| ctx.typerState.constraint.entry(tp) match { |
There was a problem hiding this comment.
Same argument as before. First case looks redundant, and what about F-bounds?
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/6329/ to see the changes. Benchmarks is based on merging with master (d39bca0) |
The fix for scala#6058 in scala#6163 caused a significant performance regression in the inductive implicits benchmark because the use of =:= rather than == in the divergence check was significantly slower. It is the right test however, so we need a quicker check to rule out negative cases. We're already computing the covering sets and sizes of the two types being compared. tp1 =:= tp2 should entail (sz1 == sz2 && cs1 == cs2), so the contrapositive (sz1 != sz2 || cs1 != cs2) should entail that !(tp1 =:= tp2). However the covering set and size computations were incorrect (they missed types mentioned in bounds which should have been included, and included symbols for unsolved type variables which should not). This commit fixes the latter issue, which allows covering set and size tests to be used to avoid expensive full type equality tests.
|
@odersky I've eliminated the redundant cases and switched to |
|
test performance please |
|
performance test scheduled: 2 job(s) in queue, 1 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/6329/ to see the changes. Benchmarks is based on merging with master (72e5145) |
| apply(n, ctx.typeComparer.bounds(tp)) | ||
| case _ => | ||
| foldOver(n, tp) | ||
| val seen: util.HashSet[Type] = new util.HashSet[Type](64) { |
There was a problem hiding this comment.
It's probably faster to use a java.util.IdentityHashMap[Type, Type], We already use that in lots of other places in the compiler.
There was a problem hiding this comment.
Or even:
import java.util.{Collections, IdentityHashMap}
val seen = Collections.newSetFromMap[Type](new IdentityHashMap)
|
Now with |
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 1 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/6329/ to see the changes. Benchmarks is based on merging with master (29aeb02) |
|
The CI failures look to be timeout and memory related, and I can't reproduce locally, so I'll restart the CI job and hope for the best. |
|
That fixed it. Merging now. |
This fixes scala#15692 and does not seem to break an existing compilation tests. The problem with seen when it comes scala#15692 is that seen means that type that has repeated types will get a lower size and this incorrectly triggers the divergence check since some of the steps involved less repeated types. The seen logic was introduced in scala#6329 and the motivation was to deal with F-bounds. Since not tests fail it not clear if this logic is still needed to deal with F-bounds? If it is still needed we can add a test and instead of removing the seen logic we can make it track only types the appear as a bound and could cause infinite recursion instead of tracking all.
The fix for #6058 in #6163 caused a significant performance regression in the inductive implicits benchmark because the use of
=:=rather than==in the divergence check was significantly slower. It is the right test however, so we need a quicker check to rule out negative cases.We're already computing the covering sets and sizes of the two types being compared.
tp1 =:= tp2should entail(sz1 == sz2 && cs1 == cs2), so the contrapositive(sz1 != sz2 || cs1 != cs2)should entail that!(tp1 =:= tp2). However the covering set and size computations were incorrect (they missed types mentioned in bounds which should have been included, and included symbols for unsolved type variables which should not).This commit fixes the latter issue, which allows covering set and size tests to be used to avoid expensive full type equality tests.
Fixes the inductive implicits part of #6300.