jamesfredley commented on PR #15800:
URL: https://github.com/apache/grails-core/pull/15800#issuecomment-4997826223
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.
--
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]
