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

Introduce PreComputeFieldFeature based on GraalVM FieldValueTransformer API #29081

Closed
sdeleuze opened this issue Sep 5, 2022 · 8 comments
Closed
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) theme: aot An issue related to Ahead-of-time processing type: enhancement A general enhancement
Milestone

Comments

@sdeleuze
Copy link
Contributor

sdeleuze commented Sep 5, 2022

Current ConstantFieldFeature uses GraalVM internal API, requiring Java module exports and making our support pretty fragile.

The new FieldValueTransformer API available thans to this PR in GraalVM 22.3 snapshots is designed to provide the same behavior in a more robust and maintainable way.

Depends on #29080.

@sdeleuze sdeleuze added in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement theme: aot An issue related to Ahead-of-time processing labels Sep 5, 2022
@sdeleuze sdeleuze added this to the 6.0.0-RC2 milestone Sep 5, 2022
@sdeleuze sdeleuze self-assigned this Sep 5, 2022
@sdeleuze
Copy link
Contributor Author

sdeleuze commented Sep 8, 2022

I have pushed a refined version of the implementation which works as expected.

About the 2 issues mentioned previously:

  • It can be surprising but the lambda passed to DuringAnalysisAccess#registerFieldValueTransformer is only invoked for reachable fields, so for example it will not be invoked for RestTemplate#gsonPresent when Jackson is in the classpath because there is no need to set the value of unreachable fields that won't be included in the native image.
  • A NoClassDefFoundError is thrown by subtype.getDeclaredFields() when some field types are not in the classpath. As far as I can tell, those classes would also throw an error on the JVM so this is just happening on classes not reachable in practice during the execution.

The next step is to decide if we keep the patterns or if we introduce an annotation to identify those fields computed at build time on native and how this annotation should be named.

I have mixed feelings : an annotation can easily be noticed when browsing the source code, but the fact the annotation will have an effect only on native and not on the JVM makes me wondering if that's a good idea to introduce it, at least for now. Also if introduced at Spring Framework level, Reactor won't be able to leverage it I (that said that's conceptually weak for Spring to set a Reactor field, and it could do it differently so this point is not blocking).

@christianwimmer
Copy link

It can be surprising but the lambda passed to DuringAnalysisAccess#registerFieldValueTransformer is only invoked for reachable fields

Yes, that is indeed the case. But is there anything you would like to do for non-reachable fields?

@christianwimmer
Copy link

The next step is to decide if we keep the patterns or if we introduce an annotation

Annotations are more explicit. But there is one important thing you need to be careful with. Calling getAnnotation(Class c) on a class, method, field, ... triggers initialization of the classes of all annotations that are on that element - not just the annotation class you are asking for. When discussing that with Paul recently, we realized that we need to expose in the native image API a way to ask for an annotation without triggering all that initialization. That is on the list for this release, see also the brief mentioning of this in oracle/graal#4862 (comment)

@sdeleuze
Copy link
Contributor Author

@christianwimmer Thanks for the update, please let me know when annotations support is available in 22.3.0-dev builds.

@sdeleuze
Copy link
Contributor Author

Once possible, I may give it a try with @PreCompute annotation name and rename the feature accordingly.

We should also try to see if we can support immutable Set<String> field for PathMatchingResourcePatternResolver#systemModuleNames as described in #29183.

@simonbasle @violetagg That means we will likely remove Reactor field handling, I will reach you to discuss how we should handle that.

sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Sep 21, 2022
spring-projectsgh-28506 introduces a big footprint regression on
native, so it should for now be skipped when
compiling to native images. Such support could
potentially be re-introduced via spring-projectsgh-29081.

Closes spring-projectsgh-29183
@christianwimmer
Copy link

@christianwimmer Thanks for the update, please let me know when annotations support is available in 22.3.0-dev builds.

It is available now: https://github.com/oracle/graal/blob/master/sdk/src/org.graalvm.nativeimage/src/org/graalvm/nativeimage/AnnotationAccess.java

@sdeleuze
Copy link
Contributor Author

sdeleuze commented Oct 6, 2022

@philwebb As discussed in latest weekly meeting, switching from pattern to annotations between RC1 and RC2 seems not possible with our release constraints. Portfolio projects are going to release against RC1 and the switch to annotations would require them to use the new annotations, test with RC2, etc. and I don't want to introduce those annotation blindly. So the best path seems to keep patterns for RCs and GA.

That will also give us more time to see what GraalVM 23.x is going to provide, if we want or not leveraging it as part of Spring Framework 6.1, so there are advantages to not publish public API for now.

@sdeleuze
Copy link
Contributor Author

This branch contains the draft commit of the new feature now names PreComputeFieldFeature and directly integrated in spring-core module since it does not require anymore JPMS exports of GraalVM internal classes.

@sdeleuze sdeleuze changed the title Update ConstantFieldFeature to leverage FieldValueTransformer Introduce PreComputeFieldFeature based on GraalVM FieldValueTransformer API Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) theme: aot An issue related to Ahead-of-time processing type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants