From d71fea533d4c5dae243954de6904b2a3a50a6c92 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sun, 16 Jul 2017 11:32:10 +0200 Subject: [PATCH 1/7] Null out owning state of TypeVar after instantiation After setting the `inst` field of a TypeVar, the owningState is no longer needed. Null it out in order to avoid a memory leak (TypeStates refer to constraints and error logs and error logs can refer to contexts via unforced diagnostic messages). --- compiler/src/dotty/tools/dotc/core/Types.scala | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/compiler/src/dotty/tools/dotc/core/Types.scala b/compiler/src/dotty/tools/dotc/core/Types.scala index 8c6474ae0e55..d0726e6ae671 100644 --- a/compiler/src/dotty/tools/dotc/core/Types.scala +++ b/compiler/src/dotty/tools/dotc/core/Types.scala @@ -3175,7 +3175,13 @@ object Types { final class TypeVar(val origin: TypeParamRef, creatorState: TyperState, val bindingTree: untpd.Tree, val owner: Symbol) extends CachedProxyType with ValueType { /** The permanent instance type of the variable, or NoType is none is given yet */ - private[core] var inst: Type = NoType + private[this] var myInst: Type = NoType + + private[core] def inst = myInst + private[core] def inst_=(tp: Type) = { + myInst = tp + if (tp.exists) owningState = null // no longer needed; null out to avoid a memory leak + } /** The state owning the variable. This is at first `creatorState`, but it can * be changed to an enclosing state on a commit. From d1486a50a4c050d22ed22f882de66349d5bceaaa Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sun, 16 Jul 2017 12:12:13 +0200 Subject: [PATCH 2/7] Avoid memory leak in mapSymbols --- compiler/src/dotty/tools/dotc/core/Symbols.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/src/dotty/tools/dotc/core/Symbols.scala b/compiler/src/dotty/tools/dotc/core/Symbols.scala index bf4eec0f1649..f7d36fee3a28 100644 --- a/compiler/src/dotty/tools/dotc/core/Symbols.scala +++ b/compiler/src/dotty/tools/dotc/core/Symbols.scala @@ -348,9 +348,9 @@ trait Symbols { this: Context => info = completer, privateWithin = ttmap1.mapOwner(odenot.privateWithin), // since this refers to outer symbols, need not include copies (from->to) in ownermap here. annotations = odenot.annotations) - } + copies.foreach(_.ensureCompleted()) // avoid memory leak copies } From 93e7fd3ee7e51b77f4a7a89e01da2b39ee342d87 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Mon, 17 Jul 2017 11:15:46 +0200 Subject: [PATCH 3/7] Make owningState a weak reference `owningState` of `TypeVar` was still a source of memory leaks even though we null it out when a TypeVar gets instantiated. The problem is that uninstantiated TypeVars can sit as parts of types in caches. --- compiler/src/dotty/tools/dotc/core/TyperState.scala | 6 +++--- compiler/src/dotty/tools/dotc/core/Types.scala | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/TyperState.scala b/compiler/src/dotty/tools/dotc/core/TyperState.scala index 8735a1fe8130..051fb20ee02c 100644 --- a/compiler/src/dotty/tools/dotc/core/TyperState.scala +++ b/compiler/src/dotty/tools/dotc/core/TyperState.scala @@ -11,6 +11,7 @@ import printing.{Showable, Printer} import printing.Texts._ import config.Config import collection.mutable +import java.lang.ref.WeakReference class TyperState(r: Reporter) extends DotClass with Showable { @@ -143,8 +144,7 @@ extends TyperState(r) { if (targetState.constraint eq previousConstraint) constraint else targetState.constraint & constraint constraint foreachTypeVar { tvar => - if (tvar.owningState eq this) - tvar.owningState = targetState + if (tvar.owningState.get eq this) tvar.owningState = new WeakReference(targetState) } targetState.ephemeral |= ephemeral targetState.gc() @@ -157,7 +157,7 @@ extends TyperState(r) { constraint foreachTypeVar { tvar => if (!tvar.inst.exists) { val inst = instType(tvar) - if (inst.exists && (tvar.owningState eq this)) { + if (inst.exists && (tvar.owningState.get eq this)) { tvar.inst = inst val lam = tvar.origin.binder if (constraint.isRemovable(lam)) toCollect += lam diff --git a/compiler/src/dotty/tools/dotc/core/Types.scala b/compiler/src/dotty/tools/dotc/core/Types.scala index d0726e6ae671..a9171fefb410 100644 --- a/compiler/src/dotty/tools/dotc/core/Types.scala +++ b/compiler/src/dotty/tools/dotc/core/Types.scala @@ -36,6 +36,7 @@ import Flags.FlagSet import language.implicitConversions import scala.util.hashing.{ MurmurHash3 => hashing } import config.Printers.{core, typr, cyclicErrors} +import java.lang.ref.WeakReference object Types { @@ -3186,7 +3187,7 @@ object Types { /** The state owning the variable. This is at first `creatorState`, but it can * be changed to an enclosing state on a commit. */ - private[core] var owningState = creatorState + private[core] var owningState = new WeakReference(creatorState) /** The instance type of this variable, or NoType if the variable is currently * uninstantiated @@ -3204,7 +3205,7 @@ object Types { private def instantiateWith(tp: Type)(implicit ctx: Context): Type = { assert(tp ne this, s"self instantiation of ${tp.show}, constraint = ${ctx.typerState.constraint.show}") typr.println(s"instantiating ${this.show} with ${tp.show}") - if ((ctx.typerState eq owningState) && !ctx.typeComparer.subtypeCheckInProgress) + if ((ctx.typerState eq owningState.get) && !ctx.typeComparer.subtypeCheckInProgress) inst = tp ctx.typerState.constraint = ctx.typerState.constraint.replace(origin, tp) tp From ed386bd550f4b3f05abfc129f6e0778338042fff Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Mon, 17 Jul 2017 16:41:28 +0200 Subject: [PATCH 4/7] Store message expressions of ErrorTypes in separate map. Message expressions usually capture contexts, which can cause space leaks if an error type is stored in a retained tree. --- .../src/dotty/tools/dotc/ast/Desugar.scala | 2 +- .../src/dotty/tools/dotc/core/Contexts.scala | 8 ++++++++ .../tools/dotc/core/SymDenotations.scala | 2 +- .../src/dotty/tools/dotc/core/Symbols.scala | 2 +- .../src/dotty/tools/dotc/core/Types.scala | 19 ++++++++++++++++--- .../tools/dotc/typer/ErrorReporting.scala | 2 +- 6 files changed, 28 insertions(+), 7 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/ast/Desugar.scala b/compiler/src/dotty/tools/dotc/ast/Desugar.scala index 4058e1b5cdc3..d311ac834450 100644 --- a/compiler/src/dotty/tools/dotc/ast/Desugar.scala +++ b/compiler/src/dotty/tools/dotc/ast/Desugar.scala @@ -75,7 +75,7 @@ object desugar { else { def msg = s"no matching symbol for ${tp.symbol.showLocated} in ${defctx.owner} / ${defctx.effectiveScope}" - if (ctx.reporter.errorsReported) new ErrorType(msg) + if (ctx.reporter.errorsReported) ErrorType(msg) else throw new java.lang.Error(msg) } case _ => diff --git a/compiler/src/dotty/tools/dotc/core/Contexts.scala b/compiler/src/dotty/tools/dotc/core/Contexts.scala index 2e7979a98c25..97624aab777a 100644 --- a/compiler/src/dotty/tools/dotc/core/Contexts.scala +++ b/compiler/src/dotty/tools/dotc/core/Contexts.scala @@ -24,6 +24,7 @@ import Implicits.ContextualImplicits import config.Settings._ import config.Config import reporting._ +import reporting.diagnostic.Message import collection.mutable import collection.immutable.BitSet import printing._ @@ -636,6 +637,12 @@ object Contexts { */ private[dotty] var unsafeNonvariant: RunId = NoRunId + /** A map from ErrorType to associated message computation. We use this map + * instead of storing message computations directly in ErrorTypes in order + * to avoid space leaks - the message computation usually captures a context. + */ + private[core] val errorTypeMsg = mutable.Map[ErrorType, () => Message]() + // Phases state private[core] var phasesPlan: List[List[Phase]] = _ @@ -662,6 +669,7 @@ object Contexts { def reset() = { for ((_, set) <- uniqueSets) set.clear() + errorTypeMsg.clear() } // Test that access is single threaded diff --git a/compiler/src/dotty/tools/dotc/core/SymDenotations.scala b/compiler/src/dotty/tools/dotc/core/SymDenotations.scala index 3a6eaef14a4b..bfaa12729090 100644 --- a/compiler/src/dotty/tools/dotc/core/SymDenotations.scala +++ b/compiler/src/dotty/tools/dotc/core/SymDenotations.scala @@ -1904,7 +1904,7 @@ object SymDenotations { case denot: ClassDenotation => ClassInfo(denot.owner.thisType, denot.classSymbol, Nil, EmptyScope) case _ => - new ErrorType(errMsg) + ErrorType(errMsg) } denot.privateWithin = NoSymbol } diff --git a/compiler/src/dotty/tools/dotc/core/Symbols.scala b/compiler/src/dotty/tools/dotc/core/Symbols.scala index f7d36fee3a28..b5c3baac2cf1 100644 --- a/compiler/src/dotty/tools/dotc/core/Symbols.scala +++ b/compiler/src/dotty/tools/dotc/core/Symbols.scala @@ -298,7 +298,7 @@ trait Symbols { this: Context => def newSkolem(tp: Type) = newSymbol(defn.RootClass, nme.SKOLEM, SyntheticArtifact | Permanent, tp) def newErrorSymbol(owner: Symbol, name: Name, msg: => Message) = { - val errType = new ErrorType(msg) + val errType = ErrorType(msg) newSymbol(owner, name, SyntheticArtifact, if (name.isTypeName) TypeAlias(errType) else errType) } diff --git a/compiler/src/dotty/tools/dotc/core/Types.scala b/compiler/src/dotty/tools/dotc/core/Types.scala index a9171fefb410..ba508a468f19 100644 --- a/compiler/src/dotty/tools/dotc/core/Types.scala +++ b/compiler/src/dotty/tools/dotc/core/Types.scala @@ -3553,11 +3553,24 @@ object Types { */ abstract class FlexType extends UncachedGroundType with ValueType - class ErrorType(_msg: => Message) extends FlexType { - def msg = _msg + class ErrorType private[Types] () extends FlexType { + def msg(implicit ctx: Context): Message = + ctx.errorTypeMsg.get(this) match { + case Some(msgFun) => msgFun() + case None => "error message from previous run no longer available" + } + } + object ErrorType { + def apply(msg: => Message)(implicit ctx: Context): ErrorType = { + val et = new ErrorType + ctx.base.errorTypeMsg(et) = () => msg + et + } } - object UnspecifiedErrorType extends ErrorType("unspecified error") + object UnspecifiedErrorType extends ErrorType() { + override def msg(implicit ctx: Context): Message = "unspecified error" + } /* Type used to track Select nodes that could not resolve a member and their qualifier is a scala.Dynamic. */ object TryDynamicCallType extends FlexType diff --git a/compiler/src/dotty/tools/dotc/typer/ErrorReporting.scala b/compiler/src/dotty/tools/dotc/typer/ErrorReporting.scala index d0ab39a988c4..fda4f05753e7 100644 --- a/compiler/src/dotty/tools/dotc/typer/ErrorReporting.scala +++ b/compiler/src/dotty/tools/dotc/typer/ErrorReporting.scala @@ -23,7 +23,7 @@ object ErrorReporting { def errorType(msg: => Message, pos: Position)(implicit ctx: Context): ErrorType = { ctx.error(msg, pos) - new ErrorType(msg) + ErrorType(msg) } def cyclicErrorMsg(ex: CyclicReference)(implicit ctx: Context) = { From 016c5d71f29b6eb5936e39badb1db9b7d0d2b03d Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Mon, 17 Jul 2017 17:58:15 +0200 Subject: [PATCH 5/7] Avoid memory leak in TypedSplice --- compiler/src/dotty/tools/dotc/ast/untpd.scala | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/ast/untpd.scala b/compiler/src/dotty/tools/dotc/ast/untpd.scala index 52a49de47f88..a9081947292a 100644 --- a/compiler/src/dotty/tools/dotc/ast/untpd.scala +++ b/compiler/src/dotty/tools/dotc/ast/untpd.scala @@ -29,8 +29,10 @@ object untpd extends Trees.Instance[Untyped] with UntypedTreeInfo { } object TypedSplice { - def apply(tree: tpd.Tree)(implicit ctx: Context): TypedSplice = - new TypedSplice(tree)(ctx.owner) {} + def apply(tree: tpd.Tree)(implicit ctx: Context): TypedSplice = { + val curOwner = ctx.owner // compute here to avoid space leak with a reference from TypedSplice to Context + new TypedSplice(tree)(curOwner) {} + } } /** mods object name impl */ From 1d30bd047f8f7ddeb0cdbb4852fca3a92f5af09f Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Tue, 18 Jul 2017 09:31:24 +0200 Subject: [PATCH 6/7] Avoid memory leak in TypedSplice (reverted from commit 016c5d71f29b6eb5936e39badb1db9b7d0d2b03d) --- compiler/src/dotty/tools/dotc/ast/untpd.scala | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/ast/untpd.scala b/compiler/src/dotty/tools/dotc/ast/untpd.scala index a9081947292a..52a49de47f88 100644 --- a/compiler/src/dotty/tools/dotc/ast/untpd.scala +++ b/compiler/src/dotty/tools/dotc/ast/untpd.scala @@ -29,10 +29,8 @@ object untpd extends Trees.Instance[Untyped] with UntypedTreeInfo { } object TypedSplice { - def apply(tree: tpd.Tree)(implicit ctx: Context): TypedSplice = { - val curOwner = ctx.owner // compute here to avoid space leak with a reference from TypedSplice to Context - new TypedSplice(tree)(curOwner) {} - } + def apply(tree: tpd.Tree)(implicit ctx: Context): TypedSplice = + new TypedSplice(tree)(ctx.owner) {} } /** mods object name impl */ From beb2e4123d17c80b7c7c35f108a779031e7ae591 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Tue, 18 Jul 2017 09:32:57 +0200 Subject: [PATCH 7/7] Fix #2883: Avoid capturing fields accessed in constructor In LambdaLift, fields used only in the constructor of a class should not be marked free in the enclosing class. --- .../dotty/tools/dotc/transform/LambdaLift.scala | 1 + tests/run/i2883.check | 1 + tests/run/i2883.scala | 16 ++++++++++++++++ 3 files changed, 18 insertions(+) create mode 100644 tests/run/i2883.check create mode 100644 tests/run/i2883.scala diff --git a/compiler/src/dotty/tools/dotc/transform/LambdaLift.scala b/compiler/src/dotty/tools/dotc/transform/LambdaLift.scala index 0cb4f3e208f2..c5f5d879178c 100644 --- a/compiler/src/dotty/tools/dotc/transform/LambdaLift.scala +++ b/compiler/src/dotty/tools/dotc/transform/LambdaLift.scala @@ -202,6 +202,7 @@ class LambdaLift extends MiniPhase with IdentityDenotTransformer { thisTransform ctx.debuglog(i"mark free: ${sym.showLocated} with owner ${sym.maybeOwner} marked free in $enclosure") val intermediate = if (enclosure.is(PackageClass)) enclosure + else if (enclosure.isConstructor) markFree(sym, enclosure.owner.enclosure) else markFree(sym, enclosure.enclosure) narrowLiftedOwner(enclosure, intermediate orElse sym.enclosingClass) if (!intermediate.isRealClass || enclosure.isConstructor) { diff --git a/tests/run/i2883.check b/tests/run/i2883.check new file mode 100644 index 000000000000..8b137891791f --- /dev/null +++ b/tests/run/i2883.check @@ -0,0 +1 @@ + diff --git a/tests/run/i2883.scala b/tests/run/i2883.scala new file mode 100644 index 000000000000..d42f715d7f0a --- /dev/null +++ b/tests/run/i2883.scala @@ -0,0 +1,16 @@ +class Wrapper(val value: Int) + +abstract class Foo(val x: Int) + +class Test { + def foo(wrapper: Wrapper): Unit = { + new Foo(wrapper.value) {} + } +} +object Test extends App { + def foo(wrapper: Wrapper): Foo = + new Foo(wrapper.value) {} + def printFields(obj: Any) = + println(obj.getClass.getDeclaredFields.map(_.toString).sorted.deep.mkString("\n")) + printFields(foo(new Wrapper(1))) +}