-
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
Bump smallrye-open-api from 3.10.0 to 3.12.0, use new builder API #41037
Conversation
b5ddc55
to
9dc6aa4
Compare
This comment has been minimized.
This comment has been minimized.
9dc6aa4
to
a283c58
Compare
This comment has been minimized.
This comment has been minimized.
@phillip-kruger , please take a look. Thank you! |
@MikeEdgar I'll have a look on Friday, on the road at the moment. Can you check the ci failure? |
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.
This new version comes with a significant regression in the native executable size.
Not sure what is causing it but I'm not sure we want to go this way given how ubiquitous SmallRye OpenAPI is.
We need to figure out what's causing it and see if we can avoid it.
I'll check into it. |
@gsmet is there a simple way to determine the delta in the native image? I suspect the new builder API may be causing additional SmallRye classes to be incorporated. |
a283c58
to
00805b9
Compare
@gsmet, @phillip-kruger , I've rebased on |
This comment has been minimized.
This comment has been minimized.
00805b9
to
becc1fd
Compare
This comment has been minimized.
This comment has been minimized.
@gsmet , I'm not sure how to proceed on this one. The failing native image test is no longer a factor. Do you have concerns that will still be a problem? |
becc1fd
to
ab79562
Compare
This comment has been minimized.
This comment has been minimized.
ab79562
to
5de7cbd
Compare
This comment has been minimized.
This comment has been minimized.
5de7cbd
to
539b72f
Compare
This comment has been minimized.
This comment has been minimized.
🎊 PR Preview e1577e6 has been successfully built and deployed to https://quarkus-pr-main-41037-preview.surge.sh/version/main/guides/
|
This comment has been minimized.
This comment has been minimized.
539b72f
to
d3798c0
Compare
This comment has been minimized.
This comment has been minimized.
Status for workflow
|
@MikeEdgar sorry this one slipped. I think we should probably get it in. If the native image metrics are too bad, I suppose it will raise a warning on @zakkak 's side and we can dig deeper from there. |
Signed-off-by: Michael Edgar <[email protected]>
d3798c0
to
e167294
Compare
This PR indeed resulted in significant binary size increase (> 3%) in some of the intergration tests:
|
@zakkak does your infra provide a list of the classes that were added? |
No, I can only get the numbers not specific classes from the collected data. We would need to set |
More detailed tables:amazon-lambda-rest-funqy - 2024-09-12
amazon-lambda-rest-reactive-routes - 2024-09-12
amazon-lambda-rest-resteasy-reactive - 2024-09-12
amazon-lambda-rest-servlet - 2024-09-12
elytron-undertow - 2024-09-12
resteasy-jackson - 2024-09-12
The results indicate that the update brings new classes into the classpath most of which also appear to be reachable. |
My guess is that the annotation scanning classes may be pulled in because of the new builder API, even though they do not execute at runtime. Would using a recorder possibly help there? Lines 84 to 90 in 3e0f8d6
|
I'm doing some testing with an additional recorder step, but it sometimes throws a MethodTooLargeException. Is it the case that a recorder method can only do a limited amount of "work"? |
Yes, since the recorder generates bytecode in a synthetic method it is constraint by the 64k limit imposed by the JVM specification. Does such a big recorded method make sense in this case (did you try looking at the decompiled code)? |
Yes, it makes sense because it's doing a significant amount of processing. Maybe it can somehow be split up to smaller chunks. |
Are there any options to explicitly exclude classes that are known to not be used at runtime? I suspect that many of the new classes are included from the SmallRyeOpenAPI.Builder class that appear to be reachable, but in practice are not due to how the builder is configured. Otherwise, I suspect that the |
You can potentially use the Unfortunately there is still no way to explicitly exclude classes (even if they seem reachable) please see oracle/graal#3225
Yes, that would be a way to go as well.
Judging by your comment I would say your understanding is pretty good already :) You can think of it as follows: |
With changes to the smallrye builder to split the build function so that the Quarkus extensions can override it, I have some promising results locally:
With updates:
|
That is pretty promising @MikeEdgar , great work! |
smallrye-open-api 3.11 introduced a new builder style API that consolidates the process of building an OpenAPI model from the various sources (reader, static file, and annotations). This PR migrates to the new API and picks up several fixes for reported Quarkus issues as noted below.
Fixes #42101
Fixes #42221