[
https://issues.apache.org/jira/browse/GROOVY-12191?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18099208#comment-18099208
]
ASF GitHub Bot commented on GROOVY-12191:
-----------------------------------------
paulk-asert commented on PR #2736:
URL: https://github.com/apache/groovy/pull/2736#issuecomment-5081857121
AI read:
> ## PR 2736 (GROOVY-12191) readiness assessment
>
> **Bottom line:** The design is sound, the timing (early Groovy 6 cycle) is
exactly right for a change this deep in the dispatch core, and the evidence
base is unusually strong. But it isn't merge-ready today: it's one day old with
zero human reviews, the SonarCloud gate is red (all trivially fixable), and
there are three or four substantive questions — two of them
backward-compatibility related — that deserve an answer on the PR first. I'd
call it "approve direction, request changes on details."
>
> ### What it does
>
> Replaces the single process-wide `SwitchPoint` guarding all indy MOP call
sites with a per-class SwitchPoint domain on `ClassInfo` (new
`org.apache.groovy.runtime.indy` package: `IndyInvalidation` policy +
`SwitchPointInvalidator` lifecycle). MetaClass changes for type T now retire
only T's SwitchPoint plus loaded subtypes/implementors; category enter/leave
and `VMPlugin.invalidateCallSites()` bulk-retire all class domains. Hot call
sites still carry exactly one guard, so steady-state shape is unchanged.
>
> ### Evidence quality — strong
>
> - All CI test jobs green across JDK 17/21/25, three OSes, plus dist builds.
> - CI JMH (calibrated): indy core **3.3×**, grails suite **4.5×**, bench
suite parity; classic runtime parity (as expected — it's untouched).
> - The **"Performance Alert" on the PR is a false alarm**:
`dispatch_8_megamorphic_java` went from 1841 to 2857 ops/ms — that's 1.55×
*faster*; the alert bot applied its smaller-is-better ratio to a throughput
metric. Worth a dismissive comment on the PR so it doesn't spook anyone.
> - Daniel's A/B verification report: ~146× on cross-type invalidation
churn, ~76× on burst-then-steady, steady-state parity, 56/56 tests. Same-type
churn and categories deliberately still pay full re-link cost (correctness
preserved, not "falsely optimized").
> - Test coverage is unusually thorough: hierarchy fan-out, interfaces,
primitives, cleared ClassInfo weak refs, concurrent detach/get races, cold
tier, child-process probes for the frozen log flags.
>
> ### Backward compatibility — the detailed look you asked for
>
> **Binary compatibility: clean.** `IndyInterface.switchPoint` is retained
(adding `volatile` and `@Deprecated` doesn't affect linkage), all `ClassInfo`
additions are additive and `@Internal`-annotated, and the build's compat checks
passed. Non-indy/legacy callsite caching is unaffected because
`ClassInfo.getVersion()` bump semantics are preserved on every path.
>
> **Behavioral compatibility: three real changes, all release-notes
material:**
>
> 1. **`IndyInterface.switchPoint` no longer means what it meant.** It's no
longer the guard on any call site, and — the sharper edge — it is *no longer
rotated on per-class MetaClass changes*, only on category enter/leave and
`invalidateCallSites()`. Any external code (it's `protected`, so subclasses of
`IndyInterface` or anything generating code against it) that installed guards
on this field will now silently miss MetaClass-change invalidations and
dispatch stale. Exposure is probably tiny, but this must go in the Groovy 6
upgrade notes, and the javadoc's claim that "external readers still see an
invalidation" is only true for the category subset of events.
>
> 2. **The legacy field rotation lost its lock.** The old code rotated
`switchPoint` under `synchronized (IndyInterface.class)`; the new code does an
unsynchronized read-replace-invalidate on the volatile. Two concurrent
`invalidateSwitchPoints()` calls can orphan a live SwitchPoint that an external
reader just grabbed — that reader then never observes any future invalidation.
Since the field is being kept *solely* for external observers, I'd restore the
synchronized block; it's a cold path and the fix is two lines. (This is also
effectively what Sonar's S3077 finding is circling.)
>
> 3. **`ClassInfo.incVersion()` (public API) no longer triggers a global
flush.** It used to call `VMPluginFactory.getPlugin().invalidateCallSites()`;
now it scopes to class+subtypes. External MOP tooling that called
`incVersion()` as a "flush everything" hammer gets weaker semantics. Legitimate
design, but it's a documented-behavior change on a public method — upgrade
notes again.
>
> **One genuine correctness hole to raise: array classes.**
`cannotHaveLoadedSubtypes()` treats final classes as fan-out-free, and
`Class.getModifiers()` reports arrays as final — but `Object[]` is
assignable-from every object array type and is the MOP superclass of array
metaclasses. So `registry.setMetaClass(Object[], emc)` (obscure, but legal)
retires only `Object[]`'s domain and leaves a `String[]`-linked site stale,
where the old global SwitchPoint caught it. Needs either a special case
(`type.isArray()` → don't short-circuit) or a test proving it can't matter.
>
> **Scalability trade-off worth acknowledging explicitly:** invalidation
cost went from O(1) (rotate one SwitchPoint) to O(loaded ClassInfos) — every
MetaClass change on a non-final type scans `getAllClassInfo()` with
`isAssignableFrom`, and every category enter/leave detaches every loaded class
domain. Batching via `invalidateAll` keeps it to one safepoint, and the grails
suite shows the trade wins decisively in realistic workloads, but Grails-style
startup (thousands of EMC mutations × tens of thousands of loaded classes) is
now O(N×M) where it was O(N). Fine to accept for alpha; I'd note a follow-up
idea (a subtype index on ClassInfo) in the JIRA rather than block on it.
>
> **Also flagged for reviewer attention:** the `invokeColdReflective`
rewrite is a behavior change beyond pure scoping — it now re-selects via
`fallback(..., false)` and caches the full wrapper into the callsite to prevent
cold-tier recursion. Reasonable, but it subtly changes cold-tier promotion and
deserves its own review eyes (it's also where most of the uncovered lines in
the codecov report live).
>
> ### Housekeeping before merge
>
> - **SonarCloud gate (red):** four findings, all ~10 minutes of work —
empty constructor needs removal or comment (S1186), `@Deprecated` needs
`since`/`forRemoval` (S6355), S3077 on the volatile field (best answered by
restoring the synchronized rotation), S1133 informational.
> - **JIRA has no Fix Version** — should get 6.0.0-beta-1.
> - **No human review yet** — for a change that rewires `ClassInfo` and the
indy guard chain, I'd want at least one other MOP-deep committer (Eric or
Jochen) to sign off, and given it changes the documented semantics of
`IndyInterface.switchPoint`, a short heads-up on dev@ would be in keeping with
how past indy-internals changes were handled.
>
> ### Groovy 6 fit
>
> Right change, right release, right point in the cycle: it's exactly the
kind of behavioral rework that must land in a major, and landing it pre-alpha
maximizes soak time across the ecosystem (Grails, Spock, Geb all lean on
MetaClass churn patterns this directly targets). Nothing here threatens the
Groovy 6 schedule — the open items are days of work, not weeks.
> 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
>
> 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)
