paulk-asert commented on PR #2736:
URL: https://github.com/apache/groovy/pull/2736#issuecomment-5137823765
AI read:
> @blackdrag asked how often the ancestor-metaclass pattern appears in
Grails. I looked at that — and, since the answer only matters if hierarchy
fan-out is load-bearing in the first place, I also tested that premise
empirically on this PR's head (`637bdaf`). Three parts, then a suggested way
forward.
>
> ## 1. Direct answers to the two open questions
>
> **"Are we even caching in that case? If we are then caching that would be
new in this pull request I assume."**
>
> Caching is pre-existing, but the cached thing is not a frozen miss. When
selection finds no method, `Selector.setMetaClassCallHandleIfNeeded` (unchanged
by this PR, `cache = true`) links a route through `MetaClass.invokeMethod`
bound to the receiver's MetaClass instance. Each invocation of that linked
handle re-runs `invokeMissingMethod` → `findMethodInClassHierarchy`
(`MetaClassImpl.java:950`), and the walk resolves ancestor MetaClasses
**freshly from the registry on every call** (`MetaClassImpl.java:3949`).
>
> **"I simply think … a simple uncached path through the metaclass would be
sufficient."**
>
> It's stronger than that: the cached route already *is* a live path. A
linked "miss" site observes ancestor EMC changes with **no invalidation at
all**.
>
> ## 2. Experiment: disable fan-out, test behaviour only
>
> Since the indy `Selector` consults only the receiver's own MetaClass
tables at link time, I reduced `invalidateClassHierarchy` to root-only (no
descendant walk) on the PR head and ran behaviour-only probes through hot call
sites (200 iterations to ensure linkage) — no `hasBeenInvalidated()`
assertions, only observable dispatch results:
>
> | Probe | fan-out OFF | fan-out ON |
> |---|---|---|
> | linked miss → parent EMC adds method (case 4 in the matrix above) | ✅ |
✅ |
> | linked miss on `String[]` → `Object[]` EMC adds method | ✅ | ✅ |
> | present child method vs parent adds more-specific overload | ✅ | ✅ |
> | linked hit via snapshot → parent EMC removed (expecting miss) | ❌ stale
| ❌ stale — **identical** |
>
> Observations:
>
> - The canonical "case that *does* need hierarchy SP" passes **without** it
— the dynamic route self-heals, including across the array lattice.
> - The one failing probe fails identically in both configurations: the
child MetaClass was constructed after the parent EMC add, so the method sits in
the child's own tables (construction-time snapshot). Re-linking re-selects from
those unchanged tables — no SwitchPoint policy can alter the outcome. This is
the same behaviour as @blackdrag's D/E and F/G scripts.
> - The 19 existing tests that fail with fan-out disabled all assert
**SwitchPoint retirement** (mechanism), not dispatch results.
>
> Why this holds generally: ancestor-dependent resolution reaches a linked
site in exactly two ways — the dynamic `invokeMethod` route (live per call) or
methods copied into the receiver's own MetaClass (snapshot / EMC inheritance,
where any refresh fires the *child's* own `incVersion`). Re-linking against an
unchanged receiver MetaClass reproduces the same target, so retiring subtype
SwitchPoints when only an ancestor changed appears **behaviourally inert** for
stock `MetaClassImpl`/EMC.
>
> Caveats, honestly stated: I probed method dispatch, not property sites;
and a custom MetaClass implementation that consults ancestor state during
*selection* could still go stale — but that unknown-MC case could be served by
bulk invalidation (rare event) rather than an indexed subtype walk. If there's
a counter-example where fan-out changes observable behaviour for stock
metaclasses, that would be the decisive artifact.
>
> <details>
> <summary>Probe test source (drop into <code>src/test/groovy/bugs/</code>
to reproduce)</summary>
>
> ```groovy
> package bugs
>
> import org.junit.jupiter.api.AfterEach
> import org.junit.jupiter.api.Test
>
> final class FanOutBehaviourExperiment {
>
> static class Parent {}
> static class Child extends Parent {}
> static class Base2 {}
> static class Sub2 extends Base2 {}
>
> @AfterEach
> void tearDown() {
> [Parent, Child, Base2, Sub2, Object[].class, String[].class].each {
> GroovySystem.metaClassRegistry.removeMetaClass(it)
> }
> }
>
> private static String probe(x) {
> try { return x.onlyOnParent() } catch (MissingMethodException e) {
return 'miss' }
> }
>
> @Test
> void linkedMissThenParentEmcAdd() {
> def c = new Child()
> 200.times { assert probe(c) == 'miss' } // hot-link the miss
route on Child
> Parent.metaClass.onlyOnParent = { -> 'now-visible' }
> assert probe(c) == 'now-visible' // passes with fan-out
disabled
> }
>
> private static String probe2(x) {
> try { return x.onlyOnBase() } catch (MissingMethodException e) {
return 'miss' }
> }
>
> @Test
> void linkedHitViaParentEmcThenRemove() {
> Base2.metaClass.onlyOnBase = { -> 'from-base' }
> def s = new Sub2() // Sub2 MC created
AFTER add → snapshot
> 200.times { assert probe2(s) == 'from-base' }
> GroovySystem.metaClassRegistry.removeMetaClass(Base2)
> assert probe2(s) == 'miss' // fails IDENTICALLY
with and without fan-out
> }
>
> private static String probeArr(x) {
> try { return x.arrHello() } catch (MissingMethodException e) {
return 'miss' }
> }
>
> @Test
> void linkedMissThenObjectArrayEmcAdd() {
> String[] arr = ['a', 'b']
> 200.times { assert probeArr(arr) == 'miss' }
> Object[].metaClass.arrHello = { -> 'array-visible' }
> assert probeArr(arr) == 'array-visible' // passes with fan-out
disabled
> }
> }
> ```
> </details>
>
> ## 3. Grails (the direct question)
>
> I audited `apache/grails-core` `8.0.x` (the monorepo — includes GORM, GSP,
testing-support; 87 modules).
>
> **How often does ancestor-EMC visibility occur? It's structural, not
rare:**
>
> - The codec system registers every codec's `encodeAsX`/`decodeX` onto the
EMCs of `[String, GStringImpl, StringBuffer, StringBuilder, Object]`
(`CodecMetaClassSupport.resolveDefaultMetaClasses()`). Every
`someValue.encodeAsHTML()` on an arbitrary receiver is exactly the
`Object`-ancestor pattern — registered at startup
(`DefaultCodecLookup.registerCodecs`) and per test class
(`GrailsWebUnitTest.mockCodec`).
> - Dev reload clears the MetaClasses of 15 common classes including
`Number` (`DefaultGrailsPluginManager.COMMON_CLASSES`).
>
> **But** these events fire at startup / reload / test-setup, and both
resolution modes they produce (construction snapshot; dynamic walk) behave
identically with fan-out disabled per §2. Also relevant: global EMC mode is
**off** in the modern runtime (`enableGlobally()` appears only in legacy
test-support), and GORM 8 is trait-based — `GormEnhancer` does runtime EMC
registration only for Java entities or explicit `dynamicEnhance`.
>
> **Where Grails hurts today is what the scoping *core* fixes:**
>
> - GSP tag dispatch registers tag methods into page/taglib/controller EMCs
on **first use of each tag per class, in production, at request time**
(`TagLibraryMetaUtils.methodMissingForTagLib`, `addMethodsToMetaClass =
!developmentMode`). A warming-up app fires many registry events while serving
traffic — each one a **process-wide** deopt under the current global
SwitchPoint, scoped to one leaf class under this PR.
> - Dev reload (`removeMetaClass` + fresh EMC per changed class, GSP
recompile) and test suites (`mockCodec` per test class, GORM `close()` per
entity) churn metaclasses constantly.
>
> That is exactly the shape of the CI JMH grails-suite result on this PR
(≈5.7× calibrated; `burstThenSteadyState` ≈112×) — and none of it depends on
hierarchy fan-out, which only ever *adds* invalidations.
>
> ## Suggested way forward
>
> - **Keep** the per-MetaClass SwitchPoint scoping core — that's the
measurable win, and Grails is its best witness.
> - **Drop** `ClassHierarchyIndex` and the hierarchy fan-out policy for
stock `MetaClassImpl`/EMC (with the mechanism tests that assert fan-out), per
@blackdrag's argument, which the experiment supports.
> - For unknown/custom MetaClass kinds, stay correctness-first via **bulk**
invalidation on those rare events instead of an indexed subtype walk.
> - Property-site probes analogous to §2 would be worth adding before merge
to close the remaining coverage gap.
>
> That's a strictly smaller machine with, as far as these experiments can
see, identical user-visible semantics — and it keeps the full performance story
for beta-1.
--
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]
