From 55c0581b476381fe66ff0df2ada44560f6511648 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Thu, 6 Oct 2016 17:45:38 +1100 Subject: SI-9946 make nullification of lazy val dependencies module aware If a non-transient lazy val is the only user of a private field in a class, the field is nulled out at the end of the lazy initializer. This is tested in the existing test `run/lazy-leaks.scala`. The analysis of which fields could be nulled out was recently moved from `mixin` to the new `fields` phase. This introduced a regression as a it didn't account for the richer pallete of trees and symbols at that juncture. This commit excludes references to private member modules from collection of private fields, thus avoiding a later compiler crash in the backend due to a nonsense tree trying to null out the module symbol. It might make sense to null out the module var, but I've opted to limit the scope of this analysis to paramaccessors and regular fields. --- .../scala/tools/nsc/transform/AccessorSynthesis.scala | 2 +- test/files/run/t9946a.scala | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) create mode 100644 test/files/run/t9946a.scala diff --git a/src/compiler/scala/tools/nsc/transform/AccessorSynthesis.scala b/src/compiler/scala/tools/nsc/transform/AccessorSynthesis.scala index 120ee5c26e..f1ac2287e2 100644 --- a/src/compiler/scala/tools/nsc/transform/AccessorSynthesis.scala +++ b/src/compiler/scala/tools/nsc/transform/AccessorSynthesis.scala @@ -380,7 +380,7 @@ trait AccessorSynthesis extends Transform with ast.TreeDSL { case tree: RefTree if tree.symbol != NoSymbol => val sym = tree.symbol // println(s"$sym in ${sym.owner} from $currentOwner ($tree)") - if ((sym.hasAccessorFlag || (sym.isTerm && !sym.isMethod)) && sym.isPrivate && !sym.isLazy // non-lazy private field or its accessor + if ((sym.hasAccessorFlag || (sym.isTerm && !sym.isMethod)) && sym.isPrivate && !sym.isLazy && !sym.isModule // non-lazy private field or its accessor && !definitions.isPrimitiveValueClass(sym.tpe.resultType.typeSymbol) // primitives don't hang on to significant amounts of heap && sym.owner == currentOwner.enclClass && !(currentOwner.isGetter && currentOwner.accessed == sym)) { diff --git a/test/files/run/t9946a.scala b/test/files/run/t9946a.scala new file mode 100644 index 0000000000..491fb31f7b --- /dev/null +++ b/test/files/run/t9946a.scala @@ -0,0 +1,14 @@ +package p1 { + object O { + private case class N(a: Any) + lazy val x: AnyRef = N + lazy val y: AnyRef = new { assert(N != null) } + } +} + +object Test { + def main(args: Array[String]): Unit = { + p1.O.x + p1.O.y + } +} -- cgit v1.2.3 From ef14a9af16d988e6240c8a3943fa3df84ee42606 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Thu, 6 Oct 2016 17:27:51 +1100 Subject: SI-9946 don't null field in lazy accessors that turn out to be live If a non-transient lazy val is the only user of a private field in a class, the field is nulled out at the end of the lazy initializer. This is tested in the existing test `run/lazy-leaks.scala`. The analysis of which fields could be nulled out was recently moved from `mixin` to the new `fields` phase. This introduced a regression as a reference from an inner- or companion-classes had not yet been processed by `explicitouter` to publicise private fields. This commit delays the analysis to mixin (after explicit outer has done its work.) Navigating from `foo$lzycompute()` to `foo()` to `foo` is a little dirty now. I'm not sure whether there is a more robust way to structure things. --- .../tools/nsc/transform/AccessorSynthesis.scala | 66 +------------------ .../scala/tools/nsc/transform/Fields.scala | 10 +-- src/compiler/scala/tools/nsc/transform/Mixin.scala | 77 ++++++++++++++++++++++ .../scala/reflect/internal/TypeDebugging.scala | 2 +- test/files/run/t9946b.scala | 12 ++++ test/files/run/t9946c.scala | 10 +++ 6 files changed, 104 insertions(+), 73 deletions(-) create mode 100644 test/files/run/t9946b.scala create mode 100644 test/files/run/t9946c.scala diff --git a/src/compiler/scala/tools/nsc/transform/AccessorSynthesis.scala b/src/compiler/scala/tools/nsc/transform/AccessorSynthesis.scala index f1ac2287e2..a1923ead21 100644 --- a/src/compiler/scala/tools/nsc/transform/AccessorSynthesis.scala +++ b/src/compiler/scala/tools/nsc/transform/AccessorSynthesis.scala @@ -326,16 +326,7 @@ trait AccessorSynthesis extends Transform with ast.TreeDSL { * * This way the inliner should optimize the fast path because the method body is small enough. */ - def expandLazyClassMember(lazyVar: Symbol, lazyAccessor: Symbol, transformedRhs: Tree, nullables: Map[Symbol, List[Symbol]]): Tree = { - // use cast so that specialization can turn null.asInstanceOf[T] into null.asInstanceOf[Long] - def nullify(sym: Symbol) = - Select(thisRef, sym.accessedOrSelf) === gen.mkAsInstanceOf(NULL, sym.info.resultType) - - val nulls = nullables.getOrElse(lazyAccessor, Nil) map nullify - - if (nulls.nonEmpty) - log("nulling fields inside " + lazyAccessor + ": " + nulls) - + def expandLazyClassMember(lazyVar: global.Symbol, lazyAccessor: global.Symbol, transformedRhs: global.Tree): Tree = { val slowPathSym = slowPathFor(lazyAccessor) val rhsAtSlowDef = transformedRhs.changeOwner(lazyAccessor -> slowPathSym) @@ -346,7 +337,7 @@ trait AccessorSynthesis extends Transform with ast.TreeDSL { def needsInit = mkTest(lazyAccessor) val doInit = Block(List(storeRes), mkSetFlag(lazyAccessor)) // the slow part of double-checked locking (TODO: is this the most efficient pattern? https://github.come/scala/scala-dev/issues/204) - val slowPathRhs = Block(gen.mkSynchronized(thisRef)(If(needsInit, doInit, EmptyTree)) :: nulls, selectVar) + val slowPathRhs = Block(gen.mkSynchronized(thisRef)(If(needsInit, doInit, EmptyTree)) :: Nil, selectVar) // The lazy accessor delegates to the compute method if needed, otherwise just accesses the var (it was initialized previously) // `if ((bitmap&n & MASK) == 0) this.l$compute() else l$` @@ -358,59 +349,6 @@ trait AccessorSynthesis extends Transform with ast.TreeDSL { } } - /** Map lazy values to the fields they should null after initialization. */ - // TODO: fix - def lazyValNullables(clazz: Symbol, templStats: List[Tree]): Map[Symbol, List[Symbol]] = { - // if there are no lazy fields, take the fast path and save a traversal of the whole AST - if (!clazz.info.decls.exists(_.isLazy)) Map() - else { - // A map of single-use fields to the lazy value that uses them during initialization. - // Each field has to be private and defined in the enclosing class, and there must - // be exactly one lazy value using it. - // - // Such fields will be nulled after the initializer has memoized the lazy value. - val singleUseFields: Map[Symbol, List[Symbol]] = { - val usedIn = mutable.HashMap[Symbol, List[Symbol]]() withDefaultValue Nil - - object SingleUseTraverser extends Traverser { - override def traverse(tree: Tree) { - tree match { - // assignment targets don't count as a dereference -- only check the rhs - case Assign(_, rhs) => traverse(rhs) - case tree: RefTree if tree.symbol != NoSymbol => - val sym = tree.symbol - // println(s"$sym in ${sym.owner} from $currentOwner ($tree)") - if ((sym.hasAccessorFlag || (sym.isTerm && !sym.isMethod)) && sym.isPrivate && !sym.isLazy && !sym.isModule // non-lazy private field or its accessor - && !definitions.isPrimitiveValueClass(sym.tpe.resultType.typeSymbol) // primitives don't hang on to significant amounts of heap - && sym.owner == currentOwner.enclClass && !(currentOwner.isGetter && currentOwner.accessed == sym)) { - - // println("added use in: " + currentOwner + " -- " + tree) - usedIn(sym) ::= currentOwner - } - super.traverse(tree) - case _ => super.traverse(tree) - } - } - } - templStats foreach SingleUseTraverser.apply - // println("usedIn: " + usedIn) - - // only consider usages from non-transient lazy vals (SI-9365) - val singlyUsedIn = usedIn filter { case (_, member :: Nil) => member.isLazy && !member.accessed.hasAnnotation(TransientAttr) case _ => false } toMap - - // println("singlyUsedIn: " + singlyUsedIn) - singlyUsedIn - } - - val map = mutable.Map[Symbol, Set[Symbol]]() withDefaultValue Set() - // invert the map to see which fields can be nulled for each non-transient lazy val - for ((field, users) <- singleUseFields; lazyFld <- users) map(lazyFld) += field - - map.mapValues(_.toList sortBy (_.id)).toMap - } - } - - class SynthInitCheckedAccessorsIn(protected val clazz: Symbol) extends SynthCheckedAccessorsTreesInClass with CheckInitAccessorSymbolSynth { private object addInitBitsTransformer extends Transformer { private def checkedGetter(lhs: Tree)(pos: Position) = { diff --git a/src/compiler/scala/tools/nsc/transform/Fields.scala b/src/compiler/scala/tools/nsc/transform/Fields.scala index aa2ccd9788..f66e00ce1a 100644 --- a/src/compiler/scala/tools/nsc/transform/Fields.scala +++ b/src/compiler/scala/tools/nsc/transform/Fields.scala @@ -659,7 +659,7 @@ abstract class Fields extends InfoTransform with ast.TreeDSL with TypingTransfor val lazyVar = lazyVarOf(getter) val rhs = cast(Apply(selectSuper, Nil), lazyVar.info) - synthAccessorInClass.expandLazyClassMember(lazyVar, getter, rhs, Map.empty) + synthAccessorInClass.expandLazyClassMember(lazyVar, getter, rhs) } (afterOwnPhase { clazz.info.decls } toList) filter checkAndClearNeedsTrees map { @@ -715,7 +715,7 @@ abstract class Fields extends InfoTransform with ast.TreeDSL with TypingTransfor // note that `LazyAccessorTreeSynth` is pretty lightweight // (it's just a bunch of methods that all take a `clazz` parameter, which is thus stored as a field) val synthAccessorInClass = new SynthLazyAccessorsIn(currOwner) - synthAccessorInClass.expandLazyClassMember(lazyVarOf(statSym), statSym, transformedRhs, nullables.getOrElse(currOwner, Map.empty)) + synthAccessorInClass.expandLazyClassMember(lazyVarOf(statSym), statSym, transformedRhs) } // drop the val for (a) constant (pure & not-stored) and (b) not-stored (but still effectful) fields @@ -744,8 +744,6 @@ abstract class Fields extends InfoTransform with ast.TreeDSL with TypingTransfor if (stat.isTerm) atOwner(exprOwner)(transform(stat)) else transform(stat) - private val nullables = perRunCaches.newMap[Symbol, Map[Symbol, List[Symbol]]] - override def transformStats(stats: List[Tree], exprOwner: Symbol): List[Tree] = { val addedStats = if (!currentOwner.isClass || currentOwner.isPackageClass) Nil @@ -756,10 +754,6 @@ abstract class Fields extends InfoTransform with ast.TreeDSL with TypingTransfor else thickets } - val inRealClass = currentOwner.isClass && !(currentOwner.isPackageClass || currentOwner.isTrait) - if (inRealClass) - nullables(currentOwner) = lazyValNullables(currentOwner, stats) - val newStats = stats mapConserve (if (exprOwner != currentOwner) transformTermsAtExprOwner(exprOwner) else transform) diff --git a/src/compiler/scala/tools/nsc/transform/Mixin.scala b/src/compiler/scala/tools/nsc/transform/Mixin.scala index 582c51b90d..e62a12ce67 100644 --- a/src/compiler/scala/tools/nsc/transform/Mixin.scala +++ b/src/compiler/scala/tools/nsc/transform/Mixin.scala @@ -10,6 +10,7 @@ package transform import symtab._ import Flags._ import scala.annotation.tailrec +import scala.collection.mutable abstract class Mixin extends InfoTransform with ast.TreeDSL with AccessorSynthesis { @@ -363,11 +364,13 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL with AccessorSynthes private val rootContext = erasure.NoContext.make(EmptyTree, rootMirror.RootClass, newScope) + private val nullables = mutable.AnyRefMap[Symbol, Map[Symbol, List[Symbol]]]() /** The first transform; called in a pre-order traversal at phase mixin * (that is, every node is processed before its children). * What transform does: * - For every non-trait class, add all mixed in members to the class info. + * - For every non-trait class, assign null to singly used private fields after use in lazy initialization. */ private def preTransform(tree: Tree): Tree = { val sym = tree.symbol @@ -381,12 +384,86 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL with AccessorSynthes else if (currentOwner.isTrait) publicizeTraitMethods(currentOwner) + if (!currentOwner.isTrait) + nullables(currentOwner) = lazyValNullables(currentOwner, body) + tree + case dd: DefDef if dd.symbol.name.endsWith(nme.LAZY_SLOW_SUFFIX) => + val fieldsToNull = nullables.getOrElse(sym.enclClass, Map()).getOrElse(sym, Nil) + if (fieldsToNull.isEmpty) dd + else { + deriveDefDef(dd) { + case blk@Block(stats, expr) => + assert(dd.symbol.originalOwner.isClass, dd.symbol) + def nullify(sym: Symbol) = + Select(gen.mkAttributedThis(sym.enclClass), sym.accessedOrSelf) === NULL + val stats1 = stats ::: fieldsToNull.map(nullify) + treeCopy.Block(blk, stats1, expr) + case tree => + devWarning("Unexpected tree shape in lazy slow path") + tree + } + } case _ => tree } } + /** Map lazy values to the fields they should null after initialization. */ + def lazyValNullables(clazz: Symbol, templStats: List[Tree]): Map[Symbol, List[Symbol]] = { + // if there are no lazy fields, take the fast path and save a traversal of the whole AST + if (!clazz.info.decls.exists(_.isLazy)) Map() + else { + // A map of single-use fields to the lazy value that uses them during initialization. + // Each field has to be private and defined in the enclosing class, and there must + // be exactly one lazy value using it. + // + // Such fields will be nulled after the initializer has memoized the lazy value. + val singleUseFields: Map[Symbol, List[Symbol]] = { + val usedIn = mutable.HashMap[Symbol, List[Symbol]]() withDefaultValue Nil + + object SingleUseTraverser extends Traverser { + override def traverse(tree: Tree) { + tree match { + // assignment targets don't count as a dereference -- only check the rhs + case Assign(_, rhs) => traverse(rhs) + case tree: RefTree if tree.symbol != NoSymbol => + val sym = tree.symbol + // println(s"$sym in ${sym.owner} from $currentOwner ($tree)") + if ((sym.hasAccessorFlag || (sym.isTerm && !sym.isMethod)) && sym.isPrivate && !sym.isLazy && !sym.isModule // non-lazy private field or its accessor + && !definitions.isPrimitiveValueClass(sym.tpe.resultType.typeSymbol) // primitives don't hang on to significant amounts of heap + && sym.owner == currentOwner.enclClass && !(currentOwner.isGetter && currentOwner.accessed == sym)) { + + // println("added use in: " + currentOwner + " -- " + tree) + usedIn(sym) ::= currentOwner + } + super.traverse(tree) + case _ => super.traverse(tree) + } + } + } + templStats foreach SingleUseTraverser.apply + // println("usedIn: " + usedIn) + + // only consider usages from non-transient lazy vals (SI-9365) + val singlyUsedIn = usedIn.filter { + case (_, member :: Nil) if member.name.endsWith(nme.LAZY_SLOW_SUFFIX) => + val lazyAccessor = member.owner.info.decl(member.name.stripSuffix(nme.LAZY_SLOW_SUFFIX)) + !lazyAccessor.accessedOrSelf.hasAnnotation(TransientAttr) + case _ => false + }.toMap + + // println("singlyUsedIn: " + singlyUsedIn) + singlyUsedIn + } + + val map = mutable.Map[Symbol, Set[Symbol]]() withDefaultValue Set() + // invert the map to see which fields can be nulled for each non-transient lazy val + for ((field, users) <- singleUseFields; lazyFld <- users) map(lazyFld) += field + + map.mapValues(_.toList sortBy (_.id)).toMap + } + } /** Add all new definitions to a non-trait class * diff --git a/src/reflect/scala/reflect/internal/TypeDebugging.scala b/src/reflect/scala/reflect/internal/TypeDebugging.scala index 63f897cd32..e9050b4e33 100644 --- a/src/reflect/scala/reflect/internal/TypeDebugging.scala +++ b/src/reflect/scala/reflect/internal/TypeDebugging.scala @@ -110,7 +110,7 @@ trait TypeDebugging { val hi_s = if (noPrint(hi)) "" else " <: " + ptTree(hi) lo_s + hi_s case _ if (t.symbol eq null) || (t.symbol eq NoSymbol) => to_s(t) - case _ => "" + t.symbol.tpe + case _ => "" + t.symbol.rawInfo.safeToString } def ptTypeParam(td: TypeDef): String = { val TypeDef(_, name, tparams, rhs) = td diff --git a/test/files/run/t9946b.scala b/test/files/run/t9946b.scala new file mode 100644 index 0000000000..ac102a38f7 --- /dev/null +++ b/test/files/run/t9946b.scala @@ -0,0 +1,12 @@ +class Test(private val x: String) { + lazy val y = x.reverse +} +object Test { + def getX(t: Test) = t.x + def main(args: Array[String]): Unit = { + val t = new Test("foo") + assert(t.y == "oof", t.y) + assert(t.x == "foo", t.x) + } +} + diff --git a/test/files/run/t9946c.scala b/test/files/run/t9946c.scala new file mode 100644 index 0000000000..f9fe68d48f --- /dev/null +++ b/test/files/run/t9946c.scala @@ -0,0 +1,10 @@ +class Test(private[this] val x: String) { + lazy val y = x.reverse +} +object Test { + def main(args: Array[String]): Unit = { + val t = new Test("foo") + assert(t.y == "oof", t.y) + } +} + -- cgit v1.2.3