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

Add support for GraalVM native-image #14

Merged
merged 6 commits into from
Apr 23, 2024

Conversation

cmdjulian
Copy link
Contributor

This pull request adds support for GraalVM native image by using Spring Boot 3 native image support.
native-image requires reflection to be register beforehand.
@Reflectiv is used to register the methods called by the starter annotated with @MqttSubscribe to native-image,

Some reading material:

@cmdjulian cmdjulian changed the title add support graalvm native image add support for GraalVM native-image Apr 6, 2024
Copy link
Member

@rubengees rubengees left a comment

Choose a reason for hiding this comment

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

Nice! I'm not very knowledgable about native images, but I think you tested this with a real application, right?

Is there a way to have an (automated) way of testing this with only this library? Once a release is shipped that promises native image compatibility we need to maintain that and make sure it keeps working.

Copy link
Member

Choose a reason for hiding this comment

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

Is this file auto generated? Where does it come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I generated the file with the native-image agent. The thing with native-image is, that you have to capture all reflections the underlying library does. Spring does already a pretty good job for that. Actually I'm unsure why we do have to register these two cglib proxy methods. They are added later on by Spring on runtime. I think this is some sort of bug by the Spring framework missing the register the reflection. I opened up a bug report on their side https://github.com/spring-projects/spring-boot/issues/40344.

Copy link
Member

Choose a reason for hiding this comment

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

I see, okay. Let's wait with this until we have an explanation or fix from the Spring maintainers.

README.md Outdated Show resolved Hide resolved
@cmdjulian
Copy link
Contributor Author

Regarding the testability of native-image, it's actually a little hard. Even Spring does it in a way, that they created a dedicated project they involve in a CI flow, basically a smoke test. I think we could come up with something as well, but the effort is quite high. When the supposed Spring bug is fixed, beside named parameters and the meta annotation @Reflective on the @MqttSubscribe as this method basically marks methods which are called reflectively, nothing else should be required.

I also created a small application testing it and it works as expected. That's how I found out about the missing reflection for cglib.

@rubengees
Copy link
Member

Alright, thanks for the explanation. I would be fine without dedicated tests if they are hard to write once we get rid of the manual configuration as discussed above.

@cmdjulian
Copy link
Contributor Author

cmdjulian commented Apr 15, 2024

I found a way to resolve the problem. The problems stems from the fact, that using @Lazy initializes a proxy which Spring seems to miss to register. The proxy is just a delegate to the underlying BeanFactory to retrieve the object up on first access. We can do that our self and even omit the proxy like this.
This is in line what Spring suggest for autowiring BeanPostProcessor, because of the aforementioned performance benefits to it. Let me know what you think :)
Now native works out of the box and we can even omit the expensive @Lazy.

One thing though, There seems to be no implementation for test of ObjectProvider available. So I just wrote it myself. We would not need that, if we abstract the collector away via an interface. The integration tests already take care of using the real implementation. If you don't like the self-written ObjectProvider I can also abstract the processor away if you prefer that.

Before changing that, I just wanted to make sure this is inline with what you prefer.

Copy link
Member

@rubengees rubengees left a comment

Choose a reason for hiding this comment

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

Great! I think this is a good change regardless of GraalVM support. I always wondered how to avoid the @Lazy, TIL.

I have a really minor suggestion, otherwise I'm happy to merge.

@cmdjulian
Copy link
Contributor Author

Done, I'm looking forward to integrate this four improvements when the next release is created, any time table?

Copy link
Member

@rubengees rubengees left a comment

Choose a reason for hiding this comment

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

Thanks again!

@rubengees rubengees changed the title add support for GraalVM native-image Add support for GraalVM native-image Apr 23, 2024
@rubengees rubengees merged commit cc3c304 into SmartsquareGmbH:main Apr 23, 2024
@rubengees
Copy link
Member

Done, I'm looking forward to integrate this four improvements when the next release is created, any time table?

Sorry for the delay, I was a bit busy. I've released 0.17.0 with all the changes which will be available soon on maven central 🙂.

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.

2 participants