-
Notifications
You must be signed in to change notification settings - Fork 1.1k
GH-3828: Initial Spring AOT support #3832
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
Conversation
@mhalbritter, FYI. Would be great if you review this and leave some your feedback. I tested the solution against the mentioned Thanks |
reflectionHints.registerTypeIfPresent(classLoader, "org.springframework.integration.xml.xpath.XPathUtils", | ||
builder -> builder.withMembers(MemberCategory.INVOKE_PUBLIC_METHODS)); | ||
|
||
Stream.of( |
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 looks like a 3rd party hint for kotlin and we should not put in our codebase, but contribute it to graalvm-reachability-metadata
.
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.
It depends if those Kotlin types are used for Spring Integration specific features, in that case could make sense to keep them here.
@artembilan Could you share more context why those Kotlin hints are needed and where is the related code in Spring Integration?
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.
The related code is in the https://github.com/spring-projects/spring-integration/blob/main/spring-integration-core/src/main/java/org/springframework/integration/util/ClassUtils.java.
I need this info at runtime to determine if I deal with this or that lambda representation.
Having Kotlin as optional dependency I don't see other way unless I calculate it through the reflection and use later on respectively.
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.
Ok I think that's ok since those signals are Spring Integration specific.
Not sure if that make sense or not, but notice you could refine the implementation with boolean static flags to allow build time code removal when Kotlin is not in the classpath, see spring-projects/spring-framework@f8508c8. That that's only worth to if the code conditionally executed by those functions are significant enough.
Side question : are isKotlinFaction0
and isKotlinFaction1
typos (I would expect isKotlinFuction0
and isKotlinFuction1
)?
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.
Doh! I will fix those typos with deprecation in the previous version, removal in the current 6.0
.
Yes, I will consider to make those class variables conditional based on proposed KotlinDetector
.
Thank you!
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.
So, I added if (KotlinDetector.isKotlinPresent()) {
before trying to load those Kotlin classes I need in other places where I call methods via reflection.
Another complexity with this that Kotlin is optional dep, so I cannot call its methods directly unlike java.util.function
classes...
registerSpringJdkProxy(proxyHints, IntegrationFlow.class, SmartLifecycle.class); | ||
|
||
// For MessagingAnnotationPostProcessor | ||
RuntimeHintsUtils.registerAnnotation(hints, Bean.class); |
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.
I wonder why this is necessary. This looks like that it should be in Spring Framework itself. @sdeleuze, your opinion on that?
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.
Indeed this RuntimeHintsUtils.registerAnnotation(hints, Bean.class);
hint looks suspicious, it should not be needed and in any case it should not live there.
@artembilan Could you share more why this is needed, the error you see when not present?
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.
I left comment on this line:
// For MessagingAnnotationPostProcessor
That one is a BeanPostProcessor
and I read annotation info from methods to register some additional beans.
I need some info from the @Bean
annotation to determine a target bean name.
I discussed this with @wilkinsona , but it doesn't look for me (yet) feasible to rework that algorithm to the BeanFactoryPostProcessor
...
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.
Let's maybe discuss that with @snicoll when he will be back, it would be great to rework it before we reach GA IMO.
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.
Right. Perhaps there will be enough to take a bean definition and analyze its annotation metadata, but I guess it is not present over there yet.
Saw some comment from @snicoll about regression from the previous AOT engine.
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.
I've created #3844 for the need for @Bean
lookup.
@@ -123,6 +124,7 @@ | |||
|
|||
private volatile boolean initialized; | |||
|
|||
@Reflective // The native image doesn't see this method because its type is not specific |
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.
I don't understand this comment. What is the problem there?
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.
See the method signature: setHandler(Object)
.
It works well with regular Java, but in the native image it just doesn't see this method: perhaps it tries to match the setter with the type we provide for the property.
And unfortunately that type is not always just MessageHandler
. It can be a ReactiveMessageHandler
.
I cannot have a couple setters because Java Beans introspector cannot guess what setter I'd like to use for XML bean definition parser.
@Override | ||
public void registerHints(RuntimeHints hints, ClassLoader classLoader) { | ||
ReflectionHints reflectionHints = hints.reflection(); | ||
reflectionHints.registerType(WebHandler.class, builder -> |
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.
You're registering types from spring-web
for reflection. Is that necessary because Spring integration needs this or because of some Spring Framework internal stuff doesn't work without it?
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.
Correct. The method for web is called by reflection (see Spring MVC @RequestMapping
infrastructure) and I need this type for my framework components.
Discussed this with @wilkinsona
Hey @artembilan, i've looked at the code and left some comments. I can't tell if the bean factory stuff is fine, as I don't have enough knowledge in Spring Integration / Spring Framework internals yet. Maybe asking for review from one of the more experienced colleagues helps :) |
|
||
SerializationHints serializationHints = hints.serialization(); | ||
Stream.of( | ||
String.class, |
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.
Please share more context why such generic type reflection hints are needed.
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.
Hi @sdeleuze !
Thank you for review!
Well, these are not reflection. This is for serialization.
The types I specify here are part of the Message<?>
hierarchy and its headers.
I believe that end-users would need to add them manually every time they need to serialize the message. In most cases we talk about a message store abstraction where Java serialization comes by default.
Does it make sense?
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.
It does but I think I would favor dynamic hints, potentially inspired from BindingReflectionHintsRegistrar but for Java Serialization rather than hardcoding those types. Maybe you could try initially to have it in spring-integration
and we see in a second time if that makes sense to move it to Framework?
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.
OK. If you see that it makes sense to have a org.springframework.messaging.Message
hierarchy serialization hints in the Framework, then feel free to raise an issue over there.
Meanwhile I don't see how to make this as smart algorithm to determine all the types related to the Message<?>
, its impls and headers.
So, it is OK for me to stick with this hard-coded list since it makes it very clear what types are supported by message serialization as an out-of-the-box feature.
485457e
to
580ca6e
Compare
@sdeleuze , @mhalbritter , @snicoll , it looks like I have answered for all your questions and we have some plan going forward in the #3844. Thanks |
Fine for me. |
spring-integration-core/src/main/resources/META-INF/spring/aot.factories
Outdated
Show resolved
Hide resolved
This is ready for review and possibly merge. |
Fixes spring-projects#3828 * Provide an infrastructure based on a new Spring AOT engine in the latest Spring Framework * Introduce `RuntimeHintsRegistrar` impls into modules which require some reflection, resources or proxies and serialization available in the native image * Mark some framework method with the `@Reflective` to make their reflection info available in the native image, for example for SpEL or JMX invocations * Add a `GatewayProxyBeanRegistrationAotProcessor` to register proxy interfaces info for messaging gateways (either instance of the `GatewayProxyFactoryBean`) * Rework `ConverterRegistrar` to not use a `beanFactory.getBeansWithAnnotation()` since it is not available after AOT phase. Instead, register an intermediate `IntegrationConverterRegistration` bean definition from the `IntegrationConverterInitializer` * Refactor `GlobalChannelInterceptorInitializer` a bit according to a new logic in the `IntegrationConverterInitializer` * Remove `JsonNodeWrapperToJsonNodeConverter` bean registration from the `DefaultConfiguringBeanFactoryPostProcessor` - it is added by the `ConverterRegistrar` into the target `ConversionService` * Fix `ParentContextTests` respectively a `JsonNodeWrapperToJsonNodeConverter` bean removal * Refactor `XsltPayloadTransformer` to not load a `ServletContextResource`, but just use its name for the `xslResource` condition
* Remove `@Bean` reflection since it is not needed any more * Add `AotDetector.useGeneratedArtifacts()` condition to not register beans one more time at runtime after AOT build phase * Fix deprecation in the WebFlux test from the latest SF
Fixes #3828
RuntimeHintsRegistrar
impls into modules which require some reflection,resources or proxies and serialization available in the native image
@Reflective
to make their reflectioninfo available in the native image, for example for SpEL or JMX invocations
GatewayProxyBeanRegistrationAotProcessor
to register proxy interfacesinfo for messaging gateways (either instance of the
GatewayProxyFactoryBean
)ConverterRegistrar
to not use abeanFactory.getBeansWithAnnotation()
since it is not available after AOT phase.
Instead, register an intermediate
IntegrationConverterRegistration
bean definitionfrom the
IntegrationConverterInitializer
GlobalChannelInterceptorInitializer
a bit according to a new logic in theIntegrationConverterInitializer
JsonNodeWrapperToJsonNodeConverter
bean registration from theDefaultConfiguringBeanFactoryPostProcessor
- it is added by theConverterRegistrar
into the target
ConversionService
ParentContextTests
respectively aJsonNodeWrapperToJsonNodeConverter
bean removalXsltPayloadTransformer
to not load aServletContextResource
, but just use itsname for the
xslResource
condition