Skip to content

Commit

Permalink
Improve test for JVM intrinsics (#2642)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
sandwwraith committed Apr 18, 2024
1 parent c7bcaf1 commit da020f9
Show file tree
Hide file tree
Showing 6 changed files with 40 additions and 20 deletions.
2 changes: 1 addition & 1 deletion build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -252,4 +252,4 @@ tasks.named("dokkaHtmlMultiModule") {

tasks.withType(org.jetbrains.kotlin.gradle.targets.js.npm.tasks.KotlinNpmInstallTask).configureEach {
args.add("--ignore-engines")
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -169,6 +168,13 @@ internal interface SerializerCache<T> {
* Returns cached serializer or `null` if serializer not found.
*/
fun get(key: KClass<Any>): KSerializer<T>?

/**
* 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
}

/**
Expand Down
16 changes: 0 additions & 16 deletions core/commonTest/src/kotlinx/serialization/CachedSerializersTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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<Object>()
}
val indirectMs = indirect.inWholeMicroseconds
if (indirectMs > directMs + (directMs / 4)) error("Direct ($directMs) and indirect ($indirectMs) times are too far apart")
}
}

13 changes: 13 additions & 0 deletions core/jvmMain/src/kotlinx/serialization/internal/Caching.kt
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@ private class ClassValueCache<T>(val compute: (KClass<*>) -> KSerializer<T>?) :
.getOrSet(key.java) { CacheEntry(compute(key)) }
.serializer
}

override fun isStored(key: KClass<*>): Boolean {
return classValue.isStored(key.java)
}
}

/**
Expand Down Expand Up @@ -85,6 +89,11 @@ private class ClassValueReferences<T> : ClassValue<MutableSoftReference<T>>() {
return ref.getOrSetWithLock { factory() }
}

fun isStored(key: Class<*>): Boolean {
val ref = get(key)
return ref.reference.get() != null
}

}

/**
Expand Down Expand Up @@ -134,6 +143,10 @@ private class ConcurrentHashMapCache<T>(private val compute: (KClass<*>) -> KSer
CacheEntry(compute(key))
}.serializer
}

override fun isStored(key: KClass<*>): Boolean {
return cache.containsKey(key.java)
}
}


Expand Down
19 changes: 18 additions & 1 deletion core/jvmTest/src/kotlinx/serialization/CachingTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<T>()")
val ser2 = serializer<Target>()
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<Target> was not intrinsified." +
"Check that compiler plugin intrinsics are enabled and working correctly."
)
val ser3 = serializer(typeOf<Target>())
assertTrue(SERIALIZERS_CACHE.isStored(Target::class), "Serializer should be stored in cache after typeOf-based lookup")
}
}

0 comments on commit da020f9

Please sign in to comment.