diff options
author | Miguel Garcia <miguelalfredo.garcia@epfl.ch> | 2012-06-25 23:53:13 +0200 |
---|---|---|
committer | Miguel Garcia <miguelalfredo.garcia@epfl.ch> | 2012-06-25 23:53:13 +0200 |
commit | 85d312053d43bdc8361abebc5a0bd998ff61e429 (patch) | |
tree | f1732be2620de8523ae7d5d23da19735a1bbd70d /src/compiler | |
parent | f43de697595eaebcf2feec03beb95daf606ac678 (diff) | |
download | scala-85d312053d43bdc8361abebc5a0bd998ff61e429.tar.gz scala-85d312053d43bdc8361abebc5a0bd998ff61e429.tar.bz2 scala-85d312053d43bdc8361abebc5a0bd998ff61e429.zip |
inliner makes fields public only when actually inlining
Diffstat (limited to 'src/compiler')
-rw-r--r-- | src/compiler/scala/tools/nsc/backend/icode/analysis/TypeFlowAnalysis.scala | 15 | ||||
-rw-r--r-- | src/compiler/scala/tools/nsc/backend/opt/Inliners.scala | 217 |
2 files changed, 157 insertions, 75 deletions
diff --git a/src/compiler/scala/tools/nsc/backend/icode/analysis/TypeFlowAnalysis.scala b/src/compiler/scala/tools/nsc/backend/icode/analysis/TypeFlowAnalysis.scala index d31eafff48..258466030f 100644 --- a/src/compiler/scala/tools/nsc/backend/icode/analysis/TypeFlowAnalysis.scala +++ b/src/compiler/scala/tools/nsc/backend/icode/analysis/TypeFlowAnalysis.scala @@ -551,14 +551,23 @@ abstract class TypeFlowAnalysis { val relevantBBs = mutable.Set.empty[BasicBlock] + /* + * Rationale to prevent some methods from ever being inlined: + * + * (1) inlining getters and setters results in exposing a private field, + * which may itself prevent inlining of the caller (at best) or + * lead to situations like SI-5442 ("IllegalAccessError when mixing optimized and unoptimized bytecode") + * + * (2) only invocations having a receiver object are considered (ie no static-methods are ever inlined). + * This is taken care of by checking `isDynamic` (ie virtual method dispatch) and `Static(true)` (ie calls to private members) + */ private def isPreCandidate(cm: opcodes.CALL_METHOD): Boolean = { val msym = cm.method val style = cm.style - // Dynamic == normal invocations - // Static(true) == calls to private members + + !msym.isAccessor && !msym.isConstructor && !blackballed(msym) && (style.isDynamic || (style.hasInstance && style.isStatic)) - // && !(msym hasAnnotation definitions.ScalaNoInlineClass) } override def init(m: icodes.IMethod) { diff --git a/src/compiler/scala/tools/nsc/backend/opt/Inliners.scala b/src/compiler/scala/tools/nsc/backend/opt/Inliners.scala index f332e8cfdd..f34faee459 100644 --- a/src/compiler/scala/tools/nsc/backend/opt/Inliners.scala +++ b/src/compiler/scala/tools/nsc/backend/opt/Inliners.scala @@ -12,6 +12,29 @@ import scala.tools.nsc.symtab._ import scala.reflect.internal.util.NoSourceFile /** + * Inliner balances two competing goals: + * (a) aggressive inlining of: + * (a.1) the apply methods of anonymous closures, so that their anon-classes can be eliminated; + * (a.2) higher-order-methods defined in an external library, e.g. `Range.foreach()` among many others. + * (b) circumventing the barrier to inter-library inlining that private accesses in the callee impose. + * + * Summing up the discussion in SI-5442 and SI-5891, + * the current implementation achieves to a large degree both goals above, and + * overcomes a problem exhibited by previous versions: + * + * (1) Problem: Attempting to access a private member `p` at runtime resulting in an `IllegalAccessError`, + * where `p` is defined in a library L, and is accessed from a library C (for Client), + * where C was compiled against L', an optimized version of L where the inliner made `p` public at the bytecode level. + * The only such members are fields, either synthetic or isParamAccessor, and thus having a dollar sign in their name + * (the accesibility of methods and constructors isn't touched by the inliner). + * + * Thus we add one more goal to our list: + * (c) Compile C (either optimized or not) against any of L or L', + * so that it runs with either L or L' (in particular, compile against L' and run with L). + * + * The chosen strategy is described in some detail in the comments for `accessRequirements()` and `potentiallyPublicized()`. + * Documentation at http://lamp.epfl.ch/~magarcia/ScalaCompilerCornerReloaded/2011Q4/Inliner.pdf + * * @author Iulian Dragos */ abstract class Inliners extends SubComponent { @@ -50,6 +73,8 @@ abstract class Inliners extends SubComponent { ) def lookup(clazz: Symbol): Symbol = { // println("\t\tlooking up " + meth + " in " + clazz.fullName + " meth.owner = " + meth.owner) + assert(clazz != NoSymbol, "Walked up past Object.superClass looking for " + sym + + ", most likely this reveals the TFA at fault (receiver and callee don't match).") if (sym.owner == clazz || isBottomType(clazz)) sym else sym.overridingSymbol(clazz) match { case NoSymbol => if (sym.owner.isTrait) sym else lookup(clazz.superClass) @@ -86,13 +111,27 @@ abstract class Inliners extends SubComponent { def name = phaseName val inliner = new Inliner - override def apply(c: IClass) { - inliner analyzeClass c + object iclassOrdering extends Ordering[IClass] { + def compare(a: IClass, b: IClass) = { + val sourceNamesComparison = (a.cunit.toString() compare b.cunit.toString()) + if(sourceNamesComparison != 0) sourceNamesComparison + else { + val namesComparison = (a.toString() compare b.toString()) + if(namesComparison != 0) namesComparison + else { + a.symbol.id compare b.symbol.id + } + } + } } + val queue = new mutable.PriorityQueue[IClass]()(iclassOrdering) + + override def apply(c: IClass) { queue += c } override def run() { try { super.run() + for(c <- queue) { inliner analyzeClass c } } finally { inliner.clearCaches() } @@ -174,12 +213,22 @@ abstract class Inliners extends SubComponent { tfa.isOnWatchlist.clear() } + object imethodOrdering extends Ordering[IMethod] { + def compare(a: IMethod, b: IMethod) = { + val namesComparison = (a.toString() compare b.toString()) + if(namesComparison != 0) namesComparison + else { + a.symbol.id compare b.symbol.id + } + } + } + def analyzeClass(cls: IClass): Unit = if (settings.inline.value) { debuglog("Analyzing " + cls) this.currentIClazz = cls - val ms = cls.methods filterNot { _.symbol.isConstructor } + val ms = cls.methods filterNot { _.symbol.isConstructor } sorted imethodOrdering ms foreach { im => if(hasInline(im.symbol)) { log("Not inlining into " + im.symbol.originalName.decode + " because it is marked @inline.") @@ -221,7 +270,7 @@ abstract class Inliners extends SubComponent { * The ensuing analysis of each candidate (performed by `analyzeInc()`) * may result in a CFG isomorphic to that of the callee being inserted in place of the callsite * (i.e. a CALL_METHOD instruction is replaced with a single-entry single-exit CFG, - * a situation we call "successful inlining"). + * a substitution we call "successful inlining"). * * (3) following iterations have `relevantBBs` updated to focus on the inlined basic blocks and their successors only. * Details in `MTFAGrowable.reinit()` @@ -280,11 +329,13 @@ abstract class Inliners extends SubComponent { } /** - Decides whether it's feasible and desirable to inline the body of the method given by `concreteMethod` - at the program point given by `i` (a callsite). The boolean result indicates whether inlining was performed. - + * Decides whether it's feasible and desirable to inline the body of the method given by `concreteMethod` + * at the program point given by `i` (a callsite). The boolean result indicates whether inlining was performed. + * */ def analyzeInc(i: CALL_METHOD, bb: BasicBlock, receiver: Symbol, stackLength: Int, concreteMethod: Symbol): Boolean = { + assert(bb.toList contains i, "Candidate callsite does not belong to BasicBlock.") + var inlined = false val msym = i.method @@ -332,25 +383,26 @@ abstract class Inliners extends SubComponent { val pair = new CallerCalleeInfo(caller, inc, fresh, inlinedMethodCount) if (pair isStampedForInlining stackLength) { + + inc.doMakePublic() // `helperIsSafeToInline() populated a list on which `doMakePublic()` acts. + if(inc.accessNeeded == NonPublicRefs.Public) { tfa.knownSafe += inc.sym } + retry = true inlined = true - if (isCountable) - count += 1 + if (isCountable) { count += 1 }; pair.doInline(bb, i) - if (!inc.inline || inc.isMonadic) - caller.inlinedCalls += 1 + if (!inc.inline || inc.isMonadic) { caller.inlinedCalls += 1 }; inlinedMethodCount(inc.sym) += 1 - /* Remove this method from the cache, as the calls-private relation - * might have changed after the inlining. - */ + // Remove the caller from the cache (this inlining might have changed its calls-private relation). usesNonPublics -= m recentTFAs -= m.symbol } else { - if (settings.debug.value) - pair logFailure stackLength + if(inc.accessNeeded == NonPublicRefs.Public) { tfa.knownUnsafe += inc.sym } + + if (settings.debug.value) { pair logFailure stackLength } warnNoInline(pair failureReason stackLength) } @@ -398,6 +450,7 @@ abstract class Inliners extends SubComponent { tfa.reinit(m, staleOut.toList, splicedBlocks, staleIn) tfa.run + staleOut.clear() splicedBlocks.clear() staleIn.clear() @@ -522,6 +575,67 @@ abstract class Inliners extends SubComponent { def addLocals(ls: List[Local]) = m.locals ++= ls def addLocal(l: Local) = addLocals(List(l)) def addHandlers(exhs: List[ExceptionHandler]) = m.exh = exhs ::: m.exh + + private var toBecomePublic: List[Symbol] = Nil + + lazy val accessNeeded: NonPublicRefs.Value = { + + def check(sym: Symbol, cond: Boolean) = + if (cond) Private + else if (sym.isProtected) Protected + else Public + + def canMakePublic(f: Symbol): Boolean = + (m.sourceFile ne NoSourceFile) && (f.isSynthetic || f.isParamAccessor) && + { toBecomePublic = f :: toBecomePublic; true } + + /* A safety check to consider as private, for the purposes of inlining, + * a public field that is presumed synthetic (due to a dollar sign in its name), + * where the field was potentially publicized by `doMakePublic()`. + * We don't want to rely on it being public, as under other compilation conditions that won't be the case. */ + def potentiallyPublicized(f: Symbol): Boolean = { + (m.sourceFile eq NoSourceFile) && f.name.containsChar('$') + } + + def checkField(f: Symbol) = check(f, potentiallyPublicized(f) || + (f.isPrivate && !canMakePublic(f))) + def checkSuper(n: Symbol) = check(n, n.isPrivate || !n.isClassConstructor) + def checkMethod(n: Symbol) = check(n, n.isPrivate) + + def getAccess(i: Instruction) = i match { + case CALL_METHOD(n, SuperCall(_)) => checkSuper(n) + case CALL_METHOD(n, _) => checkMethod(n) + case LOAD_FIELD(f, _) => checkField(f) + case STORE_FIELD(f, _) => checkField(f) + case _ => Public + } + + toBecomePublic = Nil + var seen = Public + val iter = instructions.iterator + while((seen ne Private) && iter.hasNext) { + val i = iter.next() + getAccess(i) match { + case Private => + log("instruction " + i + " requires private access.") + toBecomePublic = Nil + seen = Private + case Protected => seen = Protected + case _ => () + } + } + + seen + } + + def doMakePublic() { + for(f <- toBecomePublic) { + debuglog("Making not-private symbol out of synthetic: " + f) + f setFlag Flags.notPRIVATE + } + toBecomePublic = Nil + } + } class CallerCalleeInfo(val caller: IMethodInfo, val inc: IMethodInfo, fresh: mutable.Map[String, Int], inlinedMethodCount: collection.Map[Symbol, Int]) { @@ -711,7 +825,11 @@ abstract class Inliners extends SubComponent { def isStampedForInlining(stackLength: Int) = !sameSymbols && inc.m.hasCode && shouldInline && - isSafeToInline(stackLength) // `isSafeToInline()` must be invoked last in this AND expr bc it mutates the `knownSafe` and `knownUnsafe` maps for good. + isSafeToInline(stackLength) && { + // `isSafeToInline(stackLength, true)` must be invoked last in this AND expr because + // it prepares input for `doMakePublic()`, which in turn is invoked right before `doInline()`. + true + } def logFailure(stackLength: Int) = log( """|inline failed for %s: @@ -753,71 +871,26 @@ abstract class Inliners extends SubComponent { if(tfa.blackballed(inc.sym)) { return false } if(tfa.knownSafe(inc.sym)) { return true } - if(helperIsSafeToInline(stackLength)) { - tfa.knownSafe += inc.sym; true - } else { - tfa.knownUnsafe += inc.sym; false - } + helperIsSafeToInline(stackLength) } private def helperIsSafeToInline(stackLength: Int): Boolean = { - def makePublic(f: Symbol): Boolean = - /* - * Completely disabling member publifying. This shouldn't have been done in the first place. :| - */ - false - // (inc.m.sourceFile ne NoSourceFile) && (f.isSynthetic || f.isParamAccessor) && { - // debuglog("Making not-private symbol out of synthetic: " + f) - - // f setNotFlag Flags.PRIVATE - // true - // } if (!inc.m.hasCode || inc.isRecursive) { return false } if (inc.m.symbol.hasFlag(Flags.SYNCHRONIZED)) { return false } - val accessNeeded = usesNonPublics.getOrElseUpdate(inc.m, { - // Avoiding crashing the compiler if there are open blocks. - inc.openBlocks foreach { b => - warn(inc.sym.pos, - "Encountered open block in isSafeToInline: this indicates a bug in the optimizer!\n" + - " caller = " + caller.m + ", callee = " + inc.m + inc.openBlocks foreach { b => + warn(inc.sym.pos, + "Encountered open block in isSafeToInline: this indicates a bug in the optimizer!\n" + + " caller = " + caller.m + ", callee = " + inc.m ) - return false - } - def check(sym: Symbol, cond: Boolean) = - if (cond) Private - else if (sym.isProtected) Protected - else Public - - def checkField(f: Symbol) = check(f, f.isPrivate && !makePublic(f)) - def checkSuper(m: Symbol) = check(m, m.isPrivate || !m.isClassConstructor) - def checkMethod(m: Symbol) = check(m, m.isPrivate) - - def getAccess(i: Instruction) = i match { - case CALL_METHOD(m, SuperCall(_)) => checkSuper(m) - case CALL_METHOD(m, _) => checkMethod(m) - case LOAD_FIELD(f, _) => checkField(f) - case STORE_FIELD(f, _) => checkField(f) - case _ => Public - } - - def iterate(): NonPublicRefs.Value = inc.instructions.foldLeft(Public)((res, inc) => getAccess(inc) match { - case Private => log("instruction " + inc + " requires private access.") ; return Private - case Protected => Protected - case Public => res - }) - iterate() - }) + return false + } - canAccess(accessNeeded) && { - val isIllegalStack = (stackLength > inc.minimumStack && inc.hasNonFinalizerHandler) + val isIllegalStack = (stackLength > inc.minimumStack && inc.hasNonFinalizerHandler) + if(isIllegalStack) { debuglog("method " + inc.sym + " is used on a non-empty stack with finalizer.") } - !isIllegalStack || { - debuglog("method " + inc.sym + " is used on a non-empty stack with finalizer.") - false - } - } + !isIllegalStack && canAccess(inc.accessNeeded) } /** Decide whether to inline or not. Heuristics: |