From a12b22c04d442df96f043c7266a3ce1b2c135133 Mon Sep 17 00:00:00 2001 From: Mads Ager Date: Tue, 24 Aug 2021 09:46:55 +0200 Subject: [PATCH] [JVM] Force lock object in JVM `synchronized` implementation into local. This fixes a performance problem in the case where the lock object is a capture and the monitor enter/exit happens directly on field loads. When the locking happens on field loads instead of a local, the JVM cannot prove that locking is balanced. That has the consequence that the code is runs very slow (always in the interpreter). ^KT-48367 Fixed. --- .../codegen/FirBytecodeTextTestGenerated.java | 6 ++++++ .../testData/codegen/bytecodeText/kt48367.kt | 20 +++++++++++++++++++ .../codegen/BytecodeTextTestGenerated.java | 6 ++++++ .../codegen/IrBytecodeTextTestGenerated.java | 6 ++++++ .../jvm/src/kotlin/util/Synchronized.kt | 9 +++++++-- 5 files changed, 45 insertions(+), 2 deletions(-) create mode 100644 compiler/testData/codegen/bytecodeText/kt48367.kt 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 91cafdac32e..5f18b57e372 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 @@ -307,6 +307,12 @@ public class FirBytecodeTextTestGenerated extends AbstractFirBytecodeTextTest { runTest("compiler/testData/codegen/bytecodeText/kt46615.kt"); } + @Test + @TestMetadata("kt48367.kt") + public void testKt48367() throws Exception { + runTest("compiler/testData/codegen/bytecodeText/kt48367.kt"); + } + @Test @TestMetadata("kt5016.kt") public void testKt5016() throws Exception { diff --git a/compiler/testData/codegen/bytecodeText/kt48367.kt b/compiler/testData/codegen/bytecodeText/kt48367.kt new file mode 100644 index 00000000000..709dbbadefe --- /dev/null +++ b/compiler/testData/codegen/bytecodeText/kt48367.kt @@ -0,0 +1,20 @@ +fun foo(block: () -> String): String = block() +inline fun bar(crossinline f: () -> String) = foo { f() } + +fun flaf() { + val revoked = "A" + bar { + synchronized (revoked) { + "B" + } + } +} + +// The field $revoked$inlined should be loaded only once and stored in a local +// that is used for the monitor enter/exit instructions. Locking and unlocking +// directly on a field load makes it hard for the JVM to prove that locking is +// balanced which causes the code to be interpreted. See KT-48367 for details. + +// 1 GETFIELD Kt48367Kt\$flaf\$\$inlined\$bar\$1.\$revoked\$inlined : Ljava/lang/String; +// 1 MONITORENTER +// 2 MONITOREXIT 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 af31540ddce..bc9c67402e4 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 @@ -295,6 +295,12 @@ public class BytecodeTextTestGenerated extends AbstractBytecodeTextTest { runTest("compiler/testData/codegen/bytecodeText/kt3845.kt"); } + @Test + @TestMetadata("kt48367.kt") + public void testKt48367() throws Exception { + runTest("compiler/testData/codegen/bytecodeText/kt48367.kt"); + } + @Test @TestMetadata("kt5016.kt") public void testKt5016() 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 9169dc70970..b81907d8eb0 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 @@ -307,6 +307,12 @@ public class IrBytecodeTextTestGenerated extends AbstractIrBytecodeTextTest { runTest("compiler/testData/codegen/bytecodeText/kt46615.kt"); } + @Test + @TestMetadata("kt48367.kt") + public void testKt48367() throws Exception { + runTest("compiler/testData/codegen/bytecodeText/kt48367.kt"); + } + @Test @TestMetadata("kt5016.kt") public void testKt5016() throws Exception { diff --git a/libraries/stdlib/jvm/src/kotlin/util/Synchronized.kt b/libraries/stdlib/jvm/src/kotlin/util/Synchronized.kt index c8a855efe68..bcedeff760f 100644 --- a/libraries/stdlib/jvm/src/kotlin/util/Synchronized.kt +++ b/libraries/stdlib/jvm/src/kotlin/util/Synchronized.kt @@ -19,13 +19,18 @@ public actual inline fun synchronized(lock: Any, block: () -> R): R { callsInPlace(block, InvocationKind.EXACTLY_ONCE) } + // Force the lock object into a local and use that local for monitor enter/exit. + // This ensures that the JVM can prove that locking is balanced which is a + // prerequisite for using fast locking implementations. See KT-48367 for details. + val lockLocal = lock + @Suppress("NON_PUBLIC_CALL_FROM_PUBLIC_INLINE", "INVISIBLE_MEMBER") - monitorEnter(lock) + monitorEnter(lockLocal) try { return block() } finally { @Suppress("NON_PUBLIC_CALL_FROM_PUBLIC_INLINE", "INVISIBLE_MEMBER") - monitorExit(lock) + monitorExit(lockLocal) } }