WIP: Add AOT support for binder child contexts #2456
Conversation
| import org.springframework.util.Assert; | ||
|
|
||
| /** | ||
| * @author Chris Bono |
There was a problem hiding this comment.
I owe javadocs to this one
| return (Binder<T, ConsumerProperties, ProducerProperties>) this.binderInstanceCache | ||
| .get(configurationName).getKey(); | ||
| } | ||
|
|
||
| @SuppressWarnings("unused") |
There was a problem hiding this comment.
[UNRELATED] This has been in here for quite a while, its unused, the file has grown very large, we can pull it back out of git history if we ever really need it.
| Date d = new Date(System.currentTimeMillis() | ||
| + this.bindingServiceProperties.getBindingRetryInterval() * 1_000); | ||
| this.taskScheduler.schedule(task, d.toInstant()); | ||
| this.taskScheduler.schedule(task, Instant.ofEpochMilli(System.currentTimeMillis() |
There was a problem hiding this comment.
[UNRELATED] I am expecting this to start failing on the CI build very soon
There was a problem hiding this comment.
Ha! I figured that would (or had to) be the case. However, I saw it green in Jenkins and did not see a related commit so was a bit confused. Cool. This should go away during rebase (or I will pull this out and repush). Thanks for heads up.
There was a problem hiding this comment.
It is better to rebase the PR branch against the latest main.
| @@ -174,17 +176,24 @@ public BindingHandlerAdvise BindingHandlerAdvise( | |||
|
|
|||
| @Bean | |||
| @ConditionalOnMissingBean(BinderFactory.class) | |||
| public BinderFactory binderFactory(BinderTypeRegistry binderTypeRegistry, | |||
| public DefaultBinderFactory binderFactory(BinderTypeRegistry binderTypeRegistry, | |||
There was a problem hiding this comment.
AOT requires typed bean return types.
onobc
force-pushed
the
cbono-aot-child-context-2455
branch
from
8fca943 to
fef31be
Compare
| DefaultBinderFactory binderFactory = new DefaultBinderFactory( | ||
| getBinderConfigurations(binderTypeRegistry, bindingServiceProperties), | ||
| binderTypeRegistry, binderCustomizerProvider.getIfUnique()); | ||
| binderFactory.setDefaultBinder(bindingServiceProperties.getDefaultBinder()); | ||
| binderFactory.setListeners(this.binderFactoryListeners); | ||
| binderChildContextInitializer.setBinderFactory(binderFactory); |
There was a problem hiding this comment.
This avoid timing and circular dependencies between the 2. Really binder factory (BF) does not know about binderChildContextInitializer (BCCI) but we need to set the initializers early enough so that when FunctionConfiguration.afterPropertiesSet() starts requesting binders to bind the functions to that the AOT context is used and not the normal JVM one.
I will add the above to a shorter comment in the code as well
| @Override | ||
| public BeanRegistrationAotContribution processAheadOfTime(RegisteredBean registeredBean) { | ||
| if (registeredBean.getBeanClass().equals(getClass())) { //&& registeredBean.getBeanFactory().equals(this.context)) { | ||
| this.logger.debug(() -> "Beginning AOT processing for binder child contexts"); |
There was a problem hiding this comment.
It is better to not use lambda style, if you feed just plain string.
Saw similar things in other places, like Assert.notNull(this.context, () -> "Unable to lookup binder factory from context as this.context is null");
There was a problem hiding this comment.
Yeh, I went w/ consistency over performance but am happy to switch.
| @@ -302,99 +304,61 @@ private <T> Binder<T, ConsumerProperties, ProducerProperties> getBinderInstance( | |||
| this.binderInstanceCache.put(configurationName, | |||
| new SimpleImmutableEntry<>(binder, binderProducingContext)); | |||
| } | |||
| logger.info("Retrieving cached binder: " + configurationName); | |||
| logger.trace("Retrieving cached binder: " + configurationName); | |||
There was a problem hiding this comment.
And this one better to use with a lambda 😄
There was a problem hiding this comment.
I resisted the urge of switching the DBF to use the new log accessor so that is not an option in this PR.
| binderProducingContext = this.initializeBinderContextSimple(configurationName, binderProperties, | ||
| binderType, binderConfiguration, false); | ||
| GenericApplicationContext c = null; | ||
| this.binderChildContextInitializers.get(configurationName).initialize(binderProducingContext); |
There was a problem hiding this comment.
In our face-to-face review with Chris we have realized that after AOT we have already all the bean definitions for this kind of child application context.
So, it looks like an overhead to still call that whole initializeBinderContextSimple() which with the provided configuration going through all the scanning and parsing stuff which in the end is replaces with this initialize() call result.
There was a problem hiding this comment.
Yep, good catch @artembilan . I will amend this.
These changes are WIP and would also be a good candidate for a walkthrough over Zoom.
TODO
FunctionConfiguration(pollable source currently set tonull)NOTE
To test drive this out, follow the directions in spring-attic/spring-native#1672
Fixes #2455
cc: @OlgaMaciaszek