From d07070e184e8adca08f422ce48b7c043b0e7a0cd Mon Sep 17 00:00:00 2001 From: pyos Date: Mon, 13 Sep 2021 19:03:01 +0200 Subject: [PATCH] JVM_IR: avoid redundant accessors in private inline funs Their call sites are all in the same file, so we can check whether the declarations used in the inline function are accessible from all the places where it will be inlined. #KT-48736 Fixed --- .../FirBlackBoxCodegenTestGenerated.java | 6 + .../codegen/FirBytecodeTextTestGenerated.java | 6 + .../jvm/lower/SyntheticAccessorLowering.kt | 129 ++++++++++++------ .../packagePrivateInPrivateInline.kt | 19 +++ ...rForProtectedInSamePackagePrivateInline.kt | 17 +++ .../debug/localVariables/suspend/mergeLvt.kt | 2 - .../codegen/BlackBoxCodegenTestGenerated.java | 6 + .../codegen/BytecodeTextTestGenerated.java | 6 + .../IrBlackBoxCodegenTestGenerated.java | 6 + .../codegen/IrBytecodeTextTestGenerated.java | 6 + .../LightAnalysisModeTestGenerated.java | 5 + 11 files changed, 161 insertions(+), 47 deletions(-) create mode 100644 compiler/testData/codegen/box/syntheticAccessors/packagePrivateInPrivateInline.kt create mode 100644 compiler/testData/codegen/bytecodeText/noAccessorForProtectedInSamePackagePrivateInline.kt diff --git a/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/test/runners/codegen/FirBlackBoxCodegenTestGenerated.java b/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/test/runners/codegen/FirBlackBoxCodegenTestGenerated.java index 4fc8005c420..e91c75892e5 100644 --- a/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/test/runners/codegen/FirBlackBoxCodegenTestGenerated.java +++ b/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/test/runners/codegen/FirBlackBoxCodegenTestGenerated.java @@ -41884,6 +41884,12 @@ public class FirBlackBoxCodegenTestGenerated extends AbstractFirBlackBoxCodegenT runTest("compiler/testData/codegen/box/syntheticAccessors/packagePrivate.kt"); } + @Test + @TestMetadata("packagePrivateInPrivateInline.kt") + public void testPackagePrivateInPrivateInline() throws Exception { + runTest("compiler/testData/codegen/box/syntheticAccessors/packagePrivateInPrivateInline.kt"); + } + @Test @TestMetadata("protectedFromLambda.kt") public void testProtectedFromLambda() throws Exception { diff --git a/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/test/runners/codegen/FirBytecodeTextTestGenerated.java b/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/test/runners/codegen/FirBytecodeTextTestGenerated.java index 9e6a0da7691..65f8719778d 100644 --- a/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/test/runners/codegen/FirBytecodeTextTestGenerated.java +++ b/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/test/runners/codegen/FirBytecodeTextTestGenerated.java @@ -367,6 +367,12 @@ public class FirBytecodeTextTestGenerated extends AbstractFirBytecodeTextTest { runTest("compiler/testData/codegen/bytecodeText/noAccessorForProtectedInSamePackageCrossinline.kt"); } + @Test + @TestMetadata("noAccessorForProtectedInSamePackagePrivateInline.kt") + public void testNoAccessorForProtectedInSamePackagePrivateInline() throws Exception { + runTest("compiler/testData/codegen/bytecodeText/noAccessorForProtectedInSamePackagePrivateInline.kt"); + } + @Test @TestMetadata("noFlagAnnotations.kt") public void testNoFlagAnnotations() throws Exception { diff --git a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/lower/SyntheticAccessorLowering.kt b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/lower/SyntheticAccessorLowering.kt index a41f6bcbbb3..1b960aff094 100644 --- a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/lower/SyntheticAccessorLowering.kt +++ b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/lower/SyntheticAccessorLowering.kt @@ -11,6 +11,7 @@ import org.jetbrains.kotlin.backend.common.descriptors.synthesizedString import org.jetbrains.kotlin.backend.common.ir.* import org.jetbrains.kotlin.backend.jvm.JvmBackendContext import org.jetbrains.kotlin.backend.jvm.JvmLoweredDeclarationOrigin +import org.jetbrains.kotlin.backend.jvm.codegen.fileParent import org.jetbrains.kotlin.backend.jvm.codegen.isJvmInterface import org.jetbrains.kotlin.backend.jvm.intrinsics.receiverAndArgs import org.jetbrains.kotlin.backend.jvm.ir.IrInlineReferenceLocator @@ -19,6 +20,7 @@ import org.jetbrains.kotlin.codegen.AsmUtil import org.jetbrains.kotlin.descriptors.DescriptorVisibilities import org.jetbrains.kotlin.descriptors.DescriptorVisibility import org.jetbrains.kotlin.descriptors.Modality +import org.jetbrains.kotlin.ir.IrElement import org.jetbrains.kotlin.ir.IrStatement import org.jetbrains.kotlin.ir.UNDEFINED_OFFSET import org.jetbrains.kotlin.ir.builders.declarations.addValueParameter @@ -42,6 +44,7 @@ internal class SyntheticAccessorLowering(val context: JvmBackendContext) : IrEle private val pendingAccessorsToAdd = mutableListOf() private val inlineLambdaToCallSite = mutableMapOf() + private val inlineFunctionToCallSites = mutableMapOf>() override fun lower(irFile: IrFile) { irFile.accept(object : IrInlineReferenceLocator(context) { @@ -56,6 +59,24 @@ internal class SyntheticAccessorLowering(val context: JvmBackendContext) : IrEle inlineLambdaToCallSite[argument.symbol.owner] = LambdaCallSite(scope, parameter.isCrossinline && !callee.isCoroutineIntrinsic()) } + + override fun visitSimpleFunction(declaration: IrSimpleFunction, data: IrDeclaration?) { + if (declaration.isPrivateInline) { + inlineFunctionToCallSites.putIfAbsent(declaration, mutableSetOf()) + } + super.visitSimpleFunction(declaration, data) + } + + override fun visitCall(expression: IrCall, data: IrDeclaration?) { + val callee = expression.symbol.owner + if (callee.isPrivateInline && callee.fileParent == irFile && data != null) { + (inlineFunctionToCallSites.getOrPut(callee) { mutableSetOf() } as MutableSet).add(data) + } + super.visitCall(expression, data) + } + + private inline val IrSimpleFunction.isPrivateInline + get() = isInline && DescriptorVisibilities.isPrivate(visibility) }, null) irFile.transformChildrenVoid(this) @@ -752,58 +773,76 @@ internal class SyntheticAccessorLowering(val context: JvmBackendContext) : IrEle } } + private fun getScopeClassOrPackage(): IrDeclarationContainer? = + getScopeClassOrPackage(currentScope?.irElement, approximateToPackage = false) + // Get the class from which all accesses in the current scope will be done after bytecode generation. // If the current scope is a crossinline lambda, this is not possible, as the lambda maybe inlined // into some other class; in that case, get at least the package. - private fun getScopeClassOrPackage(): IrDeclarationContainer? { - var context = currentScope?.irElement - var throughCrossinlineLambda = false - while (context is IrDeclaration) { - val callSite = inlineLambdaToCallSite[context] - when { - // Crossinline lambdas can be inlined into some other class in the same package. However, - // classes within crossinline lambdas should not be regenerated, so if we've already found - // a class *before* reaching this lambda, it's valid: - // class C { - // fun f() {} - // fun g() = inlineFunctionWithCrossinlineArgument { - // f() // this call is done in some unknown class within C's package - // object { val x = f() } // this call is done in C$g$1$1 - // } - // } - callSite != null -> throughCrossinlineLambda = throughCrossinlineLambda || callSite.crossinline - // Inline functions can be inlined into anywhere. Not even private inline functions are safe: - // class C { - // fun f() {} - // private inline fun g1() = f() // `f` is called from C? - // fun g2() = { g1() } // ...or from C$g2$1 in the same package? - // inline fun g3() = g1() // ...or from some other package that calls g3? - // } - // TODO: this has some weird effects for inline functions in local classes, e.g. they - // access the capture fields (package-private) through accessors; this may or may not - // be necessary - local types should in theory not be usable outside the current file. - context is IrFunction && context.isInline -> return null - // TODO: if this class is an object local to an inline function, it could be regenerated, - // so the scope depends on the declaration accessed (see KT-48508): - // class C { - // fun f1() - // inline fun inlineFun() = object { - // fun f2() {} - // fun g1() { - // f1() // this access can be anywhere - // f2() // can pretend this access is from C$foo$1 - // } - // } - // } - // Further complicating things, the accessor for `f1` cannot be in `C$foo$1`, as otherwise - // the accessor itself will be regenerated (and thus not work) at `inlineFun` call sites. - context is IrClass && !throughCrossinlineLambda -> return context + private tailrec fun getScopeClassOrPackage(context: IrElement?, approximateToPackage: Boolean): IrDeclarationContainer? { + val callSite = inlineLambdaToCallSite[context] + return when { + // Crossinline lambdas can be inlined into some other class in the same package. However, + // classes within crossinline lambdas should not be regenerated, so if we've already found + // a class *before* reaching this lambda, it's valid: + // class C { + // fun f() {} + // fun g() = inlineFunctionWithCrossinlineArgument { + // f() // this call is done in some unknown class within C's package + // object { val x = f() } // this call is done in C$g$1$1 + // } + // } + callSite != null -> getScopeClassOrPackage(callSite.scope, approximateToPackage || callSite.crossinline) + // Inline functions can be inlined into anywhere. Not even private inline functions are safe: + // class C { + // fun f() {} + // private inline fun g1() = f() // `f` is called from C? + // fun g2() = { g1() } // ...or from C$g2$1 in the same package? + // inline fun g3() = g1() // ...or from some other package that calls g3? + // } + // However, for private ones we at least know where they're called, so just like inline lambdas, + // we can navigate there. + // + // TODO: this has some weird effects for inline functions in local classes, e.g. they + // access the capture fields (package-private) through accessors; this may or may not + // be necessary - local types should in theory not be usable outside the current file. + context is IrFunction && context.isInline -> { + val callSites = inlineFunctionToCallSites[context] ?: return null + when { + callSites.isEmpty() -> getScopeClassOrPackage(context.parent, approximateToPackage) + callSites.size == 1 -> getScopeClassOrPackage(callSites.single(), approximateToPackage) + else -> { + // TODO: cache the results + @Suppress("NON_TAIL_RECURSIVE_CALL") + val results = callSites.map { getScopeClassOrPackage(it, approximateToPackage = false) ?: return null } + // If all call sites are within a single class, use it. Otherwise, all scopes must be within + // the current file's package. + val single = results.first().takeIf { results.all { other -> it === other } } + getScopeClassOrPackage(single ?: context.parent, approximateToPackage || single == null) + } + } } + // TODO: if this class is an object local to an inline function, it could be regenerated, + // so the scope depends on the declaration accessed (see KT-48508): + // class C { + // fun f1() + // inline fun inlineFun() = object { + // fun f2() {} + // fun g1() { + // f1() // this access can be anywhere + // f2() // can pretend this access is from C$foo$1 + // } + // } + // } + // Further complicating things, the accessor for `f1` cannot be in `C$inlineFun$1`, as otherwise + // the accessor itself will be regenerated (and thus not work) at `inlineFun` call sites. + context is IrClass && !approximateToPackage -> context // Inline lambdas have already been moved out to the containing class, but we still need to check // the containing function (again, see above), so navigate there instead. - context = callSite?.scope ?: context.parent + context is IrDeclaration -> getScopeClassOrPackage(context.parent, approximateToPackage) + // The only non-declaration parent should be the package. + else -> context as? IrPackageFragment } - return context as? IrPackageFragment } } diff --git a/compiler/testData/codegen/box/syntheticAccessors/packagePrivateInPrivateInline.kt b/compiler/testData/codegen/box/syntheticAccessors/packagePrivateInPrivateInline.kt new file mode 100644 index 00000000000..19c0c5f545d --- /dev/null +++ b/compiler/testData/codegen/box/syntheticAccessors/packagePrivateInPrivateInline.kt @@ -0,0 +1,19 @@ +// TARGET_BACKEND: JVM +// FILE: test/J.java +package test; + +public class J { + String packagePrivate = "OK"; +} + +// FILE: test/main.kt +package test + +object O { + fun box() = privateInline() + + // no accessor needed - the only call site is in the same package + private inline fun privateInline(): String = J().packagePrivate +} + +fun box() = O.box() \ No newline at end of file diff --git a/compiler/testData/codegen/bytecodeText/noAccessorForProtectedInSamePackagePrivateInline.kt b/compiler/testData/codegen/bytecodeText/noAccessorForProtectedInSamePackagePrivateInline.kt new file mode 100644 index 00000000000..98ee4cb0734 --- /dev/null +++ b/compiler/testData/codegen/bytecodeText/noAccessorForProtectedInSamePackagePrivateInline.kt @@ -0,0 +1,17 @@ +package a + +open class A { + protected fun protectedFun(): String = "OK" +} + +class BSamePackage: A() { + // known to only be called within `BSamePackage`, so accessors are redundant + private inline fun test(): String = protectedFun() + + fun onlyTestCallSite() = test() +} + +// JVM_TEMPLATES +// 2 INVOKESTATIC a/BSamePackage.access +// JVM_IR_TEMPLATES +// 0 INVOKESTATIC a/BSamePackage.access diff --git a/compiler/testData/debug/localVariables/suspend/mergeLvt.kt b/compiler/testData/debug/localVariables/suspend/mergeLvt.kt index ab681a5af66..eb93fea73fc 100644 --- a/compiler/testData/debug/localVariables/suspend/mergeLvt.kt +++ b/compiler/testData/debug/localVariables/suspend/mergeLvt.kt @@ -39,8 +39,6 @@ suspend fun box() { // test.kt:25 box: // test.kt:26 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null // test.kt:19 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null, $i$f$suspendBar:int=0:int -// test.kt:19 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null, $i$f$suspendBar:int=0:int -// test.kt:14 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null, $i$f$suspendBar:int=0:int, $this$extensionFun$iv$iv:AtomicInt=AtomicInt, $i$f$extensionFun:int=0:int // test.kt:14 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null, $i$f$suspendBar:int=0:int, $this$extensionFun$iv$iv:AtomicInt=AtomicInt, $i$f$extensionFun:int=0:int // test.kt:15 box: $continuation:kotlin.coroutines.Continuation=TestKt$box$1, $result:java.lang.Object=null, $i$f$suspendBar:int=0:int, $this$extensionFun$iv$iv:AtomicInt=AtomicInt, $i$f$extensionFun:int=0:int // test.kt:6 getValue: diff --git a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/BlackBoxCodegenTestGenerated.java b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/BlackBoxCodegenTestGenerated.java index 013a2b0ba75..47b71ed85c3 100644 --- a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/BlackBoxCodegenTestGenerated.java +++ b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/BlackBoxCodegenTestGenerated.java @@ -41728,6 +41728,12 @@ public class BlackBoxCodegenTestGenerated extends AbstractBlackBoxCodegenTest { runTest("compiler/testData/codegen/box/syntheticAccessors/packagePrivate.kt"); } + @Test + @TestMetadata("packagePrivateInPrivateInline.kt") + public void testPackagePrivateInPrivateInline() throws Exception { + runTest("compiler/testData/codegen/box/syntheticAccessors/packagePrivateInPrivateInline.kt"); + } + @Test @TestMetadata("protectedFromLambda.kt") public void testProtectedFromLambda() throws Exception { diff --git a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/BytecodeTextTestGenerated.java b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/BytecodeTextTestGenerated.java index 4bd40099333..1d7d91b958a 100644 --- a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/BytecodeTextTestGenerated.java +++ b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/BytecodeTextTestGenerated.java @@ -355,6 +355,12 @@ public class BytecodeTextTestGenerated extends AbstractBytecodeTextTest { runTest("compiler/testData/codegen/bytecodeText/noAccessorForProtectedInSamePackageCrossinline.kt"); } + @Test + @TestMetadata("noAccessorForProtectedInSamePackagePrivateInline.kt") + public void testNoAccessorForProtectedInSamePackagePrivateInline() throws Exception { + runTest("compiler/testData/codegen/bytecodeText/noAccessorForProtectedInSamePackagePrivateInline.kt"); + } + @Test @TestMetadata("noFlagAnnotations.kt") public void testNoFlagAnnotations() throws Exception { diff --git a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/IrBlackBoxCodegenTestGenerated.java b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/IrBlackBoxCodegenTestGenerated.java index 22c763b7435..44727d52beb 100644 --- a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/IrBlackBoxCodegenTestGenerated.java +++ b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/IrBlackBoxCodegenTestGenerated.java @@ -41884,6 +41884,12 @@ public class IrBlackBoxCodegenTestGenerated extends AbstractIrBlackBoxCodegenTes runTest("compiler/testData/codegen/box/syntheticAccessors/packagePrivate.kt"); } + @Test + @TestMetadata("packagePrivateInPrivateInline.kt") + public void testPackagePrivateInPrivateInline() throws Exception { + runTest("compiler/testData/codegen/box/syntheticAccessors/packagePrivateInPrivateInline.kt"); + } + @Test @TestMetadata("protectedFromLambda.kt") public void testProtectedFromLambda() throws Exception { diff --git a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/IrBytecodeTextTestGenerated.java b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/IrBytecodeTextTestGenerated.java index 0e6dcc48f74..fb806cc3c25 100644 --- a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/IrBytecodeTextTestGenerated.java +++ b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/IrBytecodeTextTestGenerated.java @@ -367,6 +367,12 @@ public class IrBytecodeTextTestGenerated extends AbstractIrBytecodeTextTest { runTest("compiler/testData/codegen/bytecodeText/noAccessorForProtectedInSamePackageCrossinline.kt"); } + @Test + @TestMetadata("noAccessorForProtectedInSamePackagePrivateInline.kt") + public void testNoAccessorForProtectedInSamePackagePrivateInline() throws Exception { + runTest("compiler/testData/codegen/bytecodeText/noAccessorForProtectedInSamePackagePrivateInline.kt"); + } + @Test @TestMetadata("noFlagAnnotations.kt") public void testNoFlagAnnotations() throws Exception { diff --git a/compiler/tests-gen/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java b/compiler/tests-gen/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java index 8d75631b794..916d7eb8929 100644 --- a/compiler/tests-gen/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java +++ b/compiler/tests-gen/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java @@ -33568,6 +33568,11 @@ public class LightAnalysisModeTestGenerated extends AbstractLightAnalysisModeTes runTest("compiler/testData/codegen/box/syntheticAccessors/kt9958Interface.kt"); } + @TestMetadata("packagePrivateInPrivateInline.kt") + public void testPackagePrivateInPrivateInline() throws Exception { + runTest("compiler/testData/codegen/box/syntheticAccessors/packagePrivateInPrivateInline.kt"); + } + @TestMetadata("protectedFromLambda.kt") public void testProtectedFromLambda() throws Exception { runTest("compiler/testData/codegen/box/syntheticAccessors/protectedFromLambda.kt");