Reduce cached codec metaclass registration overhead

[jamesfredley]

The Problem

Fixes #15374.

Grails codec registration dynamically adds encodeAs* and decode* methods onto globally hot metaclasses (String, GStringImpl, StringBuffer, StringBuilder, Object). On Groovy Indy, repeatedly mutating those ExpandoMetaClass types invalidates call sites and creates avoidable startup/runtime overhead. The clearest, fully compatible win is to stop re-adding the exact same cached codec method when the same CodecFactory is already registered for the same target class.

The Fix (reshaped after review)

Two layers, deliberately simple:

1. Stop duplicate registration at the callers

• CodecsConfiguration.codecLookup no longer calls reInitialize() manually. DefaultCodecLookup already implements InitializingBean; Spring's afterPropertiesSet() performs the single startup registration pass. This removes a full double-registration at bean creation.
• GrailsWebUnitTest.mockCodec either adds the artefact and reInitialize()s, or calls configureCodecMethods() once - not both.

2. Light registry only where still needed (CodecMetaClassSupport)

• Caffeine weakKeys() cache keyed on factory identity.
• Registration keys use target Class<?> + method name (not class name strings), so reloaded/plugin classloaders with the same simple name do not collide.
• Claim-and-register is serialized with a per-factory lock from a second Caffeine weakKeys() cache - never synchronized(emc) on globally shared metaclasses.
• If a target metaclass was replaced and the method is missing, registration is re-done even when the factory was seen before.
• Non-cached (development/reload) path is unchanged: still re-resolves and reattaches.

What is deliberately preserved

• Distinct factory instances stay distinct even with the same codec name (last writer wins, as before).
• Aliases still register through the same path.
• No public API, config, dependency, or migration change - apps keep calling value.encodeAsHTML(), value.encodeAsURL(), value.decodeSomeCodec() exactly as before.

Impact framing

Problem shape (baseline): repeated same-factory EMC writes from:

1. CodecsConfiguration calling reInitialize() and Spring then calling afterPropertiesSet() again
2. mockCodec calling configureCodecMethods() and then reInitialize() which calls it again
3. Multiple DefaultCodecLookup.reInitialize() passes over the same artefact instances

What this PR reduces: redundant same-factory writes via caller dedupe + Class-keyed Caffeine safety net.

Measurement notes: An opt-in microbenchmark remains at :grails-encoder (grails.codec.benchmark.enabled=true). Earlier real-app timing numbers on this PR were outlier-sensitive (mean vs median gap); treat them as directional only. The durable claim is fewer redundant EMC mutations on the startup/reInitialize path, not a single wall-clock number.

Scope

Codec-registration slice of the broader #15374 / Indy-metaclass performance topic. GORM dynamic methods, taglib dispatch, static compilation, artefact indexing, and metaclass freeze mechanisms are out of scope here.

Testing

• :grails-encoder:test - CodecMetaClassSupportSpec (same-factory idempotence, decoder/aliases, concurrent same-factory registration, re-add after metaclass replacement, distinct-factory isolation, factory churn, non-cached reload behavior)
• Public-behavior assertions only (no production counters / private-field reflection)
• Existing :grails-test-suite-uber DefaultGrailsCodecClassTests still covers double configureCodecMethods on the artefact path

[Copilot]

Pull request overview

This pull request reduces startup/runtime overhead from repeated cached codec registration by making cached encodeAs* / decode* ExpandoMetaClass writes idempotent per (target class, codec method name, codec factory identity), while preserving non-cached (development/reload) behavior and distinct-factory semantics.

Changes:

• Add a weak-identity registration key and per-ExpandoMetaClass synchronization to skip duplicate cached meta-method writes.
• Add focused Spock coverage for cached idempotence, metaclass replacement, distinct factories, and non-cached re-registration.
• Add a benchmark harness plus a :grails-encoder:codecMetaClassBenchmark Gradle task to measure registration cost and write counts.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
grails-encoder/src/main/groovy/org/grails/encoder/CodecMetaClassSupport.groovy	Adds cached-registration dedupe bookkeeping and synchronized duplicate checks to avoid repeated ExpandoMetaClass mutation.
grails-encoder/src/test/groovy/org/grails/encoder/CodecMetaClassSupportSpec.groovy	Adds targeted tests covering idempotence, concurrency, metaclass replacement, stale-key pruning, and non-cached behavior.
grails-encoder/src/test/groovy/org/grails/encoder/CodecMetaClassBenchmark.groovy	Adds a runnable benchmark main for codec registration and encode-path timing/counters.
grails-encoder/build.gradle	Registers a JavaExec benchmark task and forwards grails.codec.benchmark.* system properties.
grails-test-suite-uber/src/test/groovy/org/grails/commons/DefaultGrailsCodecClassTests.groovy	Adds regression coverage calling configureCodecMethods() twice and validates public dynamic methods still work; improves metaclass cleanup.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov]

Codecov Report

❌ Patch coverage is 78.37838% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.3560%. Comparing base (4bc5bd8) to head (bd7ef51).

Files with missing lines	Patch %	Lines
...vy/org/grails/encoder/CodecMetaClassSupport.groovy	77.7778%	4 Missing and 4 partials ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##                8.0.x     #15800        +/-   ##
==================================================
+ Coverage     52.3426%   52.3560%   +0.0134%     
- Complexity      18293      18307        +14     
==================================================
  Files            2036       2036                
  Lines           96346      96373        +27     
  Branches        16831      16838         +7     
==================================================
+ Hits            50430      50457        +27     
+ Misses          38492      38490         -2     
- Partials         7424       7426         +2     

Files with missing lines	Coverage Δ
...org/grails/plugins/codecs/CodecsConfiguration.java	100.0000% <100.0000%> (ø)
...groovy/grails/testing/web/GrailsWebUnitTest.groovy	100.0000% <ø> (ø)
...vy/org/grails/encoder/CodecMetaClassSupport.groovy	73.6111% <77.7778%> (+17.7972%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[jdaugherty]

I'm worried about the memory impacts this has on large applications. It's rolling it's own caching mechanism when elsewhere in the code base we've used other solutions like caffeine, etc.

[jdaugherty]

From using fabled to review this:

Review: PR #15800 — Reduce cached codec metaclass registration overhead

First, confirming one premise: repeated emc."${name}" << closure does silently replace
(Groovy's registerIfClosure only throws for native Java methods), so the "redundant
writes invalidate call sites" problem is real. The fix's goal is sound; the
implementation has several problems.

Correctness / performance problems

1. The distinct-factory path is now quadratic — and the PR's own control benchmark
proves it regressed. removeStaleMetaMethodRegistrationKeys()
(CodecMetaClassSupport.groovy:224-231) runs a full scan of every registered key under
the global lock on every single registration:

private static void removeStaleMetaMethodRegistrationKeys() {
    CodecFactoryKey codecFactoryKey = (CodecFactoryKey) STALE_CODEC_FACTORIES.poll()
    while (codecFactoryKey != null) {
        REGISTERED_META_METHODS.remove(codecFactoryKey)
        codecFactoryKey = (CodecFactoryKey) STALE_CODEC_FACTORIES.poll()
    }
    REGISTERED_META_METHODS.keySet().removeIf { CodecFactoryKey key -> key.stale }
}

With N distinct factories that's O(N²), which is exactly why the PR's control case went
from ~9.96s to ~14.4s (+45%). The removeIf sweep also makes the ReferenceQueue
completely redundant — two stale-cleanup mechanisms where the queue alone (O(1)
amortized) would do.

2. synchronized (emc) locks globally shared metaclasses
(CodecMetaClassSupport.groovy:178). In production these are the metaclasses of
String, Object, StringBuilder, etc. Groovy runtime internals and arbitrary user
code can synchronize on those same monitors; taking them and then nesting the global
REGISTERED_META_METHODS lock inside creates a contention and lock-ordering hazard on
some of the hottest objects in the JVM. All codec registration also serializes through
the one static map lock.

3. The registration key uses the class name, not the class (registrationKey,
line 235):

private static MetaMethodRegistrationKey registrationKey(ExpandoMetaClass emc, String methodName) {
    new MetaMethodRegistrationKey(emc.getTheClass().getName(), methodName)
}

Under dev reload / plugin classloaders, a reloaded class with the same name collides
with the old entry, so its registration is skipped and only the getMetaMethod
fallback saves it — and that fallback is unreliable (next point).

4. The fallback check is name-only and cross-factory (line 219):

registeredMetaMethodKeys(codecFactory).add(key) || emc.getMetaMethod(methodName, EMPTY_ARGS) == null

After a metaclass is replaced, factory A's re-registration is skipped if any factory
has already attached a same-named method to the new EMC — so "distinct factories with
the same codec name stay distinct" (the PR's own claim) breaks across metaclass
replacement, silently changing which encoder wins. Worse, with
ExpandoMetaClass.enableGlobally() a freshly created String EMC can inherit the
expando method from Object's EMC, making getMetaMethod non-null and suppressing the
re-attach entirely.

5. This is ~150 lines of hand-rolled weak-identity caching — custom WeakReference
key with identity equals/hashCode, a ReferenceQueue, manual stale sweeps, hand-written
equals/hashCode on two key classes, double-checked locking.
Caffeine.newBuilder().weakKeys() gives exactly weak identity keys, thread-safe, with
none of this — and Caffeine is already used in grails-web-url-mappings,
grails-datastore-core, grails-rest-transforms, etc. All of that machinery exists to
dedupe what the PR itself says is ~110 writes per startup.

6. Is this even the right layer? DefaultCodecLookup.reInitialize() →
GrailsCodecClass.configureCodecMethods() is where the duplicates originate.
DefaultGrailsCodecClass already tracks an initialized flag for exactly this "called
more than once" situation (DefaultGrailsCodecClass.java:324-330). A per-codec-class
"already registered cached methods" guard there would be a few lines with no global
static registry at all. The PR never identifies where the same-factory duplicate calls
come from in a real app, which is the question that determines the simplest fix.

Evidence problems

7. The headline numbers aren't reproducible from the branch. The
realAppCodecBenchmark (5 app starts, 31–41% startup improvement) is not in this diff,
and no baseline write count is given — "110 writes on the branch" is meaningless without
knowing the baseline count (if baseline was ~130, the dedup saved ~20 writes and can't
explain a 5-second startup change). A mean of 16.36s vs median of 10.03s across 5
samples means one or two massive outliers dominated the mean; that's noise, not a
measurement. The PR description also still references a
:grails-encoder:codecMetaClassBenchmark JavaExec task that was removed in the third
commit.

Test / process problems

8. Tests violate the repo's own rules (CLAUDE.md rule 9 — test via public APIs) and
the production class is polluted with test instrumentation.
META_METHOD_REGISTRATION_COUNT is incremented on every registration forever in
production solely so tests can read it; clearMetaMethodRegistrationState() and
getMetaMethodRegistrationKeyCount() exist only for tests. The spec then goes further
and reflects into the private field (CodecMetaClassSupportSpec.groovy:211-225):

private static Set registeredMetaMethodKeys() {
    def field = CodecMetaClassSupport.getDeclaredField('REGISTERED_META_METHODS')
    field.accessible = true
    ((Map) field.get(null)).keySet()
}

Tests that can only assert via private internals are a signal the design isn't
observable through its public behavior.

9. Global-state test hygiene. The spec clears process-wide static registration state
in setup/cleanup, and DefaultGrailsCodecClassTests now removes the metaclasses of
String, GStringImpl, StringBuffer, StringBuilder, and Object in tearDown —
global mutations in a suite the project explicitly warns runs in parallel. TestLens
already flagged a flaky test on this PR's CI run.

10. Minor. grails-encoder/build.gradle reads System.properties at configuration
time for every Test task (configuration-cache unfriendly); the benchmark spec carries
a main() method and println reporting inside a Spock spec; hand-rolled
equals/hashCode instead of @EqualsAndHashCode.

Bottom line

The problem is real, but I'd push back on the shape of the fix:

• (a) Identify where the duplicate same-factory configureCodecMethods calls
  actually come from and dedupe at the caller
  (DefaultGrailsCodecClass/DefaultCodecLookup) if possible.
• (b) If a registry is genuinely needed, use Caffeine weakKeys() keyed on the
  factory with the target Class (not its name) in the value key, and drop the
  synchronized(emc) / global-lock / manual sweep machinery.
• (c) Remove the production-side counters and make tests observe public behavior.
• (d) Either include the real-app benchmark harness in the PR or restate the impact
  with the baseline write count and a defensible measurement.

[jamesfredley]

The problem this targets is real - repeated emc."${name}" << closure does silently replace and invalidate call sites, and reducing those redundant startup writes is worth doing (it's the codec slice of the Indy/metaclass performance topic, Codebase 2.3 / Google Doc 1.1). But the review is right that the current implementation needs reshaping, and I'd rather fix the shape than defend it:

• The stale-sweep is O(N²). removeStaleMetaMethodRegistrationKeys() does a full removeIf scan of every registered key under the global lock on every registration, which is why the control benchmark regressed ~+45%. The ReferenceQueue alone is O(1) amortized and makes the sweep redundant.
• synchronized (emc) locks globally-shared metaclasses (String, Object, StringBuilder...), which Groovy internals and user code can also synchronize on - a real contention / lock-ordering hazard, made worse by nesting the global map lock inside it.
• Name-based registration key collides under dev-reload / plugin classloaders (a reloaded same-named class overwrites the old entry), and the name-only cross-factory fallback can silently change which encoder wins.
• ~150 lines of hand-rolled weak-identity caching duplicates what Caffeine.newBuilder().weakKeys() gives for free - and Caffeine is already used in grails-web-url-mappings / grails-datastore-core.

So I agree with the direction you outlined:

1. Find where the duplicate same-factory configureCodecMethods calls actually originate (DefaultCodecLookup.reInitialize() -> GrailsCodecClass.configureCodecMethods()) and dedupe at the caller - DefaultGrailsCodecClass already tracks an initialized flag for exactly this, so a per-codec-class guard there may remove the need for a global registry entirely.
2. If a registry is still needed, use Caffeine weakKeys() keyed on the factory + target Class (not its name), and drop the synchronized(emc) / global-lock / manual-sweep machinery.
3. Move the test instrumentation (META_METHOD_REGISTRATION_COUNT, the reflect-into-private-field spec) out of the production class and assert via public behavior (repo test-via-public-API rule).
4. Either include the real-app benchmark harness in the PR or restate the impact with the baseline write count and a defensible measurement (a mean of 16.36s vs a 10.03s median is outlier-dominated).

I'll rework it this way. Thanks for the thorough review - this one's a genuinely better design after the feedback.

[jdaugherty]

@jamesfredley let me know once all of these changes are pushed and I'll review again.

[jamesfredley]

Reshape after review feedback

Pushed two commits on top of the previous approach:

1. Avoid double codec registration at startup and in mockCodec

   • CodecsConfiguration: drop manual reInitialize(); rely on InitializingBean.afterPropertiesSet()
   • GrailsWebUnitTest.mockCodec: either reInitialize() after addArtefact, or configureCodecMethods() once - not both

2. Key codec metaclass registration by Class and lock per factory

   • Registration keys: target Class<?> + method name (not class name strings)
   • Dropped synchronized(emc) on shared metaclasses
   • Per-factory claim+register lock via Caffeine weakKeys() (keeps same-factory concurrency atomic without locking String/Object EMCs)
   • Kept Caffeine factory-identity cache; no O(N²) stale sweep / hand-rolled weak refs
   • Tests assert via public encode/decode behavior only

Intentionally not done

• No outer metaMethodsConfigured flag on DefaultGrailsCodecClass - that would skip CodecMetaClassSupport's metaclass-replacement recovery path
• No real-app benchmark harness in this PR; impact restated as reduced redundant EMC writes + opt-in microbenchmark
• No production test instrumentation counters

Verification

./gradlew :grails-encoder:test --tests org.grails.encoder.CodecMetaClassSupportSpec

BUILD SUCCESSFUL (7/7).

Ready for another review pass.

[jdaugherty]

Approving.

I traced the lifecycle rather than taking the description at face value, and the mechanism holds up:

• BasicCodecLookup implements InitializingBean with afterPropertiesSet() -> reInitialize(), and this @Bean is the only codecLookup definition in the tree, so dropping the manual reInitialize() loses nothing. The legacy GSP harness registers its own codecLookup definition (AbstractGrailsTagTests), so it still gets the Spring lifecycle too.
• cacheLookup is driven off Environment.isDevelopmentMode(), so the reload path is genuinely untouched.
• The getMetaMethod(...) == null fallback is load-bearing, and the spec proves it: drop the fallback and re-adds methods after metaclass replacement fails. The tests pin the mechanism rather than a usage pattern, which is what I want to see here.
• The cached closures capture the resolved encoder eagerly, but DefaultGrailsCodecClass.initializeCodec() is one-shot guarded, so there is no stale-encoder path through the real factory implementation.
• Lock ordering is consistently factory-lock -> EMC and never touches the globally shared metaclasses, so there is no deadlock to worry about.

One behaviour change worth calling out in the description: when two codecs register the same method name or alias, a second registration pass over a replaced metaclass now lets whichever factory arrives first win, where previously each pass re-asserted last-writer-wins. It takes a name collision plus a metaclass reset to observe, so I am not treating it as a blocker, but it is the one semantic edge the fallback introduces.

Comments below. I would like the two test gaps closed before this lands - CodecsConfiguration and the mockCodec false branch are both touched with nothing covering them - and a decision on where the benchmark lives now that #16071 is in flight. Approving so neither of those needs another review round trip.