Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve test for JVM intrinsics: #2642

Merged
merged 1 commit into from
Apr 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no other branches of intrinsic, like value classes, e.t.c?

It may not be enough to check only the intrinsic of a regular class class Target

Copy link
Member Author

@sandwwraith sandwwraith Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There aren't any branches depending on class type. Either intrinsics are enabled and replacing serializer<T>() with something, or they are not. We have different branches for caches though (parametrized vs non-parametrized), but that is not what being tested here.


@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")
}
}