Reflection overhead

[mihic]

After migrating one of our rest clients to kotlin data classes (with jackson and KotlinModule), we observe large reflection overheads. This almost doubles our allocation rate compared to the previous implementation.

Reflection cache is on with with 500.000 limit.

Similar issue:
#136

[cowtowncoder]

This is not much information to go about: is this on-going (overhead during operation), or just during initial warmup period? Are you properly reusing ObjectMapper?

Startup overhead is to be expected as initial construction of (de)serializers requires Reflection access to find Bean/value class structure, as well as any annotations. But assuming mappers are not created on per-call basis (which is a really bad idea wrt performance), there should not be reflection access after initial use.

So you would need to show actual code, or perhaps performance test reproduction.

[Tapac]

@cowtowncoder , I had faced the same issue and after some investigation I could conclude that the problem in that line :

val isGenericTypeVar = paramDef.type.javaType is TypeVariable<*>

Call to .javaType is made even if paramVal == null && !paramDef.type.isMarkedNullable (see below in code) is false. The call for javaType is very heavy and moreother it uses soft-reference as a cache which can be invalidated under a high-load.

I would suggest to replace that part of code with something like below to prevent unneeded have calls:

            val isGenericTypeVar = { paramDef.type.javaType is TypeVariable<*> }
            val isMissingAndRequired = paramVal == null && isMissing && jsonProp.isRequired
            if (isMissingAndRequired ||
                (paramVal == null && !paramDef.type.isMarkedNullable && !isGenericTypeVar())) {

I can send a PR if it's ok with that approach.

[cowtowncoder]

@Tapac I'll have to defer to @dinomite and @viartemev here wrt proposed change. But what is weird is that modules rarely should have to rely on java.lang.reflect.Type information: those should almost always be resolved to JavaType and specifically NOT try to play with TypeVariable (it is rather unreliably way to find info on actual type parameterization).

Still.... what I do not understand is this: looking for ValueInstantiator is only done once when locating deserializer to use: deserializers should be fully cached, and lookup should not be repeated. So how is this found on a critical path?
This suggests possible problem somewhere in application code, such as not reusing ObjectMapper or such -- fixing of which would solve this issue and probably improve overall performance a lot.

[Tapac]

What kind of a cache do you mean?
As I can see from a code on every call of createFromObjectWith there is a loop over instance constructor valueParameters. And on each iteration there is a call for paramDef.type.javaType.

[cowtowncoder]

@Tapac Ah! ValueInstantiator itself should be cached, and it seems there should not be need to do any reflection from createFromObjectWith() -- that should all be resolved before getting there, during construction.
But yes, if reflection calls are made from that method, caching would not help. That seems like sub-optimal implementation -- not sure why this is done, I am sure there is a reason.

[dinomite]

Hmm, it looks like that paramDef.type.javaType call is used to determine whether we can enforce Kotlin nullability constraints—if the types have been erased, then we don't know whether the type is nullable or non-nullable, so we can't do the check that throws MissingKotlinParameterException when the value is null.

[dinomite]

I don't think we can avoid calling javaType on each param, but we could of course cache it. I think I can just add a new LRUMap<KParameter, Type> in the ReflectionCache.

The hard part in my mind is a test to verify that change is actually productive. Oh, and I had missed that @Tapac had suggested nearly this—would you create a PR with your implementation idea?

[cowtowncoder]

@dinomite I think there should be a simpler way -- why aren't these aspect resolved right in constructor of Kotlin ValueInstantiator, instead of createFromObjectWith()? Construction happens just once per type, and then results are effectively cached: there can be no change to type definitions.

So, as much of introspection code as possible should be moved in constructor (or maybe better yet, factory method, which then just passes resulting configuration settings), and createFromObjectWith() just uses pre-computed configuration.

I wish I could suggest specific changes, but unfortunately I can't quite follow Kotlinisms. But I am pretty sure that all relevant information must be statically (as in, during construction of ValueInstantiator, and not when instantiator is used to construct instance based on input) available.
And current way of dynamically determining all of those aspects looks quite inefficient so even beyond caching rewrite should speed up things in measurable way.

[dinomite]

Good explanation, I'll see how I can do this from VI's constructor.

[k163377]

Fixed the issue pointed out by @Tapac in #687.
This issue will be closed, but if a problem is identified, please submit a new issue.