-
-
Notifications
You must be signed in to change notification settings - Fork 175
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
Remove unnecessary cache #628
Conversation
Because the conversion from Class to KClass is cached on the Kotlin side
@@ -34,17 +32,11 @@ internal class ReflectionCache(reflectionCacheSize: Int) { | |||
} | |||
} | |||
|
|||
private val javaClassToKotlin = LRUMap<Class<Any>, KClass<Any>>(reflectionCacheSize, reflectionCacheSize) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Class
-> KClass
conversion is cached on the Kotlin
side as of 1.5.32, so there is no need to cache it in jackson-module-kotlin
anymore.
https://github.com/JetBrains/kotlin/blob/v1.5.32/core/reflection.jvm/src/kotlin/reflect/jvm/internal/kClassCache.kt
private val javaConstructorToKotlin = LRUMap<Constructor<Any>, KFunction<Any>>(reflectionCacheSize, reflectionCacheSize) | ||
private val javaMethodToKotlin = LRUMap<Method, KFunction<*>>(reflectionCacheSize, reflectionCacheSize) | ||
private val javaExecutableToValueCreator = LRUMap<Executable, ValueCreator<*>>(reflectionCacheSize, reflectionCacheSize) | ||
private val javaConstructorIsCreatorAnnotated = LRUMap<AnnotatedConstructor, Boolean>(reflectionCacheSize, reflectionCacheSize) | ||
private val javaMemberIsRequired = LRUMap<AnnotatedMember, BooleanTriState?>(reflectionCacheSize, reflectionCacheSize) | ||
private val kotlinGeneratedMethod = LRUMap<AnnotatedMethod, Boolean>(reflectionCacheSize, reflectionCacheSize) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cache was no longer being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't usually handle review here, but this makes sense to me so +1.
@cowtowncoder |
@k163377 I did not set it up, but Maven plugin "japicmp-maven-plugin" was recently set up to guard against API compatibility changes by maintainers (I think @dinomite remembers this) -- alas, release notes haven't been kept up to date so I don't see the PR/issue number.
The idea being it flags anything that even theoretically breaks backwards-compatibility -- and removal of a protected field is theoretically such thing (since someone could have sub-classed class, referenced field).
so you will probably need to add one or more excludes. I went and removed ones that were left over from 2.13 -> 2.14 cases. |
@k163377 I was able to reproduce the issue and add exclusion to basically disable compatibility check for To get this check to run locally you need to call
since |
SSIA
Like #627, this change also mitigates #584.