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. -- 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]
