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

Remove unnecessary cache #628

Merged
merged 3 commits into from Feb 19, 2023
Merged

Conversation

k163377
Copy link
Contributor

@k163377 k163377 commented Feb 18, 2023

SSIA

Like #627, this change also mitigates #584.

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)
Copy link
Contributor Author

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)
Copy link
Contributor Author

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.

Copy link
Member

@cowtowncoder cowtowncoder left a 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.

@k163377 k163377 merged commit 13526be into FasterXML:2.15 Feb 19, 2023
@k163377 k163377 deleted the remove-unnecessary-cache branch February 19, 2023 07:03
@k163377
Copy link
Contributor Author

k163377 commented Feb 19, 2023

@cowtowncoder
It appears that the merge is causing a METHOD_REMOVED error.
Could you please tell me how to resolve this error?
https://github.com/FasterXML/jackson-module-kotlin/actions/runs/4215048499/jobs/7315912740

@cowtowncoder
Copy link
Member

@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.
But it's this in pom.xml:

            <plugin>
                <groupId>com.github.siom79.japicmp</groupId>
                <artifactId>japicmp-maven-plugin</artifactId>

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).
In practice there are cases where such changes should be made, I think; and for that we have to add exclusions to basically tell "that's ok, don't worry about the change".
These are added under:

                    <parameter>
                        <breakBuildOnBinaryIncompatibleModifications>true</breakBuildOnBinaryIncomp\
atibleModifications>
                        <breakBuildOnSourceIncompatibleModifications>true</breakBuildOnSourceIncomp\
atibleModifications>
                        <excludes>
                           <exclude>com.fasterxml.jackson.module.kotlin.ValueClassBoxSerializer</exclude>
                           ....
                        </excludes>

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.

@cowtowncoder
Copy link
Member

@k163377 I was able to reproduce the issue and add exclusion to basically disable compatibility check for ReflectionCache class.

To get this check to run locally you need to call verify target, so like:

./mvnw clean verify

since install will not run it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants