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 kotlin-reflect and replace it with kotlinx-metadata-jvm #450

Open
k163377 opened this issue May 23, 2021 · 19 comments
Open

Remove kotlin-reflect and replace it with kotlinx-metadata-jvm #450

k163377 opened this issue May 23, 2021 · 19 comments
Labels
3.x Issue affecting/planned for Jackson 3.x enhancement

Comments

@k163377
Copy link
Contributor

k163377 commented May 23, 2021

Use case

It makes it easier to use jackson-module-kotlin in resource-limited environments.

Describe the solution you'd like

Remove kotlin-reflect and replace it with kotlinx-metadata-jvm.

Additional context

The reason for creating this issue is that, as mentioned in this spring-framework issue, kotlin-reflect consumes a lot of resources, and there is a topic to replace it with kotlinx-metadata-jvm.

I've started prototyping it, and I'd be happy to discuss how to implement it in this issue if it seems to work.

Also, if you are interested in this change, please send me a 👍 (I would personally like to know if there is any demand for this change).

@dinomite
Copy link
Member

Interesting. This would mean that all reflection would be gone and Jackson's work would happen purely at compile time? that doesn't sound like it would work with databind, would this alter just the Kotlin module?

Mainly, I want to know more, this is new to me.

@k163377
Copy link
Contributor Author

k163377 commented May 25, 2021

The kotlinx-metadata-jvm only provides the ability to read information from Metadata annotations.
The rest of the processing will be done via Java Reflection.

This means that only the Kotlin module needs to be modified.
Also, not all decisions are made at compile time, as in code generation.

@dinomite
Copy link
Member

Got it, so this would reduce the amount of reflection required when dealing with Kotlin objects?

@k163377
Copy link
Contributor Author

k163377 commented May 26, 2021

this would reduce the amount of reflection required when dealing with Kotlin objects?

I don't have a clear comparison, but it probably is.

I think the explanation of moshi-metadata-reflect is easy to understand.
https://github.com/ZacSweers/MoshiX/tree/main/moshi-metadata-reflect#motivation

@cowtowncoder
Copy link
Member

I don't know this well enough to give much input but explanation for moshi-metadata does sound compelling.
Main question would just be how likely this would be to cause issues; would there be enough time to prove safety of the new approach for, say, 2.13 (definitely should not be introduced in a patch release).

I assume that trying to allow either one dynamically would not make much sense (since kotlin-reflect would still be added as a dependency).
... but holy mackerel, is that jar REALLY 2.8 MEGS in size?!?! (see https://mvnrepository.com/artifact/org.jetbrains.kotlin/kotlin-reflect/1.5.10). If so, I can definitely see why it'd be good to be rid of.

An alternative for solely reducing jar size could be to use shading to include parts needed here. But if wholesale replacement is possible, sure, why not?

@k163377
Copy link
Contributor Author

k163377 commented May 27, 2021

Main question would just be how likely this would be to cause issues; would there be enough time to prove safety of the new approach for, say, 2.13

I don't think it's possible to implement this in 2.13.
First, the changes to complete this are throughout the module, so it will probably take a while to test it.
Also, if call functions with default arguments without kotlin-reflect, that need to include #439 as a prerequisite.

is that jar REALLY 2.8 MEGS in size?!?!

I'm also surprised.
Why is it so heavy?
And it's heavier than version 1.3.x...

@dinomite
Copy link
Member

I'll take another look at #439 this week…

@sdeleuze
Copy link

I am very interested if Jackson could stop depending on kotlin-reflect, that would allow me to do the same on Spring side.

@cowtowncoder
Copy link
Member

Note that although I hope to speed up progress towards 2.13, that is not to say there isn't time to do bigger things just due to that timeline. I try to accommodate release timings to get in important useful fixes, if a bit of extra time is needed.
But at least it would be great to get this in 2.x series (as opposed to only Jackson 3.0).

@k163377
Copy link
Contributor Author

k163377 commented Jun 6, 2021

@dinomite @cowtowncoder
As a preliminary step to working on this change, I am planning to do some refactoring regarding readability and performance.

I apologize for the many reviews I have already asked for.
I always appreciate your reviews.

@dinomite dinomite added the 3.x Issue affecting/planned for Jackson 3.x label Jun 7, 2021
@k163377
Copy link
Contributor Author

k163377 commented Jan 14, 2023

The project that solved this problem is now available.
If you are interested, please give it a try.
https://github.com/ProjectMapK/jackson-module-kogera

@sdeleuze
Copy link

Very interested on this since we would like to migrate Spring as well. I will give it a try on Spring side and provide a feedback.

@cowtowncoder
Copy link
Member

cowtowncoder commented Jan 16, 2023

As per my note on Spring issue: I would be interested in this. Perhaps this could proceed for Jackson 2.15?
Do you think this could be done with a simple (?) PR @k163377 here?
What would be the trade-offs wrt backwards compatibility?

/cc @dinomite @Spikhalskiy @viartemev

@k163377
Copy link
Contributor Author

k163377 commented Jan 17, 2023

@cowtowncoder

Do you think this could be done with a simple (?) PR

I think it would be a very complicated PR.

First of all, not only are there many changes to be made, but there are also some things (especially around error messages) that have not been done because they are too time-consuming to implement.
I intend to prioritize adding/fixing features related to value class in the development of kogera, and will work on the these migrations later.

Secondly, I have not confirmed that there are no unintended destructive changes in kogera, and I don't think we have an agreement on what would be sufficient to confirm.
If we migrate at 2.15.x, I think there is a risk of unintentionally breaking the behavior.

Finally, as I mentioned in my comment here, if we are only considering Spring, the migration timing should be after the Spring work.
As for individual migration, I think there is no problem if you use kogera.

In conclusion, I personally feel that it is better to wait until 2.16.x or later for the migration process.

@cowtowncoder
Copy link
Member

cowtowncoder commented Jan 17, 2023

@k163377 that sounds reasonable.

Looking at Kogera I like the idea of trying out how it works, and perhaps (but only perhaps) its ideas could eventually replace current jackson-module-kotlin.

I have only one concern. README states:

Since the package/module name of jackson-module-kogera is the same as that of jackson-module-kotlin,...

I think it is a bad idea to do either of these, and I would suggest you change this; especially module name.
It may make things slightly easier for migration but can cause major issues with transitive dependencies: for that reason I suggest you use unique Java package name and module name. This will save users with complicated dependency set from lots of unnecessary pain.
Even if they need to change (at least) one import statement and use different module dependency.

@sdeleuze
Copy link

2.16.x looks like a good target and let us the time to experiment on Spring side.

@k163377
Copy link
Contributor Author

k163377 commented Jan 21, 2023

I think it is a bad idea to do either of these, and I would suggest you change this; especially module name.

Sorry, I forgot to reply.
I will fix this when I move to the beta version.

@cowtowncoder
Copy link
Member

Great @k163377! This sounds like a good new module and lets you experiment much faster than with existing Jackson Kotlin module. Plus change behavior in incompatible ways where it makes sense too (fix problems)

@k163377
Copy link
Contributor Author

k163377 commented Jun 11, 2023

I apologize for the delay in responding due to my busy schedule.
I have released a version with corrected package names.
https://github.com/ProjectMapK/jackson-module-kogera/releases/tag/2.15.2-beta0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x Issue affecting/planned for Jackson 3.x enhancement
Projects
None yet
Development

No branches or pull requests

4 participants