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

Improve kotlinx.reflect.lite and rewrite it to use kotlinx.metadata.jvm under the hood #12

Closed
wants to merge 21 commits into from

Conversation

udalov
Copy link
Member

@udalov udalov commented Dec 13, 2017

This pull request prepares the kotlinx.reflect.lite for a new release 1.1.0, the main feature of which is the ability to introspect not only the java.lang.Class objects, but also .class files [1]. Some new API has been added (requested in KT-11386). All of the existing API has been documented (no API docs yet though, unfortunately), and an example has been added to the ReadMe.

[1]: UPD: As noted in KT-11386, making kotlinx.reflect.lite understand .class files without the context of Java reflection is no longer necessary because it can be achieved via kotlinx.metadata.jvm. This pull request has since been repurposed to only contain reimplementation of kotlinx.reflect.lite based on kotlinx.metadata.jvm, and requested improvements in the API.

@udalov
Copy link
Member Author

udalov commented Dec 13, 2017

Note that it's not necessary to review the code in org.jetbrains.kotlin, because it's copied automatically from Kotlin.

@sdeleuze
Copy link

sdeleuze commented Jul 3, 2018

Hey any plan to merge this PR? FYI I would be interested into making Spring Framework usable with it instead of the huge full kotlin-reflect one.

@udalov
Copy link
Member Author

udalov commented Jul 3, 2018

Yes, I'll get back to this PR soon. The main reason it was dormant is we were working on the kotlinx-metadata-jvm library with somewhat intersecting functionality. Now that the first build of that library is released, we can factor out most of the implementation of kotlinx-reflect-lite and keep it as simple as possible, with the only dependency on kotlinx-metadata-jvm.

@sdeleuze
Copy link

sdeleuze commented Jul 3, 2018

Awesome thanks. That will be a great improvement for our users to be able to leverage it!

@udalov
Copy link
Member Author

udalov commented Jul 4, 2018

Note that as per this comment, I'll remove parts of this pull request that enable to introspect .class files. This feature was requested at the time when there was no option such as kotlinx-metadata-jvm, and now adding it to kotlinx-reflect-lite would be harmful because it'd require to bundle ASM which increases the final library size.

@sdeleuze
Copy link

I am currently updating Spring in order to be more flexible about reflection, partly because we have found that using kotlin-reflect is slowing down Spring app startup and create GC pressure (see this flamegraph).

We are going to support running Spring apps without kotlin-reflect with less functionality (and currently no Jackson Kotlin support), and as said previously I would like to support kotlinx.reflect.lite as well. Here is what we use: mainly being able to identify nullable/ not nullable, primary constructors and parameter names.

Do you think kotlinx.reflect.lite will cover our use cases?

@udalov
Copy link
Member Author

udalov commented Jul 30, 2018

@sdeleuze Thanks for the flamegraph showing the kotlin-reflect execution time -- it didn't even occur to me to analyze performance this way! I'll try to investigate the larger nodes on that flamegraph and see if we can do something about it.

Do you think kotlinx.reflect.lite will cover our use cases?

Yes, looks like you will be able to use it for these purposes. I'm not sure if the performance would be much better though (although it probably should); in any case, I'd be very interested to take a look at the new flamegraph if you're planning to render it after switching to kotlinx-reflect-lite :)

The one thing that wouldn't be completely straightforward with the API proposed in this pull request is the mapping of Kotlin declarations to Java (such as ReflectJvmMapping.getKotlinConstructor() usage here), where you'd need to do the reverse and invoke ClassMetadata.getConstructor on each constructor of the class and then check if it isPrimary, which is more work than absolutely needed. Maybe we can leverage this shortcoming in the future.

@sdeleuze
Copy link

sdeleuze commented Sep 7, 2018

Hey, back from vacation ;-)

I guess you are very busy, but have you made any progress on that topic? On my side I am ready to test whatever you have via https://github.com/spring-projects/spring-fu.

For your information, I have done following benchmarks that show the cost of current kotlin-reflect (via Kotlin Jackson module) init in figures:

Reactive webapp with Jackson without kotlin-reflect:

  • started application in 0.516 seconds (JVM running for 0.779)
  • used PSYoungGen 41826K
  • used ParOldGen 16K
  • used Metaspace 19636K

Reactive webapp with Jackson with kotlin-reflect:

  • started application in 0.808 seconds (JVM running for 1.13)
  • used PSYoungGen 61169K
  • used ParOldGen 7029K
  • used Metaspace 23357K

This confirms what we saw on the flamegraph. Getting Jackson Kotlin module and Spring compatible with a lite Kotlin reflection API would be a huge improvement for our users, especially on the Cloud where these figures will be even worst.

cc @apatrida

@udalov
Copy link
Member Author

udalov commented Sep 10, 2018

@sdeleuze Thanks for the reminder! I've added a change that reimplements kotlinx.reflect.lite on top of kotlinx.metadata.jvm into this pull request. Could you please try building the artifact from the dev branch of this project and using it in your scenario?

@udalov udalov changed the title Improve kotlinx.reflect.lite and generalize it to work also with class files, not only Class objects Improve kotlinx.reflect.lite and rewrite it to use kotlinx.metadata.jvm under the hood Sep 10, 2018
@sdeleuze
Copy link

Awesome!

I have tried to create a branch of Spring based on kotlinx.reflect.lite, I have the following questions/remarks:

  • KParameter::isOptional seems missing, possible to add it?
  • Is it possible to get only value and extension receiver parameters? Or it is maybe the default? (I need to have Java parameter indexes = Kotlin ones so I filter them by Kind currently)
  • What should I use instead of KCallable::callBy? (to only provide specified parameters and rely on default ones when not provided, maybe with Java ctor with null values?)
  • What about providing a small optional kotlin-reflect API adapter with ReflectJvmMapping + KFunction + etc. to allow seamless migration of existing libraries?

@udalov
Copy link
Member Author

udalov commented Sep 10, 2018

KParameter::isOptional seems missing, possible to add it?

There's ParameterMetadata.hasDefaultValue with a slightly different semantics (doesn't include inheritance).

Is it possible to get only value and extension receiver parameters? Or it is maybe the default? (I need to have Java parameter indexes = Kotlin ones so I filter them by Kind currently)

The way this is represented in kotlinx-reflect-lite differs from the way it's done in kotlin-reflect. CallableMetadata.parameters only returns value parameters (maybe it should have the name valueParameters...); the extension receiver can be accessed via CallableMetadata.extensionReceiverType; the instance receiver is not really available via the provided API, but its presence can be predicted by whether the callable is declared in a class and not in a file.

What should I use instead of KCallable::callBy? (to only provide specified parameters and rely on default ones when not provided, maybe with Java ctor with null values?)

There's no API for that in kotlinx-reflect-lite unfortunately yet. Meanwhile you could use Java reflection for this. The ABI of methods with default arguments is very simple: the method is named foo$default for a function called foo, it takes function parameters in the same order, then an integer bitmask (or multiple bitmasks, if there are > 32 parameters) indicating what parameter values should be used (1 means use the passed argument value, 0 means use the default value), and a null in the end. Same for constructors, except the $default suffix. Please refer to the implementation of KCallableImpl.callBy for more details

What about providing a small optional kotlin-reflect API adapter with ReflectJvmMapping + KFunction + etc. to allow seamless migration of existing libraries?

It was actually specifically not a goal of this library to be compatible with kotlin-reflect, precisely because the decisions involved in shaping the design of kotlin-reflect into what it is now, have led to a more complex implementation, the one thing that kotlinx-reflect-lite avoids to prevent growing in size. There are many convenient aspects of kotlin-reflect which look innocent but are costly in implementation size, and doing the same in kotlinx-reflect-lite would probably hurt the final application size.

That said, I of course don't have any numbers as to how would the library size increase, had this been implemented as a fully compatible kotlin-reflect replacement from the start. It would be very interesting to reimplement kotlin-reflect, following precisely the API in package kotlin.reflect, but using the lightweight metadata library instead of the compiler internals (kotlinx-reflect-mid? :D). I'm not sure I have the capacity to do that in the near future, though.

@sdeleuze
Copy link

Thanks for your feedback, I will try to create a Spring Framework branch dedicated branch asap to check our tests are green.

@ZacSweers
Copy link

Is this PR still something you plan to merge?

@udalov
Copy link
Member Author

udalov commented Sep 13, 2019

Yes, we do plan to merge it. Thanks for the reminder. This hasn't been a priority for us mostly because of no real-world usages. I'll take a look at it once again soon.

@sdeleuze
Copy link

Still very much interested by this topic, just other priorities right now but that topic could be back for Spring Framework 5.3 so a merge of that PR would be nice indeed (sorry for not having found the time to test it).

@sdeleuze
Copy link

Ok so more efficient Kotlin reflection becomes a priority for Spring Framework 6, but we now have slightly different requirements because we want to make it work on GraalVM native as well, where .class resources are not available.

I have the feeling that what is currently commited in kotlinx.reflect.lite uses Java reflection and that this PR would modify this to leverage kotlinx-metadata-jvm which is using .class resources.

If that's the case, I would be against merging this PR and instead collaborate on maturing what is currently on master branch.

@udalov
Copy link
Member Author

udalov commented Oct 30, 2021

@sdeleuze kotlinx-metadata-jvm does not use .class resources. Its input is this structure which is basically a copy of the kotlin.Metadata annotation, which is generated by the compiler on every Kotlin class file.

The way the library is supposed to be used to read the metadata is:

  1. Load the data from the @kotlin.Metadata annotation on the class, in the library-/tool-specific way. For example, a library that works via reflection can get the annotation from the java.lang.Class instance. A build-time bytecode postprocessing tool can get the annotation by loading the class bytes via ASM and looking for the annotation with descriptor Lkotlin/Metadata;.
  2. Create an instance of KotlinClassHeader with exactly the same data as loaded from @kotlin.Metadata. (Hopefully we'll make this step obsolete before kotlinx-metadata-jvm goes stable, but right now there are problems.)
  3. Use KotlinClassMetadata.read on that instance and inspect the result.

The key point is that kotlinx-metadata-jvm does not care about the way you've loaded metadata from the @kotlin.Metadata annotation on the first step. It only provides a means to interpret that data.

@sdeleuze
Copy link

sdeleuze commented Nov 4, 2021

Thanks for the clarification, and nice design. If that's mean we can continue to load annotation via reflection in kotlinx.reflect.lite then that's ok for our need.

@qwwdfsad
Copy link
Contributor

Superseded by #19

@qwwdfsad qwwdfsad closed this Aug 30, 2022
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.

5 participants