[
https://issues.apache.org/jira/browse/GROOVY-12191?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18100139#comment-18100139
]
ASF GitHub Bot commented on GROOVY-12191:
-----------------------------------------
daniellansun commented on PR #2736:
URL: https://github.com/apache/groovy/pull/2736#issuecomment-5120228130
@blackdrag Thank you for the correction and for the detailed parent→child
matrix — that is exactly the right level of precision.
## 1. Hierarchy direction
Understood: the scenario of interest is **superclass MetaClass changes
observed from a child receiver**, not the reverse. Sorry the earlier matrix
inverted that ladder. The tests now follow your corrected direction (`A` parent
→ `B`/`C`/`D` children).
## 2. Missing-method-only walk (and the C/D gap)
Agreed on the important refinement:
- `MetaClassImpl.findMethodInClassHierarchy` is a **missing-method** path.
It only opens a real walk when some strong MetaClass in the hierarchy is a
**modified** `MutableMetaClass` (EMC is the usual case).
- A method that already exists on the child keeps winning for applicable
arguments after hierarchy re-link; SwitchPoint retirement does **not** rebuild
MetaClass tables.
- The **C vs D** divergence (same structure, different MetaClass
construction time relative to parent EMC) is pre-existing MOP behaviour:
construction-time snapshot / inheritance of ancestor expando methods into a new
`MetaClassImpl`, not something class-domain SwitchPoints can honestly “fix”.
**6.0 decision for this PR:** preserve current MOP visibility rules; scope
invalidation to match those rules. Making `MetaClassImpl` immune to other
MetaClasses (and removing the hierarchy walk from it) is a separate semantic
redesign — see follow-ups below.
Documented on `IndyInvalidation`, `ClassHierarchyIndex`, `package-info`, and
`ClassInfo.incVersion`.
## 3. Callsite caching vs hierarchy version
Agreed that **most of your second script is driven by MetaClass change on
the receiver**, not by a hierarchy version:
| Step | What retires the site | Runtime |
|------|----------------------|---------|
| `C` becomes EMC | exact-class domain (receiver MC changed) | `call(c,0) →
-2` |
| restore class-level `MetaClassImpl` | exact-class | `→ 40` |
| `removeMetaClass(C)` + fresh `MetaClassImpl` while `A` has EMC |
exact-class | `→ -2` (construction snapshot) |
| `A.metaClass = null` | hierarchy fan-out (parent EMC remove) | still `→
-2` (snapshot retained) |
Hierarchy SwitchPoint is only there so **already-linked miss** sites on
subtypes re-select when an ancestor becomes / stops being a modified mutable
MetaClass (e.g. linked miss → parent adds method; linked hit → parent EMC
removed).
Measured note on the last line of your draft (`c.metaClass = emc` after `A`
is cleared): the draft expected a “clueless” EMC → `40`, but the EMC created
while `A` still had methods **retains** `ClosureMetaMethod`s for `m2(Integer)`
/ `m2(String)`, so resolution stays `-2`. Same retention idea as the
`MetaClassImpl` snapshot — not live super lookup after `A` is gone. Captured in
`Groovy12191.blackdragScenario_callsiteCaching_metaClassChangeNotHierarchyVersion`.
Pure Java inheritance matches the miss path (`LinkedHashMap` / `HashMap`).
## 4. Policy (your +1)
Unchanged and reaffirmed:
- pure unmodified `MetaClassImpl` ↔ `MetaClassImpl` (or null) → **exact
class only**
- EMC install / remove / in-place update, global EMC, interface / array →
**class + hierarchy**
- per-instance MetaClass → **exact class only**
- category / unscoped → bulk
## 5. MetaClass-owned SwitchPoint — follow-up
Agreed, and we will file follow-up JIRAs (not landing in this PR):
1. **MetaClass-owned SwitchPoints** for indy MOP invalidation (natural home
for MC-specific update/replace, true per-instance domains, custom MetaClass
kinds).
2. **`MetaClassImpl` hierarchy immunity** — decide whether `MetaClassImpl`
should stop observing other MetaClasses (construction-time snapshot +
`findMethodInClassHierarchy`), so C/D timing is no longer load-order dependent.
I will link the issue keys in `IndyInvalidation` / `package-info` once they
exist.
## 6. Regression coverage added
- `blackdragScenario_missingMethodOnly_andMetaClassImplTiming`
- `blackdragScenario_callsiteCaching_metaClassChangeNotHierarchyVersion`
- `parentEmcAfterLinkedMiss_childCallSiteSeesNewMethod`
- `parentEmcRemoveAfterLinkedHit_childCallSiteMissesAgain`
- `parentEmc_doesNotReplacePresentChildMethod_afterHierarchyRelink`
- `javaSubtype_seesParentEmc_andHierarchyFanOut`
Happy to adjust further if any row of the matrix still disagrees with the
behaviour you want for 6.0.
> Scope indy SwitchPoint invalidation
> -----------------------------------
>
> Key: GROOVY-12191
> URL: https://issues.apache.org/jira/browse/GROOVY-12191
> Project: Groovy
> Issue Type: Improvement
> Reporter: Daniel Sun
> Priority: Major
> Fix For: 6.0.0-beta-1
>
>
> h3. Problem
> With invokedynamic enabled (default since Groovy 4), linked MOP call sites
> were guarded by a *single process-wide* {{SwitchPoint}}
> ({{{}IndyInterface.switchPoint{}}}).
> Any MetaClass registry change or category enter/leave invalidated that switch
> point, so *every* linked site fell back and re-linked — including sites whose
> receiver type was unrelated.
> That global invalidation is expensive when MetaClass churn is common (e.g.
> ExpandoMetaClass / mixins on startup or per-request paths, Grails-like
> patterns). Unrelated hot monomorphic sites pay re-link and JIT deopt cost
> they should not.
> h3. Goal
> Keep linked call sites optimized unless the *relevant* MetaClass state for
> that site actually changed.
> h3. Approach
> One SwitchPoint domain {*}per class{*}, stored on {{{}ClassInfo{}}}:
> * MetaClass change for type {{T}} retires {{{}T{}}}'s SwitchPoint *and*
> those of loaded subtypes / implementors (hierarchy fan-out).
> * Unrelated types keep their SwitchPoints; their call sites stay optimized.
> * Category enter/leave (and {{{}VMPlugin.invalidateCallSites(){}}})
> bulk-retire *all* loaded class SwitchPoints so sites re-link under the new
> category state. There is *no* second category SwitchPoint on the hot path.
> * Linked handles always install a *single* class-domain guard via
> {{IndyInvalidation.guardWithMopSwitchPoints(...)}} — same monomorphic guard
> shape as before, without global deopt on unrelated MetaClass churn.
> Final classes short-circuit hierarchy fan-out (no full {{ClassInfo}} scan).
> Non-final types batch retirements with {{{}SwitchPoint.invalidateAll{}}}.
> h3. Invalidation map
> ||Event||What is retired||
> |MetaClass change for type {{T}} (registry /
> {{{}ClassInfo.incVersion{}}})|{{T}} + loaded subtypes / implementors|
> |Category enter/leave, {{invalidateCallSites()}}|All loaded class
> SwitchPoints (bulk)|
> |Unattributed MetaClass registry event|All loaded class SwitchPoints (bulk)|
> |First MetaClass *install* on a class|Version bump only (no linked sites
> yet); replacement / clear retires that class's SwitchPoint|
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
