-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Generate faster reflection-free Jackson serializers #41063
Conversation
Thanks for your pull request! The title of your pull request does not follow our editorial rules. Could you have a look?
This message is automatically generated by a bot. |
This is very neat! I had done something similar a few years ago and the problem I had encountered was figuring out the proper conditions for falling back to Jackson's default behavior. |
I also wonder if it makes sense to collaborate with upstream as there may be a way to generalize Afterburner to use this approach |
This is indeed a very nice experiment! Just thinking loud but...what if you let Jackson to create it's own reflection based classes and use such to create your one? |
I'm giving a look at Afterburner (I didn't know it) and it seems that now it is considered somewhat legacy with Blackbird that is currently under development as a more modern alternative. That said, Blackbird is based on the LambdaMetafactory, while I still think that we can do better with our Gizmo bytecode generation. However it is interesting to check if we could at least reuse part of it to avoid reimplementing part of the Jackson logic and annotation semantic on our side as I think that @franz1981 was also suggesting. I will further investigate this. |
Definitely!
Thanks! |
A quick update on this: I was hoping that Unfortunately this is not case. On this regard
In essence everything is done lazily at runtime and there isn't any discovery mechanism to be reused. In my opinion this not only goes against the Quarkus philosophy but it is also quite sub-optimal compared with the solution that I sketched here, and some preliminary benchmark results already confirm this impression. If we want to continue implementing this solution I don't see any alternative other than also duplicating the Jackson logic and annotation semantics on our side and of course I see the potential risks and maintainance burden in this approach. Nevertheless I would like to keep experimenting with this and check how far I can push it, unless you don't see already a showstopper. Please @geoand and @franz1981 let me know your opinion and how you suggest to eventually proceed on this. |
What about what I have suggested in the previous comment? i.e. checking the serializers used by Jackson which contains Field and Method reference and use them to create the custom serializers as you are doing already, just based on Jackson-way to inspect the classes to serialize. This will likely require to modify Jackson to make these cached serializers accessible |
I completely agree.
+1.
When you have a understanding of how things work in Jackson around this, I propose reaching out to Tatu (Jackson's maintainer) and chatting with him to see if there is something that can be done (without requiring a massive rework of Jackson) so we can better integrate at build time. From a Jackson point of view, it could be noted that Quarkus would not be the only beneficiary of such an approach, other tools and frameworks could utilize something similar. |
private String writeMethodName(String typeName) { | ||
return switch (typeName) { | ||
case "java.lang.String" -> "writeStringField"; | ||
case "short", "java.lang.Short", "int", "java.lang.Integer", "long", "java.lang.long", "float", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
java.lang.Long uppercase L
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
d5e2b99
to
1275e7b
Compare
I made some good progresses with this PoC to the point that I believe that this is at least ready to be reviewed now, if not to be considered to be merged. As already discussed in a former comment and also here, I didn't find any reasonable way to reuse, even partially, the java objects introspection made by Jackson, mostly because it all happens at runtime on the actual object instances while we want do all the code generation at build time relying only on what we can infer (via jandex) from the classes to be serialized. For this reason I implemented a mechanism to skip the generation of the The fact that we no longer need reflection to perform json serialization (at least when we don't have to use that fallback mechanism) also implies a good performance improvement. To measure this I added a new rest endpoint to the our benchmark suites that we used for our workshop on profiling that simply performs a json serialization of an instance of a Customer class. In essence the rest endpoint produces a json like the following:
What this pull request does is generating at build time and registering a compile time a serializer for that Customer class like the following:
Running this benchmark against the current Quarkus main branch I obtained the following result
while doing the same against this pull request I got
Be aware that this quite huge speed up is also partially due to some minor improvement that I implemented in the @geoand @franz1981 If possible I'd like to let run this pull request on the Quarkus CI and check if it breaks anything. Also I'd like to further discuss on how to move forward with the adoption of this work. For instance we could (at least temporarily?) consider this feature as an opt-in improvement and allow users to give it a try and enable it on demand trough a Quarkus config property. Any other idea or feedback on this is welcome. |
Very very cool results!
I believe this is a good idea |
This comment has been minimized.
This comment has been minimized.
Do you have a similar measurement for the case where |
very nice work @mariofusco cannot wait to do a full review for this 👍 |
return true; | ||
} | ||
if (fieldType instanceof ParameterizedType pType) { | ||
if (pType.arguments().size() == 1 && (typeName.equals("java.util.List") || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding common concrete types can be helpful here (but not key, I think - I won't expect using ArrayList to be the common case TBH)
I commented out this annotation so it uses the On main I got:
while with this pull request:
so as I expected the difference is much smaller (because now I'm not taking advantage of the other optimization that I mentioned before), but still quite relevant. I will add the Quarkus property that I suggested to allow to opt-in this feature. Any suggestion on the naming? Do we have some other similar opt-in features? |
Our naming has been inconsistent to say the least 😂. I would do something like |
I only learn of this now :( I've been doing the same in Renarde, I generate Jackson serialisers and deserialisers at build-time, but for JPA entities, which have a slightly more complex use-case to POJOS in that IDs and lazy-loading has to be respected. Because JPA entities are cyclic graphs, and Jackson doesn't deal with graphs. So, it's a custom serialiser that only serialises IDs for relations (instead of serialising recursively). The code is all there: https://github.com/quarkiverse/quarkus-renarde/blob/main/transporter/deployment/src/main/java/io/quarkiverse/renarde/transporter/deployment/RenardeTransporterProcessor.java We should definitely discuss joining code. It's great to generate recursive serialisers for POJOs, but it can't work for JPA objects, and I feel it's just a toggle between the two modes. JPA entities need special id-based non-recursive serialisers, while every other POJO can use the recursive one, but generating them is mostly the same code. WDYT? |
Since we're talking about profiling JSON serialisers, did you also take a look and measure with https://github.com/quarkusio/qson ? |
@FroMage I tried to compare your implementation with mine and my impression is that, even if at a first sight they try to achieve something very similar, they also have some quite different premise and outcome, and at this point I'm afraid that they differ in many more relevant parts than the one they have in common. As you wrote your effort is very specific to JPA entities while mine is much closer to what Jackson serialization performs out-of-the-box with the only difference that I try to do the same of what Jackson does, but without using any reflection. This difference is for instance reflected in how we process the single fields that to me are basically the I started to sketch something similar to what @franz1981 proposed:
but I immediately realized that I was simply implementing something as generic as a visitor pattern for a jandex Maybe I'm missing something and I'm very open to discuss this, but at the moment I don't see any lower level common ground between our 2 implementations. Any feedback? |
exactly @mariofusco that was indeed what I was thinking - maybe @Ladicek can tell if he think this could be useful elsewhere? |
I'm honestly not sure if a visitor over Jandex objects makes sense. Everything is in memory already, so you can just access the objects directly. I'm probably missing a lot of context, so if you have anything specific you could show, I'd be happy to take a look at it. |
Well, you're right that they don't have much in common as to how they behave. Can't argue with that. And also, we're telling people to not serialise entities to JSON, and use DTOs instead. So my use-case is pretty specific and not intended for REST APIs, which your use-case is probably targetting. It's alright if this doesn't end up merged. |
...s/resteasy/reactive/jackson/deployment/test/SimpleJsonWithReflectionFreeSerializersTest.java
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
cfb60c7
to
3b2d10b
Compare
...java/io/quarkus/resteasy/reactive/jackson/deployment/processor/JacksonSerializerFactory.java
Show resolved
Hide resolved
@@ -52,6 +52,7 @@ public Response handleParseException(WebApplicationException e) { | |||
|
|||
@GET | |||
@Path("/person") | |||
@Produces(MediaType.APPLICATION_JSON) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why were these @Produces
added? They are not necessary in Quarkus and if the test now fails without them, that is a genuine issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There isn't any test failure without that annotation, but as you can see here, I'm adding the type returned among the ones for which I generate the serializers only if that annotation is present. Should I ignore it and generate the serializer regardless?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That approach is going to be a problem because in Quarkus REST we use JSON by default. See JsonDefaultProducersHandler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, I will generate the serializer regardless if the @Produces
is present or not and also remove it from tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏼
fix object mapper generation for all primitive types implement nested types in generated object mapper implement SecureField annotation support avoid generating mapper for pojo with unknown jackson annotations implement collections serialization fix all tests in rest-jackson module wip wip refactor and simplification performance tuning make reflection-free serializers generation opt-in add @produces annotation to SimpleJsonResource rest endpoints where appropriate generate serializers for returned types from rest methods regardless if the @produces annotation is present or not wip add javadoc
09a92a0
to
65712ce
Compare
I believe that this is working reasonably well now and, also considering that it is disabled by default and it has to be explicitly enabled through the property Since I learned a lot about jandex, gizmo and more in general on how to write a Quarkus extension, while developing this, I'm also thinking that it could be used as a practical example to write a blog post on these topics. /cc @holly-cummins @maxandersen |
Very nice! I'll make one final pass tomorrow and then if all is well I'll merge it |
A blog would be amazing, @mariofusco. |
This comment has been minimized.
This comment has been minimized.
awesome stuff - and congrats on what I think is the longest ever quarkus feature flag name ;) blog definitely +1000! |
Status for workflow
|
Achievement unlocked!!! |
...ava/io/quarkus/resteasy/reactive/jackson/deployment/processor/JacksonOptimizationConfig.java
Show resolved
Hide resolved
} | ||
|
||
public Optional<String> create(ClassInfo classInfo) { | ||
String beanClassName = classInfo.name().toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny nitpick: this isn't really a bean
Thanks a ton for this, this is awesome! I see two things that need to be done as a follow up:
|
Done with #42289 |
This is a PoC demonstrating a possible improvement in Jackson serialization performances that out-of-the-box is heavily based on reflection. The Idea is replacing this behavior with automatically generated serializers and configure them on the Jackson's
ObjectMapper
.To demonstrate how this works I added the following trivial rest endpoint to the benchmark suites that @franz1981 and I used for our workshop on profiling.
Running this benchmark on my machine I got the following result
and in particular the execution related with the serialization of the
Customer
object looks like the following, where the Jackson'sBeanPropertyWriter
s retrieve the value of each and every field to be serialized via reflection.With the change that I'm proposing I automatically find via Jandex the list of the classes requiring a Jackson serialization and for each of them I generate with Gizmo a custom serializer like this:
and configure the
ObjectMapper
to use it. Rerunning the former benchmark with this improvement I got this result:with an increase greater than 10% in the number of requests served per second even for such a trivial use case. In this case the serialization of the Customer class is entirely executed by the generated serializer with the advantage of avoiding any use of reflection, thus requiring less than the half of the number of samples than before.
I'm planning to keep working on this PoC in the next days, mostly to improve the generated serializer, making it cover more complex use cases and be consistent with Jackson's semantic, but any early feedback on this is welcome.
/cc @franz1981 @geoand