Refactor KVI #512

k163377 · 2021-10-30T17:39:26Z

This PR is a partial realization of the first step of the 3-step change I suggested in my comment #439.

Changes from existing

Many of the reflection operations performed by the KotlinValueInstantiator are now cached.
This is expected to speed up the process since the reflection process is omitted.

The results of the benchmark comparison are as follows.
From this result, we can see that it is actually faster.

before

Benchmark                                           Mode  Cnt        Score        Error  Units
c.w.constructor.Benchmarks.useDefaultKotlinMapper  thrpt    9   639074.213 ±  19600.918  ops/s
c.w.constructor.Benchmarks.useJavaMapper           thrpt    9  2883418.695 ± 117219.003  ops/s
c.w.constructor.Benchmarks.useKotlinMapper         thrpt    9  1163349.939 ±  34378.903  ops/s
c.w.factory.Benchmarks.useDefaultKotlinMapper      thrpt    9   556424.552 ±  10447.484  ops/s
c.w.factory.Benchmarks.useJavaMapper               thrpt    9  2848669.598 ±  25770.187  ops/s
c.w.factory.Benchmarks.useKotlinMapper             thrpt    9   571255.239 ±  14027.060  ops/s

after

Benchmark                                           Mode  Cnt        Score        Error  Units
c.w.constructor.Benchmarks.useDefaultKotlinMapper  thrpt    9   775820.306 ±  40891.839  ops/s
c.w.constructor.Benchmarks.useJavaMapper           thrpt    9  2823625.426 ± 107884.530  ops/s
c.w.constructor.Benchmarks.useKotlinMapper         thrpt    9  1312132.093 ±   6461.389  ops/s
c.w.factory.Benchmarks.useDefaultKotlinMapper      thrpt    9   749118.995 ±   2194.277  ops/s
c.w.factory.Benchmarks.useJavaMapper               thrpt    9  2873057.532 ± 142978.335  ops/s
c.w.factory.Benchmarks.useKotlinMapper             thrpt    9   820204.472 ±  32521.224  ops/s

Changes from #439

The class that was originally named Instantiator has been changed to ValueCreator.
Because the name Instantiator seemed to be confusing with the keyword derived from Jackson.

Also, the content that was originally defined as just an interface has been changed to a sealed class.
This makes it easier to standardize the process and improve the handling of conditional expressions.

Others

I don't think it's necessary to make it ExperimentalDeserializationBackend, since many of the changes are based on existing content.
Therefore, I have not incorporated the relevant changes.

Also, to keep the size of the PR small, I have not yet refactored the KVI.createFromObjectWith function, such as splitting it.
I plan to do that after this PR is merged.

k163377 · 2021-10-30T17:42:09Z

src/main/kotlin/com/fasterxml/jackson/module/kotlin/ValueCreator.kt

+            return
+        }
+
+        throw IllegalAccessException("Cannot access to function or companion object instance, target: $callable")


The error message here is a summary of what was originally individual content.
Should I follow the existing content and give a more specific message for each one?

Also, I couldn't think of any good error message content, so I would appreciate any suggestions.

I'm not sure what you mean by individual content

I don't remember exactly what I was thinking when I left this comment because it's been so long since I created the PR...

I think I was probably worrying about the following two points.

Whether people who read this error message will be able to respond appropriately

Should I indicate specifically which of constructor, factory function, or companion object was inaccessible?

Ahh, yes, if that's possible I think it could prove to be quite helpful

dinomite · 2022-01-02T21:06:20Z

src/main/kotlin/com/fasterxml/jackson/module/kotlin/ValueCreator.kt

+            return
+        }
+
+        throw IllegalAccessException("Cannot access to function or companion object instance, target: $callable")


I'm not sure what you mean by individual content

dinomite · 2022-01-02T21:07:06Z

src/main/kotlin/com/fasterxml/jackson/module/kotlin/ReflectionCache.kt

+                }
+        }
+        else -> throw IllegalStateException("Expected a constructor or method to create a Kotlin object, instead found ${_withArgsCreator.annotated.javaClass.name}")
+    } // we cannot reflect this method so do the default Java-ish behavior


Won't the else -> throw prevent any further processing? Perhaps it should be else -> null?

The reason is that the following process has been ported as is.

jackson-module-kotlin/src/main/kotlin/com/fasterxml/jackson/module/kotlin/KotlinValueInstantiator.kt

Lines 40 to 48 in 6023113

val callable = when (_withArgsCreator) {

is AnnotatedConstructor -> cache.kotlinFromJava(_withArgsCreator.annotated as Constructor<Any>)

is AnnotatedMethod -> cache.kotlinFromJava(_withArgsCreator.annotated as Method)

else -> throw IllegalStateException("Expected a constructor or method to create a Kotlin object, instead found ${_withArgsCreator.annotated.javaClass.name}")

} ?: return super.createFromObjectWith(

ctxt,

props,

buffer

) // we cannot reflect this method so do the default Java-ish behavior

Got it. For posterity the null half of that ?: was moved to KotlinValueInstantiator:31

src/main/kotlin/com/fasterxml/jackson/module/kotlin/ReflectionCache.kt

dinomite · 2022-01-02T21:11:41Z

src/main/kotlin/com/fasterxml/jackson/module/kotlin/KotlinValueInstantiator.kt

@@ -157,23 +120,19 @@ internal class KotlinValueInstantiator(
            numCallableParameters++


Existing code, but I think this would be a bit clearer as numCallableParameters + 1, since we don't need to increment the value in place. That'll also allow numCallableParameters to be declared as a val

I was planning to fix this when I introduce ArgumentBucket in the next PR.
https://github.com/FasterXML/jackson-module-kotlin/pull/439/files#diff-4fffadfbb02911c033bc87266a27253e0410d773ee8067b6f6786c440ac5306fR1

Sounds good.

dinomite · 2022-01-02T21:12:31Z

src/main/kotlin/com/fasterxml/jackson/module/kotlin/ConstructorValueCreator.kt

+    init {
+        callable.isAccessible = true
+    }


Does this override the above since this init block is lower? Is that the correct behavior?

Are you saying that it is difficult to know which comes first, initializing the property or rewriting isAccessible?
If I believe Intellij IDEA's point, it should work correctly, but if I value readability, should I rewrite it as follows?(Rewriting the accessible property was surprisingly expensive, so I've included a change to rewrite it only when necessary.)

override val accessible: Boolean = run { val initialAccessible = callable.isAccessible if (!initialAccessible) callable.isAccessible = true initialAccessible }

Understood, I was just a bit unsure whether that rewrite was intended 👍

…Cache.kt Co-authored-by: Drew Stephens <drew@dinomite.net>

Fixed a bug in #512.

k163377 added 4 commits October 30, 2021 21:41

add Value Creators

1373c75

use ValueCreator temp

bf9f0b1

fix to use cache

a166033

fix

3ffe0a0

k163377 commented Oct 30, 2021

View reviewed changes

dinomite self-requested a review October 31, 2021 10:08

dinomite self-assigned this Oct 31, 2021

k163377 mentioned this pull request Dec 30, 2021

Support for inline classes #199

Closed

dinomite reviewed Jan 2, 2022

View reviewed changes

This was referenced Jan 2, 2022

JsonValue set in value class does not work #530

Closed

Fix #530 #531

Merged

k163377 and others added 3 commits January 3, 2022 07:43

Update src/main/kotlin/com/fasterxml/jackson/module/kotlin/Reflection…

4592418

…Cache.kt Co-authored-by: Drew Stephens <drew@dinomite.net>

Update src/main/kotlin/com/fasterxml/jackson/module/kotlin/Reflection…

986e7a6

…Cache.kt Co-authored-by: Drew Stephens <drew@dinomite.net>

Merge remote-tracking branch 'FasterXML/2.13' into refactor-KVI

0b18f12

dinomite approved these changes Jan 3, 2022

View reviewed changes

dinomite mentioned this pull request Jan 3, 2022

Significant improvement in deserialization speed #439

Open

dinomite merged commit c49144b into FasterXML:2.13 Jan 3, 2022

k163377 deleted the refactor-KVI branch January 3, 2022 15:45

k163377 mentioned this pull request Jan 3, 2022

Fixed a bug in #512. #533

Merged

dinomite added a commit that referenced this pull request Jan 7, 2022

Merge pull request #533 from k163377/github512/fix

99389f3

Fixed a bug in #512.

k163377 mentioned this pull request Jan 10, 2022

Refactor KVI 2 #538

Closed

k163377 mentioned this pull request Feb 17, 2023

Merge creator cache for Constructor and Method #627

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor KVI #512

Refactor KVI #512

k163377 commented Oct 30, 2021

k163377 Oct 30, 2021

dinomite Jan 2, 2022

k163377 Jan 3, 2022 •

edited

dinomite Jan 3, 2022

dinomite Jan 2, 2022

dinomite Jan 2, 2022

k163377 Jan 2, 2022

dinomite Jan 3, 2022

dinomite Jan 2, 2022

k163377 Jan 3, 2022

dinomite Jan 3, 2022

dinomite Jan 2, 2022

k163377 Jan 2, 2022

dinomite Jan 3, 2022

	val callable = when (_withArgsCreator) {
	is AnnotatedConstructor -> cache.kotlinFromJava(_withArgsCreator.annotated as Constructor<Any>)
	is AnnotatedMethod -> cache.kotlinFromJava(_withArgsCreator.annotated as Method)
	else -> throw IllegalStateException("Expected a constructor or method to create a Kotlin object, instead found ${_withArgsCreator.annotated.javaClass.name}")
	} ?: return super.createFromObjectWith(
	ctxt,
	props,
	buffer
	) // we cannot reflect this method so do the default Java-ish behavior

		@@ -157,23 +120,19 @@ internal class KotlinValueInstantiator(
		numCallableParameters++

Refactor KVI #512

Refactor KVI #512

Conversation

k163377 commented Oct 30, 2021

Changes from existing

Changes from #439

Others

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k163377 Jan 3, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k163377 Jan 3, 2022 •

edited