paulk-asert commented on PR #3086:
URL: https://github.com/apache/groovy/pull/3086#issuecomment-6700400031
Thanks @jdaugherty, the explanation of how this differs from GROOVY-12046
and the framework examples settle the "why" for me. I probed the PR (rebased
onto current master) against master. I ran four receiver shapes: a subclass
given the type's EMC (Spock `Mock()`), a subclass answering the registry
metaclass (GORM proxy), a `DelegatingMetaClass` over the EMC, and a plain
instance. Each ran six operations: a method replaced twice, the real method
first then replaced, getter, setter, a property added later, and `+=`. Every
case ran with the cold reflective tier off and on.
**Results**
- The PR fixes all the stale method and `+=` cases for both proxy shapes,
including the "real method first, then replaced" case. The new test doesn't
cover that one, because it installs the replacement before the first call.
- Getters, setters and properties added later were already correct on
master. The `DelegatingMetaClass` and plain controls are correct before and
after.
- For ordinary receivers, `foreignMetaClassSwitchPointFor` returns `null`,
so they get no extra guard. I checked null, `Class`, closures,
`String`/`GString`, collections, arrays, lambdas, scripts, Groovy objects, `as`
proxies, `withTraits`, per-instance EMCs, enums and `BigDecimal`. The extra
guard is confined to the proxy shapes, which makes the change look safe to
backport to 6.0.x.
**One gap: the cold reflective tier**
With `-Dgroovy.indy.cold.reflection=true` (also the default for AOT-linked
sites), "call the real method, then replace it on the EMC" stays stale with the
PR, for both proxy shapes. `ColdReflectiveMethodHandleWrapper.tryBuild` keeps
only `selector.mopSwitchPoint`, so `isValidFor` never sees the new
`metaClassSwitchPoint`. The wrapper should carry and check that switch point
too.
**Suggested additions**
1. The cold-tier fix above.
2. Tests for:
- the real method called first, then replaced, with the cold tier off and
on (the on case needs a forked JVM or a reset of the flag, since it's read once
at class init);
- the registry-answering proxy (the GORM shape) alongside the shared-EMC
one;
- `DelegatingMetaClass` as an unaffected control, as you offered.
3. A small one: in your table comment, the GORM-proxy and
`DelegatingMetaClass` rows are missing cells.
The 6.0.x backport commit is the same patch, and the three files it touches
are identical on both branches, so the cold-tier fix should carry over
unchanged.
On @blackdrag's design point: I agree a metaclass standing in for another
class isn't what `MetaClassImpl` was designed for. But Groovy 5 and classic
call sites both handle these receivers, and the cost here is a second
`SwitchPoint`, only on the shapes that need it. I looked at the alternatives:
- guarding with only the metaclass owner's switch point would be unsafe for
per-instance metaclass changes;
- making these sites uncacheable would slow GORM proxies in production;
- a hierarchy fan-out is what GROOVY-12191 removed.
So this approach looks like the right one to me.
--
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]
