From da020f9730ffbac18b380ef25f9a9a3e09745946 Mon Sep 17 00:00:00 2001 From: Leonid Startsev Date: Thu, 18 Apr 2024 19:39:57 +0200 Subject: [PATCH] Improve test for JVM intrinsics (#2642) Old one used time-based approach. It was a good indicator, but in rare circumstances it may have been flaky and produced incorrect results. Flaky time-based tests are problematic for large builds, such as Kotlin's Aggregate build. New approach is based on cache presence and should not give incorrect results. See also #KTI-1726 --- build.gradle | 2 +- .../kotlinx/serialization/SerializersCache.kt | 2 +- .../serialization/internal/Platform.common.kt | 8 +++++++- .../serialization/CachedSerializersTest.kt | 16 ---------------- .../kotlinx/serialization/internal/Caching.kt | 13 +++++++++++++ .../src/kotlinx/serialization/CachingTest.kt | 19 ++++++++++++++++++- 6 files changed, 40 insertions(+), 20 deletions(-) diff --git a/build.gradle b/build.gradle index 5a9d29821e..3ff75141b1 100644 --- a/build.gradle +++ b/build.gradle @@ -252,4 +252,4 @@ tasks.named("dokkaHtmlMultiModule") { tasks.withType(org.jetbrains.kotlin.gradle.targets.js.npm.tasks.KotlinNpmInstallTask).configureEach { args.add("--ignore-engines") -} \ No newline at end of file +} diff --git a/core/commonMain/src/kotlinx/serialization/SerializersCache.kt b/core/commonMain/src/kotlinx/serialization/SerializersCache.kt index d211d4d376..a4327a7fe8 100644 --- a/core/commonMain/src/kotlinx/serialization/SerializersCache.kt +++ b/core/commonMain/src/kotlinx/serialization/SerializersCache.kt @@ -19,7 +19,7 @@ import kotlin.reflect.KType * Cache for non-null non-parametrized and non-contextual serializers. */ @ThreadLocal -private val SERIALIZERS_CACHE = createCache { it.serializerOrNull() ?: it.polymorphicIfInterface() } +internal val SERIALIZERS_CACHE = createCache { it.serializerOrNull() ?: it.polymorphicIfInterface() } /** * Cache for nullable non-parametrized and non-contextual serializers. diff --git a/core/commonMain/src/kotlinx/serialization/internal/Platform.common.kt b/core/commonMain/src/kotlinx/serialization/internal/Platform.common.kt index b6f1e96e99..4bba9a6cfe 100644 --- a/core/commonMain/src/kotlinx/serialization/internal/Platform.common.kt +++ b/core/commonMain/src/kotlinx/serialization/internal/Platform.common.kt @@ -6,7 +6,6 @@ package kotlinx.serialization.internal import kotlinx.serialization.* import kotlinx.serialization.descriptors.* -import kotlin.native.concurrent.* import kotlin.reflect.* internal object InternalHexConverter { @@ -169,6 +168,13 @@ internal interface SerializerCache { * Returns cached serializer or `null` if serializer not found. */ fun get(key: KClass): KSerializer? + + /** + * Use SOLELY for test purposes. + * May return `false` even if `get` returns value. It means that entry was computed, but not + * stored (behavior for all non-JVM platforms). + */ + fun isStored(key: KClass<*>): Boolean = false } /** diff --git a/core/commonTest/src/kotlinx/serialization/CachedSerializersTest.kt b/core/commonTest/src/kotlinx/serialization/CachedSerializersTest.kt index 212169e621..39de561532 100644 --- a/core/commonTest/src/kotlinx/serialization/CachedSerializersTest.kt +++ b/core/commonTest/src/kotlinx/serialization/CachedSerializersTest.kt @@ -61,21 +61,5 @@ class CachedSerializersTest { fun testAbstractSerializersAreSame() { assertSame(Abstract.serializer(), Abstract.serializer()) } - - - @OptIn(ExperimentalTime::class) - @Test - fun testSerializersAreIntrinsified() = jvmOnly { - val m = SerializersModule { } - val direct = measureTime { - Object.serializer() - } - val directMs = direct.inWholeMicroseconds - val indirect = measureTime { - m.serializer() - } - val indirectMs = indirect.inWholeMicroseconds - if (indirectMs > directMs + (directMs / 4)) error("Direct ($directMs) and indirect ($indirectMs) times are too far apart") - } } diff --git a/core/jvmMain/src/kotlinx/serialization/internal/Caching.kt b/core/jvmMain/src/kotlinx/serialization/internal/Caching.kt index 191b30c17c..8ab4c852b3 100644 --- a/core/jvmMain/src/kotlinx/serialization/internal/Caching.kt +++ b/core/jvmMain/src/kotlinx/serialization/internal/Caching.kt @@ -52,6 +52,10 @@ private class ClassValueCache(val compute: (KClass<*>) -> KSerializer?) : .getOrSet(key.java) { CacheEntry(compute(key)) } .serializer } + + override fun isStored(key: KClass<*>): Boolean { + return classValue.isStored(key.java) + } } /** @@ -85,6 +89,11 @@ private class ClassValueReferences : ClassValue>() { return ref.getOrSetWithLock { factory() } } + fun isStored(key: Class<*>): Boolean { + val ref = get(key) + return ref.reference.get() != null + } + } /** @@ -134,6 +143,10 @@ private class ConcurrentHashMapCache(private val compute: (KClass<*>) -> KSer CacheEntry(compute(key)) }.serializer } + + override fun isStored(key: KClass<*>): Boolean { + return cache.containsKey(key.java) + } } diff --git a/core/jvmTest/src/kotlinx/serialization/CachingTest.kt b/core/jvmTest/src/kotlinx/serialization/CachingTest.kt index b146c92053..686ac473a1 100644 --- a/core/jvmTest/src/kotlinx/serialization/CachingTest.kt +++ b/core/jvmTest/src/kotlinx/serialization/CachingTest.kt @@ -6,9 +6,9 @@ package kotlinx.serialization import kotlinx.serialization.internal.* import kotlinx.serialization.modules.* -import org.junit.Test import kotlin.reflect.* import kotlin.test.* +import kotlin.test.Test class CachingTest { @Test @@ -43,4 +43,21 @@ class CachingTest { assertEquals(1, factoryCalled) } + + @Serializable + class Target + + @Test + fun testJvmIntrinsics() { + val ser1 = Target.serializer() + assertFalse(SERIALIZERS_CACHE.isStored(Target::class), "Cache shouldn't have values before call to serializer()") + val ser2 = serializer() + assertFalse( + SERIALIZERS_CACHE.isStored(Target::class), + "Serializer for Target::class is stored in the cache, which means that runtime lookup was performed and call to serializer was not intrinsified." + + "Check that compiler plugin intrinsics are enabled and working correctly." + ) + val ser3 = serializer(typeOf()) + assertTrue(SERIALIZERS_CACHE.isStored(Target::class), "Serializer should be stored in cache after typeOf-based lookup") + } }