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

API to get serializer for a class without creating kotlin reflection class #1819

Closed
develar opened this issue Jan 9, 2022 · 12 comments
Closed

Comments

@develar
Copy link

develar commented Jan 9, 2022

What is your use-case and why do you need this feature?

Current experimental API ::kotlin.get().serializer() implies creating extra kotlin reflection class. That's not acceptable from a performance point of view in all cases.

Describe the solution you'd like

Could you please provide API to use just a java Class?

@develar develar added the feature label Jan 9, 2022
@develar
Copy link
Author

develar commented Jan 9, 2022

official API:
Screenshot 2022-01-09 at 09 03 41

using java class only:

   // findStaticGetter cannot be used because type of Companion is unknown
    val companion = lookup.unreflectGetter(aClass.getDeclaredField("Companion")).invoke()
    @Suppress("UNCHECKED_CAST")
    serializer = companion.javaClass.getDeclaredMethod("serializer").invoke(companion) as KSerializer<Any>

Screenshot 2022-01-09 at 09 03 51

SergeyZh pushed a commit to JetBrains/intellij-community that referenced this issue Jan 9, 2022
…efection wrapper creation

Kotlin/kotlinx.serialization#1819

GitOrigin-RevId: 9a1bce7a70b6c68c9c6267baa53ac0509b9ff294
@qwwdfsad
Copy link
Member

Current experimental API ::kotlin.get().serializer() implies creating extra kotlin reflection class.

Could you please share a small example of the overhead for us to have something we can compare?

It seems like we indeed can provide a delicate API that operates with a regular Class and even speedup Type-based API

@develar
Copy link
Author

develar commented Jan 10, 2022

For years already in IJ Platform, we do not create kotlin reflection class wrappers if we can avoid it. Profiler snapshot provided above. We have hundreds of persistence state components. That's also not only about performance, but about avoiding extra caches and so on (dynamic plugin reloading, for example).

Say, because it is visible in a profiler snapshot, then another core team member will ask me again and again about that on some performance and/or memory issues investigation.

Fewer issues, easier for me to promote kotlin.

@qwwdfsad
Copy link
Member

I'm not a developer of the IJ platform and not aware of your internal guidelines and constraints, thus I've been asking for additional details.

Profiler snapshot provided above.

There may be a lot of reasons why this call appeared in the snapshot. For example, it's the very first call to Kotlin reflection and thus it's loading all the classes.
The very first call to LambdaMetafactory is also slow and may appear in the snapshot, but it's not the reason to avoid Java lambdas, is it?

about avoiding extra caches and so on (dynamic plugin reloading, for example).

Are you talking about the cache in Kotlin reflection itself (one in kotlin.jvm.internal.Reflection.getOrCreateKotlinClass) or about your own caches?

Are you interested in a "cold" performance (e.g. one-shot invocation) or a "hot" one (when JIT kicks in)?
Do you have any performance criteria besides "it's visible in the profiler"? E.g. if the overhead of KClass instantiation will take ~10% of the serializer() call -- will it still be an issue?
Are Kotlin reflection classes loaded in IJ platform at all?

@develar
Copy link
Author

develar commented Jan 10, 2022

but it's not the reason to avoid Java lambdas, is it?

It may be a reason. We are working on speeding up the start-up. Every used API is checked and profiled. If we can avoid using something and it is worth it, we do avoid it.

Are you talking about the cache in Kotlin reflection itself

Yes, I mean potential not clear side-effects of used 3rd-party API.

Are you interested in a "cold" performance (e.g. one-shot invocation) or a "hot" one (when JIT kicks in)?

Cold :( For example, running tests, application start-up.

Do you have any performance criteria besides "it's visible in the profiler"? E.g. if the overhead of KClass instantiation will take ~10% of the serializer() call -- will it still be an issue?

We have very different people in a core team ;) In short — often easier to avoid questionable code than to explain and check again and again that this piece of code is innocent and doesn't bring any issues. The first question is always — can we avoid it at all? If we cannot, then the second question — is it acceptable or not? Here no need to think about "is 10% ok or not", because it is clear that kotlin reflection wrapper is not needed here at all.

Are Kotlin reflection classes loaded in IJ platform at all?

Only by mistake or if we cannot avoid it. For example, in some cases we have to use a special shim in Java to avoid it (https://github.com/JetBrains/intellij-community/blob/master/platform/object-serializer/src/PropertyAnnotationUtil.java)

qwwdfsad added a commit that referenced this issue Jan 10, 2022
…uctSerializerForGivenTypeArgs

Prerequisite for #1819
@qwwdfsad
Copy link
Member

I've investigated potential solutions for the problem.

In general, it seems to be working as intended with the only exception -- if the target class is marked with @Polymorphic (or, in general, is interface), the implementation is obliged to return PolymorphicSerilizer(javaClass.kotlin). Is it going to work for you?

Also, I've studied implementation from JetBrains/intellij-community@51be4e7 -- it seems to be incomplete. E.g. it won't work for enums, primitive types, classes with named companions*, classes with generic parameters and arrays.

  • -- e.g.:
@Serializable
class Box(...) {
    companion object Default {

    }
}

@develar
Copy link
Author

develar commented Jan 10, 2022

Is it going to work for you?

Not required for our use case. Polymorphic cannot be on a root class, only on a nested (but we don't check nested — that's up to root class serializer). Under "nested" I mean a class of some root class fields.

it seems to be incomplete

For now, we do not use kotlinx serialization for these types. I do understand, that you probably don't want to open API, that will support only regular classes (it is currently internal in your implementation) — to reduce API surface and simplify it. I don't bother about performance loss on such checks, I guess it is negligible.

@qwwdfsad
Copy link
Member

qwwdfsad commented Jan 10, 2022

Let me rephrase it directly: consider we have an API Class<?>.serializer() which mirrors KClass<?>.serializer() -- supports all the types the latter does, instantiates KClass for polymorphic serializers and does all the checks required for API to function -- whether the class is enum, interface and so on, will such API be useful for you? Are you still going to adopt it?

@develar
Copy link
Author

develar commented Jan 10, 2022

Yes, such an API will be useful for me and it will be used as is (and serializerOrNull in addition). Currently, it is under aClass.isAnnotationPresent(Serializable::class.java) check anyway, so, we do call it only for serializable classes.

qwwdfsad added a commit that referenced this issue Jan 12, 2022
…lass.serializerOrNull(), but attempts not to instantiate KClass when possible

Fixes #1819
@sandwwraith
Copy link
Member

sandwwraith commented Jan 17, 2022

I'm a bit late here, but what's wrong with the serializer(java.lang.Type) function?

public fun serializer(type: Type): KSerializer<Any> = EmptySerializersModule.serializer(type)

j.l.Class implements Type so everything should work fine. Or do you want to avoid instanceof check completely? Or is it a generic argument you want to preserve?

@qwwdfsad
Copy link
Member

serializer(type) is fine, but now it uses quite a lot of Klass machinery internally, the PR has the explanation

@qwwdfsad
Copy link
Member

In the new release,it should be enough to use Type-based functions, they avoid instantiating KClass as much as possible

pdvrieze pushed a commit to pdvrieze/kotlinx.serialization that referenced this issue Apr 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants