Skip to content

Implicit scope caching: bug fixes and performance improvements #1288

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Jun 3, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/dotty/tools/dotc/core/Types.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2145,7 +2145,12 @@ object Types {
unique(new CachedAndType(tp1, tp2))
}
def make(tp1: Type, tp2: Type)(implicit ctx: Context): Type =
if (tp1 eq tp2) tp1 else apply(tp1, tp2)
if ((tp1 eq tp2) || (tp2 eq defn.AnyType))
tp1
else if (tp1 eq defn.AnyType)
tp2
else
apply(tp1, tp2)
}

abstract case class OrType(tp1: Type, tp2: Type) extends CachedGroundType with AndOrType {
Expand Down Expand Up @@ -3450,7 +3455,7 @@ object Types {
case TypeBounds(_, hi) =>
apply(x, hi)
case tp: ThisType =>
apply(x, tp.underlying)
apply(x, tp.tref)
case tp: ConstantType =>
apply(x, tp.underlying)
case tp: MethodParam =>
Expand Down
14 changes: 10 additions & 4 deletions src/dotty/tools/dotc/typer/Applications.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1146,11 +1146,17 @@ trait Applications extends Compatibility { self: Typer =>
alts
}

def narrowByTrees(alts: List[TermRef], args: List[Tree], resultType: Type): List[TermRef] =
alts filter ( alt =>
if (!ctx.isAfterTyper) isApplicable(alt, targs, args, resultType)
else isDirectlyApplicable(alt, targs, args, resultType)
def narrowByTrees(alts: List[TermRef], args: List[Tree], resultType: Type): List[TermRef] = {
val alts2 = alts.filter(alt =>
isDirectlyApplicable(alt, targs, args, resultType)
)
if (alts2.isEmpty && !ctx.isAfterTyper)
alts.filter(alt =>
isApplicable(alt, targs, args, resultType)
)
else
alts2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That changes the logic of overloading resolution. With the change, we prefer methods that are applicable without implicit conversions to other methods. That's not as specced I think. What is the motivation for the change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the commit message of 7a060ba:

If a directly applicable alternative exists, there is no need to try
every alternative (which would require searching for implicit
conversions for every argument that doesn't directly match). This should
not affect semantics since applicable overloads that require some
implicit conversions are dismissed in narrowMostSpecific

The motivation for this change is that I observed that many of the implicit searches when compiling dotty were caused by overloading resolution (e.g., every call to Int#+ will check if the argument can be converted to Char,Short,Double, etc even if the argument is an Int) It's possible that I missed a case where this does change the semantics, if so could you give an example?

}

val alts1 = narrowBySize(alts)
//ctx.log(i"narrowed by size: ${alts1.map(_.symbol.showDcl)}%, %")
Expand Down
33 changes: 24 additions & 9 deletions src/dotty/tools/dotc/typer/Implicits.scala
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ object Implicits {
*/
class OfTypeImplicits(tp: Type, val companionRefs: TermRefSet)(initctx: Context) extends ImplicitRefs(initctx) {
assert(initctx.typer != null)
val refs: List[TermRef] = {
lazy val refs: List[TermRef] = {
val buf = new mutable.ListBuffer[TermRef]
for (companion <- companionRefs) buf ++= companion.implicitMembers
buf.toList
Expand Down Expand Up @@ -284,10 +284,13 @@ trait ImplicitRunInfo { self: RunInfo =>
override implicit protected val ctx: Context = liftingCtx
override def stopAtStatic = true
def apply(tp: Type) = tp match {
case tp: TypeRef if tp.symbol.isLambdaTrait =>
defn.AnyType
case tp: TypeRef if tp.symbol.isAbstractOrAliasType =>
val pre = tp.prefix
def joinClass(tp: Type, cls: ClassSymbol) =
AndType(tp, cls.typeRef.asSeenFrom(pre, cls.owner))
if (cls.isLambdaTrait) tp
else AndType.make(tp, cls.typeRef.asSeenFrom(pre, cls.owner))
val lead = if (tp.prefix eq NoPrefix) defn.AnyType else apply(tp.prefix)
(lead /: tp.classSymbols)(joinClass)
case tp: TypeVar =>
Expand Down Expand Up @@ -323,7 +326,7 @@ trait ImplicitRunInfo { self: RunInfo =>
def addParentScope(parent: TypeRef): Unit = {
iscopeRefs(parent) foreach addRef
for (param <- parent.typeParams)
comps ++= iscopeRefs(pre.member(param.name).info)
comps ++= iscopeRefs(tp.member(param.name).info)
}
val companion = cls.companionModule
if (companion.exists) addRef(companion.valRef)
Expand All @@ -338,8 +341,6 @@ trait ImplicitRunInfo { self: RunInfo =>
}
}

def ofTypeImplicits(comps: TermRefSet) = new OfTypeImplicits(tp, comps)(ctx)

/** The implicit scope of type `tp`
* @param isLifted Type `tp` is the result of a `liftToClasses` application
*/
Expand All @@ -349,9 +350,12 @@ trait ImplicitRunInfo { self: RunInfo =>
ctx.typerState.ephemeral = false
try {
val liftedTp = if (isLifted) tp else liftToClasses(tp)
val result =
if (liftedTp ne tp) iscope(liftedTp, isLifted = true)
else ofTypeImplicits(collectCompanions(tp))
val refs =
if (liftedTp ne tp)
iscope(liftedTp, isLifted = true).companionRefs
else
collectCompanions(tp)
val result = new OfTypeImplicits(tp, refs)(ctx)
if (ctx.typerState.ephemeral) record("ephemeral cache miss: implicitScope")
else if (cacheResult) implicitScopeCache(tp) = result
result
Expand All @@ -363,7 +367,18 @@ trait ImplicitRunInfo { self: RunInfo =>
computeIScope(cacheResult = false)
else implicitScopeCache get tp match {
case Some(is) => is
case None => computeIScope(cacheResult = seen.isEmpty)
case None =>
// Implicit scopes are tricky to cache because of loops. For example
// in `tests/pos/implicit-scope-loop.scala`, the scope of B contains
// the scope of A which contains the scope of B. We break the loop
// by returning EmptyTermRefSet in `collectCompanions` for types
// that we have already seen, but this means that we cannot cache
// the computed scope of A, it is incomplete.
// Keeping track of exactly where these loops happen would require a
// lot of book-keeping, instead we choose to be conservative and only
// cache scopes before any type has been seen. This is unfortunate
// because loops are very common for types in scala.collection.
computeIScope(cacheResult = seen.isEmpty)
}
}

Expand Down
17 changes: 17 additions & 0 deletions tests/pos/implicit-scope-loop.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
trait Dummy[T]


trait A[T] extends B
trait B extends Dummy[A[Int]]
object B {
implicit def theB: B = new B {}
implicit def theA: A[Int] = new A[Int] {}
}

object Test {
def getB(implicit b: B) = b
def getA[T](implicit a: A[T]) = a

getB
getA
}
16 changes: 16 additions & 0 deletions tests/pos/implicit_cache.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
class A
object A {
implicit def theA: A = new A
}
class Foo[T]
object Foo {
implicit def theFoo: Foo[A] = new Foo[A]
}

object Test {
def getFooA(implicit foo: Foo[A]) = foo
def getA(implicit a: A) = a

getFooA
getA
}
12 changes: 12 additions & 0 deletions tests/pos/implicit_tparam.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
class Foo[T]
class Bar extends Foo[A]

class A
object A {
implicit val bar: Bar = new Bar
}

object Test {
def getBar(implicit bar: Bar) = bar
getBar
}
1 change: 1 addition & 0 deletions tests/run/overload_directly_applicable.check
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
C1
16 changes: 16 additions & 0 deletions tests/run/overload_directly_applicable.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
class A
class B

class C1 {
def f(x: A): Unit = println("C1")
}
class C2 extends C1 {
def f(x: B): Unit = println("C2")
}

object Test extends C2 {
implicit def a2b(x: A): B = new B
def main(args: Array[String]): Unit = {
f(new A)
}
}