codeconsole commented on code in PR #16149:
URL: https://github.com/apache/grails-core/pull/16149#discussion_r3888770694
##########
grails-web-databinding/src/main/groovy/grails/web/databinding/DataBindingUtils.java:
##########
@@ -774,4 +800,65 @@ private static Object unwrapGString(Object value) {
}
return value;
}
+
+ /**
+ * The data binding beans of a single {@link ApplicationContext}, resolved
on first use and kept for as long as
+ * that context is the one being bound against.
+ */
+ private static final class ContextBoundBeans {
+
+ private final ApplicationContext applicationContext;
+ private final CachedBean<DataBindingSourceRegistry>
dataBindingSourceRegistry =
+ new CachedBean<>(DataBindingSourceRegistry.BEAN_NAME,
DataBindingSourceRegistry.class);
+ private final CachedBean<MimeTypeResolver> mimeTypeResolver =
+ new CachedBean<>(MimeTypeResolver.BEAN_NAME,
MimeTypeResolver.class);
+ private final CachedBean<DataBinder> dataBinder =
+ new CachedBean<>(DATA_BINDER_BEAN_NAME, DataBinder.class);
+
+ private ContextBoundBeans(ApplicationContext applicationContext) {
+ this.applicationContext = applicationContext;
+ }
+
+ private DataBindingSourceRegistry getDataBindingSourceRegistry() {
+ return dataBindingSourceRegistry.get(applicationContext);
+ }
+
+ private MimeTypeResolver getMimeTypeResolver() {
+ return mimeTypeResolver.get(applicationContext);
+ }
+
+ private DataBinder getDataBinder() {
+ return dataBinder.get(applicationContext);
+ }
+ }
+
+ /**
+ * A singleton bean looked up at most once per {@link ApplicationContext}.
Each bean is resolved lazily so that
+ * asking for one of them never triggers the creation of another.
+ * <p>
+ * Both fields are volatile and {@code resolved} is written last, so a
thread which sees {@code resolved} also
+ * sees the bean it was resolved to. Two threads racing simply look the
same singleton up twice.
+ *
+ * @param <T> the type of the bean
+ */
+ private static final class CachedBean<T> {
+
+ private final String beanName;
+ private final Class<T> beanType;
+ private volatile T bean;
+ private volatile boolean resolved;
+
+ private CachedBean(String beanName, Class<T> beanType) {
+ this.beanName = beanName;
+ this.beanType = beanType;
+ }
+
+ private T get(ApplicationContext applicationContext) {
+ if (!resolved) {
+ bean = applicationContext.containsBean(beanName) ?
applicationContext.getBean(beanName, beanType) : null;
+ resolved = true;
+ }
+ return bean;
+ }
Review Comment:
Good catch — you're right, and it's fixed in cfc47a55fe.
The miss was memoized as firmly as a hit: `bean = null` and `resolved =
true` were set together, so every later call short-circuited on `resolved` and
returned null for the life of the context. A bean registered after that first
attempt was never seen.
I took your suggestion of keying on the bean and dropping `resolved`, with
one small change — only writing the field when a bean is actually found, which
avoids a redundant volatile write on the miss path and keeps Checkstyle happy
about the ternary:
```java
private T get(ApplicationContext applicationContext) {
T resolved = bean;
if (resolved == null && applicationContext.containsBean(beanName)) {
resolved = applicationContext.getBean(beanName, beanType);
bean = resolved;
}
return resolved;
}
```
Worth noting for this PR in particular: the hot path did not get slower.
Once the bean is found this reads **one** volatile field instead of two, so the
steady state is marginally cheaper than what it replaces. The repeated
`containsBean` only happens while a bean is genuinely absent, which is a
context still being populated rather than the per-request path this PR is about.
Added a regression test, `DataBindingUtilsSpec.'test a bean registered after
a lookup found nothing is still picked up'`. I confirmed it actually catches
this by reverting the implementation and watching it fail, then restoring it:
```
// with the original implementation
DataBindingUtilsSpec > test a bean registered after a lookup found nothing
is still picked up FAILED
8 tests completed, 1 failed
```
`grails-web-databinding`, `grails-databinding` and `grails-controllers` are
green, and `codeStyle` is clean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
