From f92773cb1006b32e96a976f86eed4794f26291b3 Mon Sep 17 00:00:00 2001 From: Miles Sabin Date: Wed, 17 Apr 2019 12:48:38 +0100 Subject: [PATCH 1/4] Fix inductive implicits performance regression 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 =:= 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. --- .../src/dotty/tools/dotc/core/Types.scala | 22 ++++++++++++++++--- .../dotty/tools/dotc/typer/Implicits.scala | 5 +++-- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/Types.scala b/compiler/src/dotty/tools/dotc/core/Types.scala index d403aa1dd183..b06ca9c0b098 100644 --- a/compiler/src/dotty/tools/dotc/core/Types.scala +++ b/compiler/src/dotty/tools/dotc/core/Types.scala @@ -4973,6 +4973,14 @@ object Types { foldOver(n + 1, tp) case tp: TypeRef if tp.info.isTypeAlias => apply(n, tp.superType) + case tp: TermRef => + apply(n, tp.underlying) + case tp: TypeParamRef => + ctx.typerState.constraint.entry(tp) match { + case tb: TypeBounds => foldOver(n, tb) + case NoType => foldOver(n, tp.underlying) + case inst => foldOver(n, inst) + } case _ => foldOver(n, tp) } @@ -4990,10 +4998,18 @@ object Types { foldOver(cs + sym, tp) case tp: TypeRef if tp.info.isTypeAlias => apply(cs, tp.superType) - case tp: TypeBounds => - foldOver(cs, tp) - case other => + case tp: TypeRef if tp.prefix.isValueType => foldOver(cs + sym, tp) + case tp: TermRef => + apply(cs, tp.underlying) + case tp: TypeParamRef => + ctx.typerState.constraint.entry(tp) match { + case tb: TypeBounds => foldOver(cs, tb) + case NoType => foldOver(cs, tp.underlying) + case inst => foldOver(cs, inst) + } + case other => + foldOver(cs, tp) } } } diff --git a/compiler/src/dotty/tools/dotc/typer/Implicits.scala b/compiler/src/dotty/tools/dotc/typer/Implicits.scala index c488f915013b..3eb8becb2d41 100644 --- a/compiler/src/dotty/tools/dotc/typer/Implicits.scala +++ b/compiler/src/dotty/tools/dotc/typer/Implicits.scala @@ -1413,9 +1413,10 @@ abstract class SearchHistory { outer => if (cand1.ref == cand.ref) { val wideTp = tp.widenExpr lazy val wildTp = wildApprox(wideTp) + lazy val tpSize = wideTp.typeSize if (belowByname && (wildTp <:< wildPt)) false - else if ((wideTp.typeSize < ptSize && wideTp.coveringSet == ptCoveringSet) || (wildTp =:= wildPt)) true - else loop(tl, isByname(tp) || belowByname) + else if (tpSize > ptSize || wideTp.coveringSet != ptCoveringSet) loop(tl, isByname(tp) || belowByname) + else tpSize < ptSize || wildTp =:= wildPt || loop(tl, isByname(tp) || belowByname) } else loop(tl, isByname(tp) || belowByname) } From 8ab7710c22bd897121e2929ab48ca75a970db363 Mon Sep 17 00:00:00 2001 From: Miles Sabin Date: Wed, 24 Apr 2019 11:22:50 +0100 Subject: [PATCH 2/4] Updates following feedback. --- .../src/dotty/tools/dotc/core/Types.scala | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/Types.scala b/compiler/src/dotty/tools/dotc/core/Types.scala index b06ca9c0b098..19a44ab4ae4a 100644 --- a/compiler/src/dotty/tools/dotc/core/Types.scala +++ b/compiler/src/dotty/tools/dotc/core/Types.scala @@ -4973,14 +4973,8 @@ object Types { foldOver(n + 1, tp) case tp: TypeRef if tp.info.isTypeAlias => apply(n, tp.superType) - case tp: TermRef => - apply(n, tp.underlying) case tp: TypeParamRef => - ctx.typerState.constraint.entry(tp) match { - case tb: TypeBounds => foldOver(n, tb) - case NoType => foldOver(n, tp.underlying) - case inst => foldOver(n, inst) - } + apply(n, ctx.typeComparer.bounds(tp)) case _ => foldOver(n, tp) } @@ -4998,16 +4992,13 @@ object Types { foldOver(cs + sym, tp) case tp: TypeRef if tp.info.isTypeAlias => apply(cs, tp.superType) - case tp: TypeRef if tp.prefix.isValueType => + case tp: TypeRef if sym.isClass => foldOver(cs + sym, tp) case tp: TermRef => - apply(cs, tp.underlying) + val tsym = if (tp.termSymbol.is(Param)) tp.underlying.typeSymbol else tp.termSymbol + foldOver(cs + tsym, tp) case tp: TypeParamRef => - ctx.typerState.constraint.entry(tp) match { - case tb: TypeBounds => foldOver(cs, tb) - case NoType => foldOver(cs, tp.underlying) - case inst => foldOver(cs, inst) - } + apply(cs, ctx.typeComparer.bounds(tp)) case other => foldOver(cs, tp) } From 0324994200acf51215371436208eaa5937aea964 Mon Sep 17 00:00:00 2001 From: Miles Sabin Date: Wed, 24 Apr 2019 11:50:36 +0100 Subject: [PATCH 3/4] Add and handle test case with F-bounds. --- .../src/dotty/tools/dotc/core/Types.scala | 76 +++++++++++-------- .../dotty/tools/dotc/CompilationTests.scala | 3 +- tests/neg-custom-args/i6300.scala | 8 ++ 3 files changed, 56 insertions(+), 31 deletions(-) create mode 100644 tests/neg-custom-args/i6300.scala diff --git a/compiler/src/dotty/tools/dotc/core/Types.scala b/compiler/src/dotty/tools/dotc/core/Types.scala index 19a44ab4ae4a..f5ed94c0f2be 100644 --- a/compiler/src/dotty/tools/dotc/core/Types.scala +++ b/compiler/src/dotty/tools/dotc/core/Types.scala @@ -4966,41 +4966,57 @@ object Types { } class TypeSizeAccumulator(implicit ctx: Context) extends TypeAccumulator[Int] { - def apply(n: Int, tp: Type): Int = tp match { - case tp: AppliedType => - foldOver(n + 1, tp) - case tp: RefinedType => - foldOver(n + 1, tp) - case tp: TypeRef if tp.info.isTypeAlias => - apply(n, tp.superType) - case tp: TypeParamRef => - apply(n, ctx.typeComparer.bounds(tp)) - case _ => - foldOver(n, tp) + val seen: util.HashSet[Type] = new util.HashSet[Type](64) { + override def hash(x: Type): Int = System.identityHashCode(x) + override def isEqual(x: Type, y: Type) = x.eq(y) + } + def apply(n: Int, tp: Type): Int = + if (seen contains tp) n + else { + seen.addEntry(tp) + tp match { + case tp: AppliedType => + foldOver(n + 1, tp) + case tp: RefinedType => + foldOver(n + 1, tp) + case tp: TypeRef if tp.info.isTypeAlias => + apply(n, tp.superType) + case tp: TypeParamRef if !seen(tp) => + apply(n, ctx.typeComparer.bounds(tp)) + case _ => + foldOver(n, tp) + } } } class CoveringSetAccumulator(implicit ctx: Context) extends TypeAccumulator[Set[Symbol]] { + val seen: util.HashSet[Type] = new util.HashSet[Type](64) { + override def hash(x: Type): Int = System.identityHashCode(x) + override def isEqual(x: Type, y: Type) = x.eq(y) + } def apply(cs: Set[Symbol], tp: Type): Set[Symbol] = { - val sym = tp.typeSymbol - tp match { - case tp if tp.isTopType || tp.isBottomType => - cs - case tp: AppliedType => - foldOver(cs + sym, tp) - case tp: RefinedType => - foldOver(cs + sym, tp) - case tp: TypeRef if tp.info.isTypeAlias => - apply(cs, tp.superType) - case tp: TypeRef if sym.isClass => - foldOver(cs + sym, tp) - case tp: TermRef => - val tsym = if (tp.termSymbol.is(Param)) tp.underlying.typeSymbol else tp.termSymbol - foldOver(cs + tsym, tp) - case tp: TypeParamRef => - apply(cs, ctx.typeComparer.bounds(tp)) - case other => - foldOver(cs, tp) + if (seen contains tp) cs + else { + seen.addEntry(tp) + tp match { + case tp if tp.isTopType || tp.isBottomType => + cs + case tp: AppliedType => + foldOver(cs + tp.typeSymbol, tp) + case tp: RefinedType => + foldOver(cs + tp.typeSymbol, tp) + case tp: TypeRef if tp.info.isTypeAlias => + apply(cs, tp.superType) + case tp: TypeRef if tp.typeSymbol.isClass => + foldOver(cs + tp.typeSymbol, tp) + case tp: TermRef => + val tsym = if (tp.termSymbol.is(Param)) tp.underlying.typeSymbol else tp.termSymbol + foldOver(cs + tsym, tp) + case tp: TypeParamRef => + apply(cs, ctx.typeComparer.bounds(tp)) + case other => + foldOver(cs, tp) + } } } } diff --git a/compiler/test/dotty/tools/dotc/CompilationTests.scala b/compiler/test/dotty/tools/dotc/CompilationTests.scala index ce81eabc4f0c..b275459485c4 100644 --- a/compiler/test/dotty/tools/dotc/CompilationTests.scala +++ b/compiler/test/dotty/tools/dotc/CompilationTests.scala @@ -171,7 +171,8 @@ class CompilationTests extends ParallelTesting { compileList("duplicate source", List( "tests/neg-custom-args/toplevel-samesource/S.scala", "tests/neg-custom-args/toplevel-samesource/nested/S.scala"), - defaultOptions) + defaultOptions) + + compileFile("tests/neg-custom-args/i6300.scala", allowDeepSubtypes) }.checkExpectedErrors() @Test def fuzzyAll: Unit = { diff --git a/tests/neg-custom-args/i6300.scala b/tests/neg-custom-args/i6300.scala new file mode 100644 index 000000000000..f69766b06b01 --- /dev/null +++ b/tests/neg-custom-args/i6300.scala @@ -0,0 +1,8 @@ +object Test { + class Foo[T <: Foo[T]] + class Bar extends Foo[Bar] + + implicit def i[T <: Foo[T]](implicit t: Foo[Foo[T]]): Foo[T] = ??? + + implicitly[Foo[Bar]] // error +} From d4c80fa518e4848018968373655cc3365e74c25f Mon Sep 17 00:00:00 2001 From: Miles Sabin Date: Tue, 30 Apr 2019 16:46:32 +0100 Subject: [PATCH 4/4] Use an IdentityHashMap instead of a HashSet --- .../src/dotty/tools/dotc/core/Types.scala | 20 +++++++------------ 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/Types.scala b/compiler/src/dotty/tools/dotc/core/Types.scala index f5ed94c0f2be..3bb56a77e7da 100644 --- a/compiler/src/dotty/tools/dotc/core/Types.scala +++ b/compiler/src/dotty/tools/dotc/core/Types.scala @@ -4966,14 +4966,11 @@ object Types { } class TypeSizeAccumulator(implicit ctx: Context) extends TypeAccumulator[Int] { - val seen: util.HashSet[Type] = new util.HashSet[Type](64) { - override def hash(x: Type): Int = System.identityHashCode(x) - override def isEqual(x: Type, y: Type) = x.eq(y) - } + val seen = new java.util.IdentityHashMap[Type, Type] def apply(n: Int, tp: Type): Int = - if (seen contains tp) n + if (seen.get(tp) != null) n else { - seen.addEntry(tp) + seen.put(tp, tp) tp match { case tp: AppliedType => foldOver(n + 1, tp) @@ -4981,7 +4978,7 @@ object Types { foldOver(n + 1, tp) case tp: TypeRef if tp.info.isTypeAlias => apply(n, tp.superType) - case tp: TypeParamRef if !seen(tp) => + case tp: TypeParamRef => apply(n, ctx.typeComparer.bounds(tp)) case _ => foldOver(n, tp) @@ -4990,14 +4987,11 @@ object Types { } class CoveringSetAccumulator(implicit ctx: Context) extends TypeAccumulator[Set[Symbol]] { - val seen: util.HashSet[Type] = new util.HashSet[Type](64) { - override def hash(x: Type): Int = System.identityHashCode(x) - override def isEqual(x: Type, y: Type) = x.eq(y) - } + val seen = new java.util.IdentityHashMap[Type, Type] def apply(cs: Set[Symbol], tp: Type): Set[Symbol] = { - if (seen contains tp) cs + if (seen.get(tp) != null) cs else { - seen.addEntry(tp) + seen.put(tp, tp) tp match { case tp if tp.isTopType || tp.isBottomType => cs