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

Revisit AOT constructor and factory method resolution #27920

Closed
snicoll opened this issue Jan 11, 2022 · 8 comments
Closed

Revisit AOT constructor and factory method resolution #27920

snicoll opened this issue Jan 11, 2022 · 8 comments
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

@snicoll
Copy link
Member

snicoll commented Jan 11, 2022

We currently resolve the constructor or factory method to use to instantiate a bean using the package private ConstructorResolver class. When processing a BeanFactory at build-time, we need to retain that information (without instantiating the bean) so that we lower reflection and processing at runtime.

Right now, ConstructorResolver:

  • Detect and instantiate the Constructor to use.
  • Detect the factory method to use and set some additional metadata in the bean definition
  • Instantiate the bean using the state of the bean definition

Rather than returning BeanWrapper we could return an object that provides the metadata that we need and wrap the instantiation call for the regular runtime behavior. However, the difference between constructor and factory method is annoying as I'd rather have a consistent API for both.

It is also unknown how we'd store the resolved arguments or if we'd need to.

@snicoll snicoll added type: enhancement A general enhancement theme: aot An issue related to Ahead-of-time processing labels Jan 11, 2022
@snicoll snicoll added this to the 6.0.0-M3 milestone Jan 11, 2022
@snicoll snicoll self-assigned this Jan 11, 2022
@snicoll snicoll added the in: core Issues in core modules (aop, beans, core, context, expression) label Jan 11, 2022
@snicoll snicoll removed their assignment Jan 11, 2022
@snicoll
Copy link
Member Author

snicoll commented Mar 9, 2022

FTR we have a very hackish/temporary version in the meantime in DefaultBeanRegistrationContributionProvider.

@jhoeller jhoeller modified the milestones: 6.0.0-M3, 6.0.0-M4 Mar 16, 2022
@jhoeller jhoeller modified the milestones: 6.0.0-M4, 6.0.0-M5 May 4, 2022
@jhoeller
Copy link
Contributor

On a related note, after #28414, the ConstructorOrFactoryMethodResolver class re-implementing constructor resolution at this point refers to the @Autowired annotation directly, creating a package cycle between beans.factory.aot and beans.factory.annotation. This should be resolved along the proposed reusable constructor resolution algorithm here.

jhoeller added a commit that referenced this issue May 12, 2022
This reduces the package cycle between aot and annotation to an Autowired annotation reference in ConstructorOrFactoryMethodResolver, to be resolved along with gh-27920.

See gh-28414
@jhoeller jhoeller modified the milestones: 6.0.0-M5, 6.0.0-M6 Jul 8, 2022
@snicoll
Copy link
Member Author

snicoll commented Sep 9, 2022

Juergen and I brainstormed about this one today and we're thinking about an API that would provide a richer model to account, namely, for generic constructor argument values.

Given a BeanDefinition, the resolver would return:

  1. The Executable to use
  2. A view over the arguments that have been "pre-converted" if necessary. This includes TypeStringValue that exposes a raw String value with an optional type that should be taken into account to determine the constructor anyway. There are other cases where the input value doesn't strictly match the parameter type, including for indexed constructor arguments.
  3. Potentially a way to determine that an argument has to be autowired.

Based on this model we could generate code so that a ConstructorArgumentValue is already in the expected type. If a generic constructor argument has been resolved, it becomes indexed rather so that code instantiation is as straightforward as possible.

A further refinement is the case where all the arguments are known upfront, avoiding the use of AutowiredArguments altogether.

There is such an example of such a generic argument in #29087.

@snicoll
Copy link
Member Author

snicoll commented Sep 14, 2022

Regarding 2. I should add some sort of SPI for this would be very much welcome as this could be reused transparently for PropertyValues. The conversion for the expected type can happen and it could handle TypeStringValue transparently as well.

@jhoeller jhoeller changed the title Expose a bean instance creator resolver API Revisit AOT constructor and factory method resolution Oct 5, 2022
@jhoeller
Copy link
Contributor

jhoeller commented Oct 5, 2022

I'm narrowing the scope of this ticket for our immediate purposes in 6.0, namely merging AOT constructor and factory method resolution into ConstructorResolver in the beans.factory.support package. This moves related code into the same class, unifies candidate determination for constructors and factory methods, and gets rid of the package cycle around the hard-coded Autowired annotation check (which is implicitly coming from AutowiredAnnotationBeanPostProcessor via the determineCandidateConstructors SPI now). The API entry point for AOT pre-resolution purposes is in RegisteredBean now.

Follow-up work might happen before GA still or afterwards in the 6.0.x line, ideally without affecting applications or the rest of the portfolio.

@snicoll
Copy link
Member Author

snicoll commented Oct 8, 2022

@jhoeller I didn't see a follow-up issue for the argument resolution that is quite important for XML-based scenarios. I've created another issue.

@CynanX
Copy link

CynanX commented Oct 31, 2022

HI, do you have a link to the follow up issue, please so I can follow it?

With the latest snapshots I still see the issue as I raised under #29052 when trying to compile as native.

@snicoll
Copy link
Member Author

snicoll commented Oct 31, 2022

Alright. Please create a separate issue and share a small sample we can use to reproduce.

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

3 participants