Skip to content

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

Merged
merged 2 commits into from
Sep 1, 2022

Conversation

artembilan
Copy link
Member

Fixes #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

@artembilan
Copy link
Member Author

@mhalbritter, FYI.

Would be great if you review this and leave some your feedback.

I tested the solution against the mentioned integration application in Spring Native at sb-3.0.x branch.
It works well without Spring Data. Its failure, though, is not relevant to Spring Integration: Spring Data team investigates some effort for inner bean with Spring AOT.
Perhaps I will need more serialization hints when my GenericJackson2JsonRedisSerializer is available with Spring Data fix. But this one can simply go to the next PR/commit.

Thanks

reflectionHints.registerTypeIfPresent(classLoader, "org.springframework.integration.xml.xpath.XPathUtils",
builder -> builder.withMembers(MemberCategory.INVOKE_PUBLIC_METHODS));

Stream.of(

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.

Copy link

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?

Copy link
Member Author

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.

Copy link

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)?

Copy link
Member Author

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!

Copy link
Member Author

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);

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?

Copy link

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?

Copy link
Member Author

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...

Copy link

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.

Copy link
Member Author

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.

Copy link
Member

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

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?

Copy link
Member Author

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 ->

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?

Copy link
Member Author

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

@mhalbritter
Copy link

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,
Copy link

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.

Copy link
Member Author

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?

Copy link

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?

Copy link
Member Author

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.

@artembilan artembilan force-pushed the GH-3828 branch 3 times, most recently from 485457e to 580ca6e Compare July 12, 2022 20:36
@artembilan
Copy link
Member Author

@sdeleuze , @mhalbritter , @snicoll ,

it looks like I have answered for all your questions and we have some plan going forward in the #3844.
Is there anything what keeps us from merging this?

Thanks

@mhalbritter
Copy link

Fine for me.

@artembilan
Copy link
Member Author

This is ready for review and possibly merge.
The PR for spring-aot-smoke-tests project is follow up.

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
@garyrussell garyrussell merged commit 5080fc2 into spring-projects:main Sep 1, 2022
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.

Missing runtime hints for GenericHandler
5 participants