jdaugherty commented on PR #15800:
URL: https://github.com/apache/grails-core/pull/15800#issuecomment-4905757703
From using fabled to review this:
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 keySet().removeIf { key.stale } — a
full scan of every registered key — under the global lock on every single
registration. 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 — it's 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). 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. add(key) ||
emc.getMetaMethod(methodName, EMPTY_ARGS) == null (line 219): 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. Your original review concern stands: 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 this repo's own rules (CLAUDE.md rule 9) 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 REGISTERED_META_METHODS field with
setAccessible(true) (CodecMetaClassSupportSpec.groovy:211-225) to forcibly
clear weak references. 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; and (d) either
include the real-app benchmark harness in the PR or restate the impact with the
baseline write count and a defensible measurement.
--
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]
