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

OASFactoryResolverImpl clinit registry lazy init #1821

Merged
merged 1 commit into from
May 17, 2024

Conversation

franz1981
Copy link
Contributor

@franz1981 franz1981 commented May 3, 2024

According to the quarkus start stop tests (at quarkus-qe/quarkus-startstop#371):

image

https://github.com/quarkusio/quarkus/blob/4eaa586672bbc18a25f3dc82f18201260ee8ce0a/extensions/smallrye-openapi/runtime/src/main/java/io/quarkus/smallrye/openapi/runtime/OpenApiRecorder.java#L98

(which is in the flamegraph, at io/quarkus/smallrye/openapi/runtime/OpenApiRecorder.classLoaderHack)

cause SPI loading and allocating OASFactoryResolverImpl , causing its registry allocation (with the many class loading events it brings...).

By default just hitting

$ curl http://localhost:8080/q/openapi

nor any of the endpoints, I'm not able to get OASFactoryResolverImpl::createObject to be used, but please correct me if I have some naive assumption on how this is supposed to work.

WIth this PR the registry allocation/population, would happen lazily, making it lighter and faster application startup if this registry map isn't used.

@franz1981
Copy link
Contributor Author

Regardless, I'm a bit concerned on the lack of concurrency protection while writing/reading on HashMap, but is a different problem...

@Sanne
Copy link

Sanne commented May 3, 2024

Good idea!

MikeEdgar
MikeEdgar previously approved these changes May 3, 2024
Copy link
Member

@MikeEdgar MikeEdgar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @franz1981 , this looks good. The put method previously used to load the map can be taken out now too.

@MikeEdgar MikeEdgar added this to the 3.11.0 milestone May 3, 2024
@MikeEdgar
Copy link
Member

@franz1981 I'm considering doing a release this week and updating the version used by Quarkus (which requires some code changes as well). I think this PR would be good to get in for the release. Are you planning to push another commit to remove the put on the REGISTRY?

@franz1981
Copy link
Contributor Author

Sure thing, I was 100% sure to have pushed it already, but is likely that I didn't, sorry for that!

@MikeEdgar MikeEdgar enabled auto-merge (squash) May 17, 2024 00:24
@MikeEdgar MikeEdgar merged commit 58f8390 into smallrye:main May 17, 2024
5 checks passed
Copy link

sonarcloud bot commented May 17, 2024

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.

None yet

3 participants