jdaugherty commented on code in PR #15800:
URL: https://github.com/apache/grails-core/pull/15800#discussion_r3724393238
##########
grails-encoder/src/main/groovy/org/grails/encoder/CodecMetaClassSupport.groovy:
##########
@@ -38,6 +43,16 @@ class CodecMetaClassSupport {
static final Object[] EMPTY_ARGS = []
static final String ENCODE_AS_PREFIX = 'encodeAs'
static final String DECODE_PREFIX = 'decode'
+ private static final Cache<CodecFactory, Set<MetaMethodRegistrationKey>>
REGISTERED_META_METHODS = Caffeine.newBuilder()
Review Comment:
This closes the unbounded-static concern from my earlier pass. `weakKeys()`
means an entry can only outlive a factory that is already retained elsewhere,
and each factory's value set is bounded by target classes x method names -
roughly 30 small objects for the default five targets. No LRU needed at that
size.
##########
grails-encoder/src/main/groovy/org/grails/encoder/CodecMetaClassSupport.groovy:
##########
@@ -152,8 +168,79 @@ class CodecMetaClassSupport {
}
protected void addMetaMethod(List<ExpandoMetaClass> targetMetaClasses,
String methodName, Closure closure) {
+ addMetaMethod(targetMetaClasses, methodName, closure, false, null)
+ }
+
+ protected void addMetaMethod(List<ExpandoMetaClass> targetMetaClasses,
String methodName, Closure closure, boolean cacheLookup, CodecFactory
codecFactory) {
Review Comment:
`addMetaMethod(List, String, Closure)` is `protected` on a public class, so
it is an extension point. Cached registration now routes through this five-arg
overload and never calls the three-arg one, which means an external subclass
overriding it silently stops intercepting.
Nothing in this repo overrides it, so this is not urgent. Keeping the
three-arg method as the single hook and making the claim/skip decision in
`configureCodecMethods` before calling it would avoid changing the protected
surface at all.
##########
grails-encoder/build.gradle:
##########
@@ -63,4 +64,11 @@ dependencies {
apply {
from rootProject.layout.projectDirectory.file('gradle/docs-config.gradle')
from rootProject.layout.projectDirectory.file('gradle/test-config.gradle')
-}
\ No newline at end of file
+}
+
+tasks.withType(Test).configureEach {
+ systemProperties System.properties.findAll { key, value ->
key.toString().startsWith('grails.codec.benchmark.') }
Review Comment:
This does not follow the property-bridging pattern already established in
`gradle/test-config.gradle`, which this module applies:
```groovy
project.properties.each { key, value ->
if (key.toString().startsWith('grails.geb.')) {
systemProperty key, value
}
}
```
Two consequences of reading `System.properties` and using the bulk form
instead: only `-D` works, not `-P` or `local.properties` like every other
switch in this build; and iterating the full property set at configuration time
becomes a configuration-cache input the moment `org.gradle.configuration-cache`
stops being `false`. Please match the existing `grails.geb.` shape.
##########
grails-encoder/src/test/groovy/org/grails/encoder/CodecMetaClassBenchmarkSpec.groovy:
##########
@@ -0,0 +1,159 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.grails.encoder
+
+import java.util.concurrent.atomic.AtomicInteger
+
+import groovy.lang.MetaClassRegistryChangeEventListener
+import groovy.transform.CompileStatic
+import groovy.transform.TypeCheckingMode
+import org.codehaus.groovy.runtime.InvokerHelper
+
+import grails.util.GrailsMetaClassUtils
+import spock.lang.IgnoreIf
+import spock.lang.Specification
+
+@IgnoreIf({ !Boolean.getBoolean('grails.codec.benchmark.enabled') })
+class CodecMetaClassBenchmarkSpec extends Specification {
Review Comment:
The separate JavaExec is gone, which was the ask. The open question is
placement: #16071 adds a `grails-benchmarks` module with real JMH harnesses
plus per-PR regression reporting, also targeting 8.0.x. A hand-rolled
`System.nanoTime()` loop here becomes a third benchmark pattern in the tree,
and the description already concedes the wall-clock numbers were
outlier-sensitive and directional only.
I would rather this spec and the `build.gradle` block come out of this PR,
and the measurement land as a JMH benchmark on top of #16071 where the numbers
get compared automatically instead of by hand. That also makes the
`build.gradle` comment moot.
##########
grails-testing-support-web/src/main/groovy/grails/testing/web/GrailsWebUnitTest.groovy:
##########
@@ -163,10 +163,11 @@ trait GrailsWebUnitTest implements GrailsUnitTest {
}
loadedCodecs << codecClass
DefaultGrailsCodecClass grailsCodecClass = new
DefaultGrailsCodecClass(codecClass)
- grailsCodecClass.configureCodecMethods()
grailsApplication.addArtefact(CodecArtefactHandler.TYPE,
grailsCodecClass)
if (reinitialize) {
applicationContext.getBean(DefaultCodecLookup).reInitialize()
+ } else {
+ grailsCodecClass.configureCodecMethods()
Review Comment:
This branch is now the only direct `configureCodecMethods()` caller left in
the tree, and `mockCodec` has exactly one caller outside this trait
(`CodecSpec`, which takes the `reinitialize = true` default). So the `false`
path - the one `WebSetupSpecInterceptor` drives for every web spec - has no
coverage at all.
Please add a spec asserting `mockCodec(SomeCodec, false)` still installs the
`encodeAs*` methods.
##########
grails-encoder/src/main/groovy/org/grails/encoder/CodecMetaClassSupport.groovy:
##########
@@ -152,8 +168,79 @@ class CodecMetaClassSupport {
}
protected void addMetaMethod(List<ExpandoMetaClass> targetMetaClasses,
String methodName, Closure closure) {
+ addMetaMethod(targetMetaClasses, methodName, closure, false, null)
+ }
+
+ protected void addMetaMethod(List<ExpandoMetaClass> targetMetaClasses,
String methodName, Closure closure, boolean cacheLookup, CodecFactory
codecFactory) {
targetMetaClasses.each { ExpandoMetaClass emc ->
- emc."${methodName}" << closure
+ if (!cacheLookup) {
+ emc."${methodName}" << closure
+ }
+ else {
+ // Serialize only this factory's registrations; never lock the
shared EMC.
+ synchronized (factoryRegistrationLock(codecFactory)) {
+ if (shouldRegisterMetaMethod(emc, methodName,
codecFactory)) {
+ emc."${methodName}" << closure
+ }
+ }
+ }
+ }
+ }
+
+ @CompileStatic
+ private static boolean shouldRegisterMetaMethod(ExpandoMetaClass emc,
String methodName, CodecFactory codecFactory) {
+ MetaMethodRegistrationKey key = registrationKey(emc, methodName)
+ registeredMetaMethodKeys(codecFactory).add(key) ||
emc.getMetaMethod(methodName, EMPTY_ARGS) == null
+ }
+
+ @CompileStatic
+ private static MetaMethodRegistrationKey registrationKey(ExpandoMetaClass
emc, String methodName) {
+ new MetaMethodRegistrationKey(emc.getTheClass(), methodName)
+ }
+
+ @CompileStatic
+ private static Set<MetaMethodRegistrationKey>
registeredMetaMethodKeys(CodecFactory codecFactory) {
+ REGISTERED_META_METHODS.get(codecFactory) { CodecFactory ignored ->
+ Collections.newSetFromMap(new
ConcurrentHashMap<MetaMethodRegistrationKey, Boolean>())
+ }
+ }
+
+ @CompileStatic
+ private static Object factoryRegistrationLock(CodecFactory codecFactory) {
+ FACTORY_REGISTRATION_LOCKS.get(codecFactory) { CodecFactory ignored ->
+ new Object()
+ }
+ }
+
+ @CompileStatic
+ private static class MetaMethodRegistrationKey {
+
+ private final Class<?> targetClass
+ private final String methodName
+
+ MetaMethodRegistrationKey(Class<?> targetClass, String methodName) {
+ this.targetClass = targetClass
+ this.methodName = methodName
+ }
+
+ @Override
+ boolean equals(Object other) {
+ if (this.is(other)) {
+ return true
+ }
+ if (!(other instanceof MetaMethodRegistrationKey)) {
+ return false
+ }
+
+ MetaMethodRegistrationKey otherKey = (MetaMethodRegistrationKey)
other
+ targetClass == otherKey.targetClass &&
+ methodName == otherKey.methodName
+ }
+
+ @Override
+ int hashCode() {
Review Comment:
`@EqualsAndHashCode` on the class replaces both of these. As written the
`hashCode()` body reads like an unfinished `Objects.hash` - the intermediate
`result` assignment does not earn its keep with the multiplier on the next line.
##########
grails-encoder/src/main/groovy/org/grails/encoder/CodecMetaClassSupport.groovy:
##########
@@ -38,6 +43,16 @@ class CodecMetaClassSupport {
static final Object[] EMPTY_ARGS = []
static final String ENCODE_AS_PREFIX = 'encodeAs'
static final String DECODE_PREFIX = 'decode'
+ private static final Cache<CodecFactory, Set<MetaMethodRegistrationKey>>
REGISTERED_META_METHODS = Caffeine.newBuilder()
+ .weakKeys()
+ .build()
+ /**
+ * Per-factory locks keep claim+register atomic for the same CodecFactory
without
+ * synchronizing on globally shared ExpandoMetaClass instances (String,
Object, ...).
+ */
+ private static final Cache<CodecFactory, Object>
FACTORY_REGISTRATION_LOCKS = Caffeine.newBuilder()
Review Comment:
This second cache is redundant. `registeredMetaMethodKeys(codecFactory)`
already returns a stable per-factory instance, so it can serve as the monitor
directly:
```groovy
synchronized (registeredMetaMethodKeys(codecFactory)) {
```
Same locking granularity, one fewer static cache, and
`factoryRegistrationLock` plus its weak-key bookkeeping goes away.
##########
grails-codecs/src/main/groovy/org/grails/plugins/codecs/CodecsConfiguration.java:
##########
@@ -36,9 +36,7 @@ public class CodecsConfiguration {
@Bean("codecLookup")
@Primary
- public CodecLookup codecLookup(GrailsApplication grailsApplication) throws
Exception {
- final DefaultCodecLookup defaultCodecLookup = new
DefaultCodecLookup(grailsApplication);
- defaultCodecLookup.reInitialize();
- return defaultCodecLookup;
+ public CodecLookup codecLookup(GrailsApplication grailsApplication) {
Review Comment:
Correct - `BasicCodecLookup.afterPropertiesSet()` already calls
`reInitialize()`, and this is the only `codecLookup` bean definition, so the
second pass was pure waste.
Registration now rides entirely on the `InitializingBean` contract, and
there is no test anywhere touching this class. Please add one asserting the
`codecLookup` bean has codecs registered after a normal context refresh, so
this cannot silently regress if the bean is ever constructed somewhere that
skips the lifecycle.
--
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]
