Skip to content

Commit

Permalink
Desugar mixed in module var and accessor in refchecks
Browse files Browse the repository at this point in the history
Rather than leaving it until mixin.

The current code in mixin is used for both lazy val and modules,
and puts the "slow path" code that uses the monitor into a
dedicated method (`moduleName$lzyCompute`). I tracked this
back to a3d4d17. I can't tell from that commit whether the
performance sensititivity was related to modules or lazy vals,
from the commit message I'd say the latter.

As the initialization code for a module is just a constructor call,
rather than an arbitraryly large chunk of code for a lazy initializer,
this commit opts to inline the `lzycompute` method.
  • Loading branch information
retronym committed Aug 25, 2015
1 parent 3d62009 commit a9e7492
Show file tree
Hide file tree
Showing 6 changed files with 58 additions and 58 deletions.
5 changes: 5 additions & 0 deletions src/compiler/scala/tools/nsc/ast/TreeGen.scala
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ abstract class TreeGen extends scala.reflect.internal.TreeGen with TreeDSL {
def lhsRef = if (lhs.owner.isClass) Select(This(lhs.owner), lhs) else Ident(lhs)
Block(Assign(lhsRef, rhs) :: Nil, lhsRef)
}
def mkDoubleCheckedLockAssignAndReturn(lhs: Symbol, rhs: Tree): Tree = {
val Block(List(assign @ Assign(moduleVarRef, _)), returnTree) = gen.mkAssignAndReturn(lhs, rhs)
val cond = Apply(Select(moduleVarRef, Object_eq), List(Literal(Constant(null))))
If(cond, gen.mkSynchronizedCheck(This(lhs.owner), cond, List(assign), List(returnTree)), returnTree)
}

def newModule(accessor: Symbol, tpe: Type) = {
val ps = tpe.typeSymbol.primaryConstructor.info.paramTypes
Expand Down
35 changes: 1 addition & 34 deletions src/compiler/scala/tools/nsc/transform/Mixin.scala
Original file line number Diff line number Diff line change
Expand Up @@ -861,16 +861,6 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL {
typedPos(init.head.pos)(mkFastPathLazyBody(clazz, lzyVal, cond, syncBody, nulls, retVal))
}

def mkInnerClassAccessorDoubleChecked(attrThis: Tree, rhs: Tree, moduleSym: Symbol, args: List[Tree]): Tree =
rhs match {
case Block(List(assign), returnTree) =>
val Assign(moduleVarRef, _) = assign
val cond = Apply(Select(moduleVarRef, Object_eq), List(NULL))
mkFastPathBody(clazz, moduleSym, cond, List(assign), List(NULL), returnTree, attrThis, args)
case _ =>
abort(s"Invalid getter $rhs for module in $clazz")
}

def mkCheckedAccessor(clazz: Symbol, retVal: Tree, offset: Int, pos: Position, fieldSym: Symbol): Tree = {
val sym = fieldSym.getterIn(fieldSym.owner)
val bitmapSym = bitmapFor(clazz, offset, sym)
Expand Down Expand Up @@ -926,18 +916,6 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL {
deriveDefDef(stat)(rhs => Block(List(rhs, localTyper.typed(mkSetFlag(clazz, fieldOffset(getter), getter, bitmapKind(getter)))), UNIT))
else stat
}
else if (sym.isModule && (!clazz.isTrait || clazz.isImplClass) && !sym.isBridge) {
deriveDefDef(stat)(rhs =>
typedPos(stat.pos)(
mkInnerClassAccessorDoubleChecked(
// Martin to Hubert: I think this can be replaced by selfRef(tree.pos)
// @PP: It does not seem so, it crashes for me trying to bootstrap.
if (clazz.isImplClass) gen.mkAttributedIdent(stat.vparamss.head.head.symbol) else gen.mkAttributedThis(clazz),
rhs, sym, stat.vparamss.head
)
)
)
}
else stat
}
stats map {
Expand Down Expand Up @@ -1082,18 +1060,7 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL {
})
}
else if (sym.isModule && !(sym hasFlag LIFTED | BRIDGE)) {
// add modules
val vsym = sym.owner.newModuleVarSymbol(sym)
addDef(position(sym), ValDef(vsym))

// !!! TODO - unravel the enormous duplication between this code and
// eliminateModuleDefs in RefChecks.
val rhs = gen.newModule(sym, vsym.tpe)
val assignAndRet = gen.mkAssignAndReturn(vsym, rhs)
val attrThis = gen.mkAttributedThis(clazz)
val rhs1 = mkInnerClassAccessorDoubleChecked(attrThis, assignAndRet, sym, List())

addDefDef(sym, rhs1)
// Moved to Refchecks
}
else if (!sym.isMethod) {
// add fields
Expand Down
52 changes: 30 additions & 22 deletions src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1188,26 +1188,6 @@ abstract class RefChecks extends InfoTransform with scala.reflect.internal.trans
// set NoType so it will be ignored.
val cdef = ClassDef(module.moduleClass, impl) setType NoType

// Create the module var unless the immediate owner is a class and
// the module var already exists there. See SI-5012, SI-6712.
def findOrCreateModuleVar() = {
val vsym = (
if (site.isTerm) NoSymbol
else site.info decl nme.moduleVarName(moduleName)
)
vsym orElse (site newModuleVarSymbol module)
}
def newInnerObject() = {
// Create the module var unless it is already in the module owner's scope.
// The lookup is on module.enclClass and not module.owner lest there be a
// nullary method between us and the class; see SI-5012.
val moduleVar = findOrCreateModuleVar()
val rhs = gen.newModule(module, moduleVar.tpe)
val body = if (site.isTrait) rhs else gen.mkAssignAndReturn(moduleVar, rhs)
val accessor = DefDef(module, body.changeOwner(moduleVar -> module))

ValDef(moduleVar) :: accessor :: Nil
}
def matchingInnerObject() = {
val newFlags = (module.flags | STABLE) & ~MODULE
val newInfo = NullaryMethodType(module.moduleClass.tpe)
Expand All @@ -1219,10 +1199,37 @@ abstract class RefChecks extends InfoTransform with scala.reflect.internal.trans
if (module.isStatic)
if (module.isOverridingSymbol) matchingInnerObject() else Nil
else
newInnerObject()
newInnerObject(site, module)
)
transformTrees(newTrees map localTyper.typedPos(moduleDef.pos))
}
def newInnerObject(site: Symbol, module: Symbol): List[Tree] = {
// Create the module var unless the immediate owner is a class and
// the module var already exists there. See SI-5012, SI-6712.
def findOrCreateModuleVar() = {
val vsym = (
if (site.isTerm) NoSymbol
else site.info decl nme.moduleVarName(module.name.toTermName)
)
vsym orElse (site newModuleVarSymbol module)
}
// Create the module var unless it is already in the module owner's scope.
// The lookup is on module.enclClass and not module.owner lest there be a
// nullary method between us and the class; see SI-5012.
val moduleVar = findOrCreateModuleVar()
val rhs = gen.newModule(module, moduleVar.tpe)
val body = if (site.isTrait) rhs else if (site.isTerm) gen.mkAssignAndReturn(moduleVar, rhs) else gen.mkDoubleCheckedLockAssignAndReturn(moduleVar, rhs)
val accessor = if (module.owner == site) module else site.newMethod(module.name.toTermName, site.pos, STABLE).setInfo(moduleVar.tpe)
val accessorDef = DefDef(accessor, body.changeOwner(moduleVar -> accessor))

ValDef(moduleVar) :: accessorDef :: Nil
}

def elimMixinModuleDef(clazz: Symbol): List[Tree] = {
val mixinModules = if (clazz.isTrait) Nil else clazz.mixinClasses.flatMap(_.info.decls.iterator.filter(_.isModule))
val newMembers = mixinModules flatMap (module => newInnerObject(clazz, module)) map localTyper.typedPos(clazz.pos)
transformTrees(newMembers)
}

def transformStat(tree: Tree, index: Int): List[Tree] = tree match {
case t if treeInfo.isSelfConstrCall(t) =>
Expand Down Expand Up @@ -1651,11 +1658,12 @@ abstract class RefChecks extends InfoTransform with scala.reflect.internal.trans
// SI-7870 default getters for constructors live in the companion module
checkOverloadedRestrictions(currentOwner, currentOwner.companionModule)
val bridges = addVarargBridges(currentOwner)
val moduleDesugared = elimMixinModuleDef(currentOwner)
checkAllOverrides(currentOwner)
checkAnyValSubclass(currentOwner)
if (currentOwner.isDerivedValueClass)
currentOwner.primaryConstructor makeNotPrivate NoSymbol // SI-6601, must be done *after* pickler!
if (bridges.nonEmpty) deriveTemplate(tree)(_ ::: bridges) else tree
if (bridges.nonEmpty || moduleDesugared.nonEmpty) deriveTemplate(tree)(_ ::: bridges ::: moduleDesugared) else tree

case dc@TypeTreeWithDeferredRefCheck() => abort("adapt should have turned dc: TypeTreeWithDeferredRefCheck into tpt: TypeTree, with tpt.original == dc")
case tpt@TypeTree() =>
Expand Down
2 changes: 1 addition & 1 deletion src/reflect/scala/reflect/internal/Symbols.scala
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
def newModuleVarSymbol(accessor: Symbol): TermSymbol = {
val newName = nme.moduleVarName(accessor.name.toTermName)
val newFlags = MODULEVAR | ( if (this.isClass) PrivateLocal | SYNTHETIC else 0 )
val newInfo = accessor.tpe.finalResultType
val newInfo = thisType.memberType(accessor).finalResultType
val mval = newVariable(newName, accessor.pos.focus, newFlags.toLong) addAnnotation VolatileAttr

if (this.isClass)
Expand Down
2 changes: 1 addition & 1 deletion src/reflect/scala/reflect/internal/transform/Erasure.scala
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ trait Erasure {
case tref @ TypeRef(pre, sym, args) =>
if (sym == ArrayClass)
if (unboundedGenericArrayLevel(tp) == 1) ObjectTpe
else if (args.head.typeSymbol.isBottomClass) arrayType(ObjectTpe)
else if (args.head.typeSymbol.isBottomClass) arrayType(ObjectTpe)
else typeRef(apply(pre), sym, args map applyInArray)
else if (sym == AnyClass || sym == AnyValClass || sym == SingletonClass) ObjectTpe
else if (sym == UnitClass) BoxedUnitTpe
Expand Down
20 changes: 20 additions & 0 deletions test/files/run/trait-defaults-modules.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
trait T1 { def a: Any }

trait T2 extends T1 { object a; object b }
trait T3 extends T2

class C1 extends T1 { object a; object b }
class C2 extends C1
class C3 extends T2
class C4 extends T3

object Test {
def main(args: Array[String]): Unit = {
val (c1, c2, c3, c4) = (new C1, new C2, new C3, new C4)
c1.a; c1.b; (c1: T1).a
c2.a; c2.b; (c2: T1).a
c3.a; c3.b; (c3: T1).a
c4.a; c4.b; (c4: T1).a
}

}

0 comments on commit a9e7492

Please sign in to comment.