Skip to content

GROOVY-12191: Scope indy SwitchPoint invalidation - #2736

Open
daniellansun wants to merge 4 commits into
masterfrom
GROOVY-12191
Open

GROOVY-12191: Scope indy SwitchPoint invalidation#2736
daniellansun wants to merge 4 commits into
masterfrom
GROOVY-12191

Conversation

@daniellansun

Copy link
Copy Markdown
Contributor

@daniellansun daniellansun changed the title Scope indy SwitchPoint invalidation and harden PIC/math paths GROOVY-12191: Scope indy SwitchPoint invalidation and harden PIC/math paths Jul 25, 2026
@asf-gitbox-commits
asf-gitbox-commits force-pushed the GROOVY-12191 branch 4 times, most recently from 5c5a20a to 793abf8 Compare July 25, 2026 13:17

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.

Benchmark suite Current: 0303da7 Previous: 023b9f5 Ratio
org.apache.groovy.perf.grails.CalibrationBench.allocationChurn 106.35391783145705 us/op 55.452524374328895 us/op 1.92

This comment was automatically generated by workflow using github-action-benchmark.

@codecov-commenter

codecov-commenter commented Jul 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.93774% with 31 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.9053%. Comparing base (c513a3a) to head (0303da7).
⚠️ Report is 17 commits behind head on master.

Files with missing lines Patch % Lines
...g/apache/groovy/runtime/indy/IndyInvalidation.java 87.0968% 8 Missing and 8 partials ⚠️
...org/codehaus/groovy/vmplugin/v8/IndyInterface.java 63.6364% 6 Missing and 2 partials ⚠️
...pache/groovy/runtime/indy/ClassHierarchyIndex.java 93.7500% 1 Missing and 2 partials ⚠️
...java/org/codehaus/groovy/reflection/ClassInfo.java 95.8333% 0 Missing and 1 partial ⚠️
...vmplugin/v8/ColdReflectiveMethodHandleWrapper.java 80.0000% 0 Missing and 1 partial ⚠️
...dehaus/groovy/vmplugin/v8/MethodHandleWrapper.java 66.6667% 1 Missing ⚠️
...java/org/codehaus/groovy/vmplugin/v8/Selector.java 50.0000% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                 @@
##               master      #2736        +/-   ##
==================================================
+ Coverage     69.3657%   69.9053%   +0.5396%     
- Complexity      35082      35240       +158     
==================================================
  Files            1553       1557         +4     
  Lines          131490     130960       -530     
  Branches        24045      23958        -87     
==================================================
+ Hits            91209      91548       +339     
+ Misses          32045      31154       -891     
- Partials         8236       8258        +22     
Files with missing lines Coverage Δ
...he/groovy/runtime/indy/SwitchPointInvalidator.java 100.0000% <100.0000%> (ø)
...odehaus/groovy/vmplugin/v8/IndyCompoundAssign.java 84.0909% <100.0000%> (ø)
...java/org/codehaus/groovy/reflection/ClassInfo.java 92.0792% <95.8333%> (+1.3688%) ⬆️
...vmplugin/v8/ColdReflectiveMethodHandleWrapper.java 92.3077% <80.0000%> (-1.5699%) ⬇️
...dehaus/groovy/vmplugin/v8/MethodHandleWrapper.java 95.0000% <66.6667%> (-5.0000%) ⬇️
...java/org/codehaus/groovy/vmplugin/v8/Selector.java 80.4538% <50.0000%> (ø)
...pache/groovy/runtime/indy/ClassHierarchyIndex.java 93.7500% <93.7500%> (ø)
...org/codehaus/groovy/vmplugin/v8/IndyInterface.java 83.9779% <63.6364%> (-3.4507%) ⬇️
...g/apache/groovy/runtime/indy/IndyInvalidation.java 87.0968% <87.0968%> (ø)

... and 117 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

JMH summary — classic (commit 644913f)

Speedup vs trailing 90-day baseline on gh-pages. Higher = faster.
1.00 = in line with history. Per-benchmark ratio, geomean within group.
Time-per-op units inverted so direction is consistent. The calibrated
column divides out this runner's speed vs the baseline hardware, as
measured by Groovy-independent pure-Java ruler benchmarks.

Group Speedup Calibrated n
bench 1.086 × 1.015 × 92
core 1.044 × 1.071 × 77
grails 0.852 × 0.752 × 80

Runner calibration (this run vs baseline hardware): bench 1.07× (26 rulers) · core-ag 0.99× (3 rulers) · core-hz 0.96× (3 rulers) · grails-ad 1.12× (3 rulers) · grails-ez 1.14× (3 rulers)

Baseline: dev/bench/jmh/<part>/classic/data.js on gh-pages, trailing 90 days. Daily dashboard · Per-suite raw data

@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

JMH summary — indy (commit 644913f)

Speedup vs trailing 90-day baseline on gh-pages. Higher = faster.
1.00 = in line with history. Per-benchmark ratio, geomean within group.
Time-per-op units inverted so direction is consistent. The calibrated
column divides out this runner's speed vs the baseline hardware, as
measured by Groovy-independent pure-Java ruler benchmarks.

Group Speedup Calibrated n
bench 0.972 × 0.965 × 92
core 3.369 × 3.220 × 77
grails 4.437 × 4.073 × 80

Runner calibration (this run vs baseline hardware): bench 1.00× (26 rulers) · core-ag 1.12× (3 rulers) · core-hz 0.97× (3 rulers) · grails-ad 1.07× (3 rulers) · grails-ez 1.10× (3 rulers)

Baseline: dev/bench/jmh/<part>/indy/data.js on gh-pages, trailing 90 days. Daily dashboard · Per-suite raw data

@asf-gitbox-commits
asf-gitbox-commits force-pushed the GROOVY-12191 branch 5 times, most recently from 8e3d5cf to 9f118c0 Compare July 25, 2026 17:14
@daniellansun daniellansun changed the title GROOVY-12191: Scope indy SwitchPoint invalidation and harden PIC/math paths GROOVY-12191: Scope indy SwitchPoint invalidation Jul 25, 2026
@daniellansun

Copy link
Copy Markdown
Contributor Author

GROOVY-12191 Performance Verification Report

Scoped indy SwitchPoint invalidation

Item Value
Issue GROOVY-12191
Commit under test ae8e49741e74067371823148c8993e8c0b73e317
Baseline (parent) 0ca665dad0d4070bfa218d9b5577ec423c8b2181
Date 2026-07-26
Verdict PASS — cross-type MetaClass churn no longer globally deoptimizes hot monomorphic call sites; steady-state hot path is parity; correctness semantics retained

1. Executive summary

This commit changes Groovy’s invokedynamic MOP SwitchPoint invalidation domain from a single process-wide SwitchPoint to a per-class domain attached to ClassInfo (with hierarchy fan-out to loaded subtypes/implementors). Category enter/leave and unscoped MetaClass events still bulk-retire loaded class domains. Linked call sites still carry one SwitchPoint guard on the hot path—the same monomorphic shape as before 6.0.

On the same host, JDK, and JMH annotation settings, A/B measurement shows:

  1. Near-zero regression on steady-state monomorphic hot paths (baseline ratios ≈ 0.99–1.05×, within noise).
  2. Large gains under cross-type MetaClass churn — the primary win:
    • CallSiteInvalidationBench.crossTypeInvalidationEvery1000: 405.6 → 2.77 ms/op (≈ 146×).
    • ScopedInvalidationBench.hotLoop_unrelatedMetaClassChurn: throughput ≈ 156×.
  3. Framework-style “unrelated burst then steady request loop” (burstThenSteadyState): ≈ 76×.
  4. Same-type invalidation and Category semantics are not “falsely optimized”: same-type churn remains far slower than baseline (forced re-link); category enter/leave stays bulk and matches parent throughput.
  5. Correctness: 56/56 tests passed (full scoped-invalidation suite introduced by this commit).

In one line: without adding a second hot-path guard, the change shrinks the blast radius of unrelated MetaClass mutations from process-global to type-scoped, with measured gains on the order of 10¹–10²× (depending on churn frequency and site count).


2. Change mechanism and performance hypotheses

2.1 Previous model (parent)

  • IndyInterface.switchPoint was a process-wide shared SwitchPoint.
  • Any MetaClass registry change → invalidateSwitchPoints()every linked indy site’s guard failed → mass re-link.
  • Typical symptom: a framework/plugin extends MetaClass on ColdType at startup or hot-reload; application hot paths on HotTarget.compute() were deoptimized together with it.

2.2 New model (ae8e497)

Component Role
SwitchPointInvalidator Single-domain lifecycle: CAS get / detachLive / invalidate
IndyInvalidation Policy: invalidateClass (+ hierarchy), invalidateCategory (bulk), guardWithMopSwitchPoints
ClassInfo Holds one indySwitchPoint per class; coordinates version bumps with domain retirement
Selector / IndyCompoundAssign / ColdReflectiveMethodHandleWrapper Install a per-receiver-class SwitchPoint guard at link time

Hot-path shape (intentionally monomorphic):

handle = classSp.guardWithTest(fast, fallback)

Still a single guard—matching the pre-6.0 monomorphic shape. There is no second category SwitchPoint on the hot path.

Invalidation policy:

Event Behavior Expected hot-path cost
Unrelated-type MetaClass change Retire that type’s domain (and loaded subtypes) only Hot sites do not re-link
Same-type / parent MetaClass change Retire class + hierarchy fan-out Must re-link (correctness)
Category enter/leave, invalidateCallSites() Bulk-retire all loaded class domains Same order of cost as old global invalidation
Unattributed MetaClass event invalidateUnscoped() bulk Conservative, correctness-first

2.3 Hypotheses under test

ID Hypothesis How verified
H1 Baseline hot loop has no material regression A/B ratio ≈ 1
H2 Cross-type churn throughput/latency improves substantially Unrelated ≫ parent; on HEAD, unrelated ≫ same-type
H3 Pure hot loop after unrelated burst approaches baseline afterUnrelatedBurstbaseline
H4 Same-type / hierarchy / category still pay re-link tax Clearly slower than baseline
H5 Correctness suite stays green Unit / integration tests

3. Methodology

3.1 Environment

Item Value
OS Linux 6.15.5 x86_64 (hostname: hera)
CPU AMD EPYC 7763 (6 vCPUs visible in container)
Memory 23 GiB
JDK Amazon Corretto 25.0.2 (25.0.2+10-LTS)
Build Gradle wrapper, ./gradlew :perf:jmh, indy enabled by default
Trees HEAD: project worktree; parent: worktree at 0ca665dad0
Bench source parity ScopedInvalidationBench workload identical on parent (Javadoc-only diff)

3.2 Benchmark suites

  1. org.apache.groovy.bench.ScopedInvalidationBench (added by this commit; Throughput, ops/ms)

    • 100 000 monomorphic calls per op; MetaClass write every 1000 iterations in churn cases.
    • JMH: @Warmup(3×2s) @Measurement(5×2s) @Fork(2) → 10 samples per benchmark.
  2. org.apache.groovy.perf.grails.CallSiteInvalidationBench (existing Grails-oriented suite; AverageTime, ms/op)

    • Same 100 000-iteration loops; cross-type / same-type / burst patterns.
    • Same JMH annotation settings.

3.3 Correctness

./gradlew :test --tests Groovy12191 \
  --tests org.apache.groovy.runtime.indy.IndyInvalidationTest \
  --tests org.apache.groovy.runtime.indy.SwitchPointInvalidatorTest \
  --tests org.codehaus.groovy.vmplugin.v8.IndyScopedSwitchPointTest --rerun-tasks

Result: 56 passed / 0 failed / 0 error.

Test class Cases
bugs.Groovy12191 8
IndyInvalidationTest 27
SwitchPointInvalidatorTest 12
IndyScopedSwitchPointTest 9

3.4 Statistics and artifacts

  • Scores are JMH means; ± is JMH’s default 99.9% CI.
  • Throughput ratio = HEAD / parent (>1 is better).
  • AverageTime speedup = parent / HEAD (>1 is better).
  • Builds ran sequentially to avoid dual-JVM core contention; no CPU pinning. Conclusions rest on order-of-magnitude and structural separation, not ±1% micro-deltas.

Raw JSON (local run artifacts, not committed):

  • /tmp/groovy-12191-perf-results/head/scoped-results.json
  • /tmp/groovy-12191-perf-results/parent/scoped-results.json
  • /tmp/groovy-12191-perf-results/head/callsite-results.json
  • /tmp/groovy-12191-perf-results/parent/callsite-results.json

4. Results

4.1 ScopedInvalidationBench (Throughput, ops/ms — higher is better)

Benchmark Parent HEAD HEAD/Parent Interpretation
hotLoop_baseline 3.891 ± 0.224 3.869 ± 0.219 0.99× H1: no hot-path regression
hotLoop_afterUnrelatedBurst 3.774 ± 0.201 4.522 ± 0.168 1.20× H3: full speed after unrelated burst
hotLoop_unrelatedMetaClassChurn 0.00247 ± 0.00017 0.386 ± 0.078 ≈156× H2 primary win
hotLoop_sameTypeMetaClassChurn 0.00249 ± 0.00017 0.0188 ± 0.0012 ≈7.6× Still forced re-link; narrower blast radius than parent
hotLoop_categoryEnterLeave 0.00225 ± 0.00045 0.00231 ± 0.00028 ≈1.03× H4: bulk semantics retained
hierarchy_baseline 3.325 ± 0.118 2.896 ± 0.113 0.87× Noise / load variance; both full-speed order
hierarchy_parentMetaClassChurn 0.00233 ± 0.00021 0.00957 ± 0.00080 ≈4.1× H4: parent change still invalidates child sites

Structural fingerprint on HEAD only:

baseline        ≈ 3.87 ops/ms
unrelated churn ≈ 0.39 ops/ms   (~20× slower than baseline: mostly MetaClass write cost)
same-type churn ≈ 0.019 ops/ms  (~200× slower than baseline: write + same-class re-link)
category        ≈ 0.002 ops/ms  (bulk re-link tax)

On parent, unrelated and same-type both collapse to ~0.002, showing that the old global SwitchPoint made “unrelated churn” equivalent to “global deopt.” HEAD separates them by ~20× (0.386 / 0.019)—a direct fingerprint that scoping is live.

4.2 CallSiteInvalidationBench (AverageTime, ms/op — lower is better)

Benchmark Parent HEAD Speedup (P/H) Interpretation
baselineHotLoop 0.257 ± 0.017 0.248 ± 0.014 1.04× H1
baselineListSize 0.242 ± 0.010 0.236 ± 0.006 1.03× H1
baselineSteadyStateNoBurst 0.612 ± 0.029 0.583 ± 0.033 1.05× H1
baselineMultipleCallSites 20.42 ± 1.03 18.86 ± 0.54 1.08× H1
crossTypeInvalidationEvery10000 288.5 ± 24.3 2.73 ± 1.20 ≈106× H2
crossTypeInvalidationEvery1000 405.6 ± 27.3 2.77 ± 0.88 ≈146× H2 primary win
crossTypeInvalidationEvery100 137.4 ± 10.2 12.5 ± 1.0 ≈11× Still large under denser churn
listSizeWithCrossTypeInvalidation 330.7 ± 25.0 2.21 ± 0.53 ≈150× JDK-type hot sites benefit too
multipleCallSitesWithInvalidation 1098 ± 89 25.9 ± 1.7 ≈42× Multi-site still much improved
burstThenSteadyState 145.8 ± 30.6 1.93 ± 0.16 ≈76× “Framework MC extend + request steady state”
sameTypeInvalidationEvery1000 407.5 ± 29.4 53.3 ± 3.2 ≈7.6× Same-type still slow; local invalidation cheaper than global rotate

Penalty vs baseline on HEAD:

Scenario Relative cost Meaning
crossType @1000 2.77 / 0.25 ≈ 11× baselineHotLoop Dominated by ~100 ColdType MetaClass writes, not hot-site deopt
sameType @1000 53.3 / 0.25 ≈ 215× baselineHotLoop Clear re-link tax
burstThenSteady 1.93 / 0.58 ≈ 3.3× steady baseline Burst write cost dominates; steady calls recovered

On parent, crossType@1000 and sameType@1000 are both ~406 ms/op, again confirming global invalidation collapsed cross-type into full deopt.


5. Why it is faster

                    Old path                              New path
  ColdType.metaClass.foo = ...           ColdType.metaClass.foo = ...
            │                                      │
            ▼                                      ▼
   invalidate global SwitchPoint        invalidate ClassInfo(ColdType) SP
            │                                      │
            ▼                                      ▼
  ALL sites: guard fails                ColdType sites only
  HotTarget.compute → full re-select    HotTarget.compute → stays linked
  (MH rebuild / re-guard / cold path)   (single SwitchPoint check passes)

Cost breakdown:

  1. Avoided re-links — After each global invalidation, hot sites re-run Selector, reinstall guards, and rewrite CallSites. At 100 churn events × many sites this dominates (parent’s 400+ ms/op).
  2. Avoided JIT churn — Frequent SwitchPoint.invalidateAll breaks monomorphic inlining assumptions and amplifies deopt/recompile cost (especially under dense cross-type churn).
  3. Unchanged hot-path guard count — Still one guardWithTest, so baselines stay flat (H1).
  4. Same-type / hierarchy still correctly charged — Prevents “fast but wrong” semantics (H4 + 56 tests).
  5. Category remains bulk — No second hot-path guard; category visibility stays simple and correct; cost matches the old global path (measured ~1.0×).

Same-type still improves ~7–8× on HEAD vs parent, not because re-link is skipped, but because the invalidation surface shrinks from “all process sites” to “this class hierarchy” and batch invalidateAll is cheaper—a secondary benefit, not the primary goal.


6. Risks and boundaries

Topic Assessment
Hot-path regression Not observed (baselines 0.99–1.08×)
Semantic regression 56/56 tests for this commit; Category / hierarchy / per-instance MC covered
Dense Category enter/leave Still expensive (by design); Category-heavy workloads do not get faster
Non-final parent MetaClass change Hierarchy scan over ClassInfo.getAllClassInfo() is O(loaded classes); cold path, correctness-first
Legacy IndyInterface.switchPoint Rotated for binary compatibility only; no longer the real call-site MOP guard; external readers should migrate to IndyInvalidation
Environment noise Shared/container CPUs; conclusions use order-of-magnitude gaps (10¹–10²×), robust to ±10% noise
Not covered here Concurrent multi-thread churn throughput, full Grails end-to-end, JDK 17/21 matrix (recommended for CI)

7. Hypothesis scorecard

Hypothesis Result Evidence
H1 baseline parity Holds Scoped baseline 0.99×; CallSite baselines 1.03–1.08×
H2 cross-type speedup Holds ≈156× thrpt; ≈146× avgt (@1000)
H3 post-unrelated-burst full speed Holds afterUnrelatedBurst ≥ baseline on HEAD
H4 necessary invalidation still paid Holds same-type / hierarchy / category ≪ baseline
H5 correctness Holds 56/56 tests

8. Conclusions and recommendations

Verdict

ae8e497 (GROOVY-12191) keeps a single-guard monomorphic hot path while scoping indy MOP invalidation to per-class domains (+ hierarchy). For unrelated-type MetaClass churn with hot monomorphic call sites—a Grails/framework-shaped load—measured improvement is on the order of 10¹–10²×. Steady-state undisturbed paths show no material regression. Correctness costs for Category and same-type changes remain visible and intentional.

Performance verification: PASS.

Recommendations

  1. Keep ScopedInvalidationBench and the cross-type / burst rows of CallSiteInvalidationBench in regular JMH regression (README already documents the suite) so a return to a global SwitchPoint cannot land silently.
  2. In release notes, state clearly that Category and unscoped MetaClass events may still bulk-invalidate; the win is type-local MetaClass change.
  3. Optionally re-run CallSite crossType@1000 and baselines on JDK 17/21 to confirm cross-LTS consistency.
  4. Document migration away from reading IndyInterface.switchPoint (field is @Deprecated).

Appendix A — Reproduction commands

# Correctness
./gradlew :test --tests Groovy12191 \
  --tests org.apache.groovy.runtime.indy.IndyInvalidationTest \
  --tests org.apache.groovy.runtime.indy.SwitchPointInvalidatorTest \
  --tests org.codehaus.groovy.vmplugin.v8.IndyScopedSwitchPointTest

# Performance (HEAD)
./gradlew :perf:jmh -PbenchInclude=ScopedInvalidation -PjmhResultFormat=JSON
./gradlew :perf:jmh -PbenchInclude=CallSiteInvalidation -PjmhResultFormat=JSON

# Performance (parent @ 0ca665dad0; mount the same ScopedInvalidationBench sources for a fair A/B)
cd /path/to/parent && ./gradlew :perf:jmh -PbenchInclude=ScopedInvalidation -PjmhResultFormat=JSON
cd /path/to/parent && ./gradlew :perf:jmh -PbenchInclude=CallSiteInvalidation -PjmhResultFormat=JSON

Appendix B — Files touched by the commit

  • New: org.apache.groovy.runtime.indy.IndyInvalidation, SwitchPointInvalidator
  • Updated: ClassInfo, IndyInterface, Selector, ColdReflectiveMethodHandleWrapper, IndyCompoundAssign
  • Tests: Groovy12191, IndyInvalidationTest, SwitchPointInvalidatorTest, IndyScopedSwitchPointTest
  • Benchmarks: ScopedInvalidationBench; docs: subprojects/performance/README.adoc

Report generated from local JMH A/B measurements on 2026-07-26. Raw JSON and run logs: /tmp/groovy-12191-perf-results/.

@paulk-asert

Copy link
Copy Markdown
Contributor

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.

@daniellansun

Copy link
Copy Markdown
Contributor Author

@paulk-asert

Thanks for your review.

Point Action
Array fan-out hole (Object[] final short-circuit) Fixed via ClassHierarchyIndex (full JLS array lattice) + single invalidate path; tests for Object[] / interface arrays / multi-dim
Legacy switchPoint lost lock Restored synchronized (IndyInterface.class)
switchPoint semantics / javadoc over-claim Docs: bulk-only rotation; misses per-class MC; @Deprecated(since="6.0.0", forRemoval=false)
incVersion() no longer global flush Documented + test; bulk callers → invalidateCallSites() / invalidateCategory()
Scalability O(N×M) Shipped weak ancestor→descendant index; typed MC invalidation is O(subtypes). Category still full walk (cheap no-op detach)
Sonar S1186 / S6355 / S3077 Empty-ctor comment; deprecate metadata; sync rotation
invokeColdReflective Documented as intentional cold-tier change (not pure scoping)
Perf Alert bot False alarm: 1841→2857 ops/ms is faster (throughput misread as latency)

Happy to adjust if anything still looks off.

@daniellansun

Copy link
Copy Markdown
Contributor Author

GROOVY-12191 Performance Verification Report V2

Scoped indy SwitchPoint invalidation (re-verification after review follow-ups)

Item Value
Issue GROOVY-12191
Commit under test f48ab19256f6ae9888dd48bb34cd770dd6a05958
Baseline (parent) 0ca665dad0d4070bfa218d9b5577ec423c8b2181
Date 2026-07-26
Verdict PASS — cross-type MetaClass churn no longer globally deoptimizes hot monomorphic call sites; steady-state hot path is parity; hierarchy fan-out is indexed; correctness suite green (76/76)

1. Executive summary

This verification covers the full GROOVY-12191 stack on the current tip of PR #2736:

  1. ae8e497 — Replace the process-wide MOP SwitchPoint with a per-class domain on ClassInfo (plus hierarchy fan-out). Category enter/leave and unscoped MetaClass events still bulk-retire loaded class domains. Linked call sites still carry one SwitchPoint guard on the hot path.
  2. f48ab19 — Address review: restore synchronized legacy switchPoint rotation; document incVersion() / field semantics; fix array-class fan-out via a weak ancestor→descendant index (ClassHierarchyIndex); typed MetaClass invalidation becomes O(|subtypes of T|) instead of scanning every loaded ClassInfo.

On the same host, JDK, and JMH annotation settings, A/B measurement of HEAD (f48ab19) vs baseline (0ca665da) shows:

  1. Near-zero regression on steady-state monomorphic hot paths (baseline ratios ≈ 0.96–1.10×, within noise).
  2. Large gains under cross-type MetaClass churn — the primary win:
    • CallSiteInvalidationBench.crossTypeInvalidationEvery1000: 428.4 → 2.25 ms/op (≈ 190×).
    • ScopedInvalidationBench.hotLoop_unrelatedMetaClassChurn: throughput ≈ 197×.
  3. Framework-style “unrelated burst then steady request loop” (burstThenSteadyState): ≈ 114×.
  4. Same-type invalidation and Category semantics are not “falsely optimized”: same-type churn remains far slower than baseline (forced re-link); category enter/leave stays bulk and matches parent throughput (~1×).
  5. Correctness: 76/76 tests passed (full scoped-invalidation + hierarchy-index suite).

In one line: without adding a second hot-path guard, the change shrinks the blast radius of unrelated MetaClass mutations from process-global to type-scoped (with correct hierarchy/array fan-out), with measured gains on the order of 10¹–10²× (depending on churn frequency and site count).


2. Change mechanism and performance hypotheses

2.1 Previous model (baseline 0ca665da)

  • IndyInterface.switchPoint was a process-wide shared SwitchPoint.
  • Any MetaClass registry change → invalidateSwitchPoints()every linked indy site’s guard failed → mass re-link.
  • Typical symptom: a framework/plugin extends MetaClass on ColdType at startup or hot-reload; application hot paths on HotTarget.compute() were deoptimized together with it.
// baseline (simplified)
protected static SwitchPoint switchPoint = new SwitchPoint();
// MetaClassRegistry listener → always:
synchronized (IndyInterface.class) {
    SwitchPoint old = switchPoint;
    switchPoint = new SwitchPoint();
    SwitchPoint.invalidateAll(new SwitchPoint[]{old});
}

2.2 New model (ae8e497 + f48ab19)

Component Role
SwitchPointInvalidator Single-domain lifecycle: CAS get / detachLive / invalidate
IndyInvalidation Policy: invalidateClass (+ hierarchy), invalidateCategory (bulk), guardWithMopSwitchPoints
ClassHierarchyIndex Weak ancestor→descendant index; O(|subtypes|) fan-out incl. JLS array lattice
ClassInfo Holds one indySwitchPoint per class; registers into the index; coordinates version bumps
Selector / IndyCompoundAssign / ColdReflectiveMethodHandleWrapper Install a per-receiver-class SwitchPoint guard at link time

Hot-path shape (intentionally monomorphic):

handle = classSp.guardWithTest(fast, fallback)

Still a single guard—matching the pre-6.0 monomorphic shape. There is no second category SwitchPoint on the hot path.

Invalidation policy:

Event Behavior Expected hot-path cost
Unrelated-type MetaClass change Retire that type’s domain (and indexed subtypes) only Hot sites do not re-link
Same-type / parent MetaClass change Retire class + hierarchy fan-out Must re-link (correctness)
Category enter/leave, invalidateCallSites() Bulk-retire all loaded class domains Same order of cost as old global invalidation
Unattributed MetaClass event invalidateUnscoped() bulk Conservative, correctness-first
Array root (Object[], interface arrays) Fan-out via full JLS array covariance lattice in the index Correct subtype retirement (review fix)

What f48ab19 specifically adds beyond ae8e497:

Review item Resolution in f48ab19
Array fan-out hole (Object[] treated as final leaf) ClassHierarchyIndex indexes full array lattice
Legacy switchPoint lost lock Restored synchronized (IndyInterface.class) on bulk rotation
switchPoint semantics over-claim Docs: bulk-only rotation; @Deprecated(since="6.0.0", forRemoval=false)
incVersion() no longer global flush Documented; bulk callers → invalidateCallSites() / invalidateCategory()
Scalability O(N×M) per typed MC change Weak subtype index → O(|subtypes of T|); category still full walk

2.3 Hypotheses under test

ID Hypothesis How verified
H1 Baseline hot loop has no material regression A/B ratio ≈ 1
H2 Cross-type churn throughput/latency improves substantially Unrelated ≫ parent; on HEAD, unrelated ≫ same-type
H3 Pure hot loop after unrelated burst approaches baseline afterUnrelatedBurstbaseline
H4 Same-type / hierarchy / category still pay re-link tax Clearly slower than baseline
H5 Correctness suite stays green (incl. index / array cases) Unit / integration tests

3. Methodology

3.1 Environment

Item Value
OS Linux 6.15.5 x86_64 (hostname: hera)
CPU AMD EPYC 7763 (6 vCPUs visible)
Memory 23 GiB
JDK Amazon Corretto 25.0.2 (25.0.2+10-LTS)
Build Gradle wrapper, ./gradlew :perf:jmh, indy enabled by default
Trees HEAD: project worktree at f48ab19; baseline: worktree at 0ca665da (/tmp/groovy-12191-parent)
Bench source parity ScopedInvalidationBench + CallSiteInvalidationBench workloads identical on parent (sources copied into parent worktree for fair A/B)
Run order Sequential (HEAD then parent per suite) to avoid dual-JVM core contention; no CPU pinning

3.2 Benchmark suites

  1. org.apache.groovy.bench.ScopedInvalidationBench (added by ae8e497; Throughput, ops/ms)

    • 100 000 monomorphic calls per op; MetaClass write every 1000 iterations in churn cases.
    • JMH: @Warmup(3×2s) @Measurement(5×2s) @Fork(2) → 10 samples per benchmark.
  2. org.apache.groovy.perf.grails.CallSiteInvalidationBench (existing Grails-oriented suite; AverageTime, ms/op)

    • Same 100 000-iteration loops; cross-type / same-type / burst patterns.
    • Same JMH annotation settings.

3.3 Correctness

./gradlew :test --tests Groovy12191 \
  --tests org.apache.groovy.runtime.indy.IndyInvalidationTest \
  --tests org.apache.groovy.runtime.indy.SwitchPointInvalidatorTest \
  --tests org.apache.groovy.runtime.indy.ClassHierarchyIndexTest \
  --tests org.codehaus.groovy.vmplugin.v8.IndyScopedSwitchPointTest --rerun-tasks

Result: 76 passed / 0 failed / 0 skipped / 0 error.

Test class Cases (approx.) Notes
bugs.Groovy12191 10 End-to-end MOP / category / incVersion scoping
IndyInvalidationTest 34 Policy, bulk, unscoped, hierarchy, concurrency probes
SwitchPointInvalidatorTest 12 CAS lifecycle, concurrent detach/get races
ClassHierarchyIndexTest 9 Class/interface/array lattice indexing (f48ab19)
IndyScopedSwitchPointTest 11 Guard install, cold tier, logging child process

3.4 Statistics and artifacts

  • Scores are JMH means; ± is JMH’s default 99.9% CI.
  • Throughput ratio = HEAD / parent (>1 is better).
  • AverageTime speedup = parent / HEAD (>1 is better).
  • Conclusions rest on order-of-magnitude and structural separation, not ±1% micro-deltas.

Raw JSON (local run artifacts, not committed):

  • /tmp/groovy-12191-perf-v2/head/scoped-results.json
  • /tmp/groovy-12191-perf-v2/parent/scoped-results.json
  • /tmp/groovy-12191-perf-v2/head/callsite-results.json
  • /tmp/groovy-12191-perf-v2/parent/callsite-results.json
  • /tmp/groovy-12191-perf-v2/summary.json

4. Results

4.1 ScopedInvalidationBench (Throughput, ops/ms — higher is better)

Benchmark Parent HEAD HEAD/Parent Interpretation
hotLoop_baseline 3.931 ± 0.207 4.068 ± 0.113 1.03× H1: no hot-path regression
hotLoop_afterUnrelatedBurst 3.889 ± 0.242 4.360 ± 0.289 1.12× H3: full speed after unrelated burst
hotLoop_unrelatedMetaClassChurn 0.002278 ± 0.000316 0.4486 ± 0.124 ≈197× H2 primary win
hotLoop_sameTypeMetaClassChurn 0.002327 ± 0.000251 0.01892 ± 0.00132 ≈8.1× Still forced re-link; narrower blast radius than parent
hotLoop_categoryEnterLeave 0.002350 ± 0.000136 0.002194 ± 0.000421 ≈0.93× H4: bulk semantics retained (parity within noise)
hierarchy_baseline 3.295 ± 0.142 2.791 ± 0.117 0.85× Both full-speed order; container noise
hierarchy_parentMetaClassChurn 0.002352 ± 0.000169 0.01021 ± 0.000709 ≈4.3× H4: parent change still invalidates child sites

Structural fingerprint on HEAD only:

baseline        ≈ 4.07 ops/ms
after burst     ≈ 4.36 ops/ms   (≥ baseline: no residual deopt)
unrelated churn ≈ 0.45 ops/ms   (~9× slower than baseline: mostly MetaClass write cost)
same-type churn ≈ 0.019 ops/ms  (~215× slower than baseline: write + same-class re-link)
category        ≈ 0.002 ops/ms  (bulk re-link tax)

On parent, unrelated and same-type both collapse to ~0.002, showing that the old global SwitchPoint made “unrelated churn” equivalent to “global deopt.” HEAD separates them by ~24× (0.449 / 0.019)—a direct fingerprint that scoping is live.

4.2 CallSiteInvalidationBench (AverageTime, ms/op — lower is better)

Benchmark Parent HEAD Speedup (P/H) Interpretation
baselineHotLoop 0.264 ± 0.021 0.252 ± 0.015 1.05× H1
baselineListSize 0.241 ± 0.004 0.250 ± 0.018 0.96× H1 (noise)
baselineSteadyStateNoBurst 0.608 ± 0.021 0.586 ± 0.020 1.04× H1
baselineMultipleCallSites 20.82 ± 1.56 19.01 ± 1.09 1.10× H1
crossTypeInvalidationEvery10000 306.1 ± 33.1 2.503 ± 0.653 ≈122× H2
crossTypeInvalidationEvery1000 428.4 ± 49.0 2.250 ± 0.894 ≈190× H2 primary win
crossTypeInvalidationEvery100 139.2 ± 12.8 7.916 ± 0.701 ≈18× Still large under denser churn
listSizeWithCrossTypeInvalidation 323.4 ± 56.4 1.750 ± 0.394 ≈185× JDK-type hot sites benefit too
multipleCallSitesWithInvalidation 1103 ± 56.2 25.68 ± 5.97 ≈43× Multi-site still much improved
burstThenSteadyState 154.7 ± 23.0 1.361 ± 0.112 ≈114× “Framework MC extend + request steady state”
sameTypeInvalidationEvery1000 410.6 ± 23.5 51.45 ± 2.06 ≈8.0× Same-type still slow; local invalidation cheaper than global rotate

Penalty vs baseline on HEAD:

Scenario Relative cost Meaning
crossType @1000 2.25 / 0.25 ≈ baselineHotLoop Dominated by ~100 ColdType MetaClass writes, not hot-site deopt
sameType @1000 51.5 / 0.25 ≈ 205× baselineHotLoop Clear re-link tax
burstThenSteady 1.36 / 0.59 ≈ 2.3× steady baseline Burst write cost dominates; steady calls recovered

On parent, crossType@1000 and sameType@1000 are both ~410–428 ms/op, again confirming global invalidation collapsed cross-type into full deopt.

4.3 Comparison to the earlier ae8e497-only report

An earlier local report (posted on PR #2736 for ae8e497 alone) measured ~146× on crossType@1000 and ~156× on scoped unrelated thrpt. This re-run on f48ab19 measures ~190× / ~197× on the same primary rows. Differences of tens of percent on multi-hundred-× ratios are expected on a 6-vCPU shared host; the structural story is unchanged:

  • baselines stay ~1×,
  • cross-type improves by 10¹–10²×,
  • same-type / category remain correctly expensive,
  • HEAD separates unrelated from same-type by ~20× where parent could not.

f48ab19 is primarily a correctness / scalability / review-hygiene commit (index, array lattice, lock restore, docs). It is not expected to move monomorphic hot-path baselines; the A/B above confirms that.


5. Why it is faster

                    Old path                              New path
  ColdType.metaClass.foo = ...           ColdType.metaClass.foo = ...
            │                                      │
            ▼                                      ▼
   invalidate global SwitchPoint        invalidate ClassInfo(ColdType) SP
            │                           (+ ClassHierarchyIndex descendants)
            ▼                                      ▼
  ALL sites: guard fails                ColdType (+ subtypes) sites only
  HotTarget.compute → full re-select    HotTarget.compute → stays linked
  (MH rebuild / re-guard / cold path)   (single SwitchPoint check passes)

Cost breakdown:

  1. Avoided re-links — After each global invalidation, hot sites re-run Selector, reinstall guards, and rewrite CallSites. At 100 churn events × many sites this dominates (parent’s 400+ ms/op).
  2. Avoided JIT churn — Frequent SwitchPoint.invalidateAll breaks monomorphic inlining assumptions and amplifies deopt/recompile cost (especially under dense cross-type churn).
  3. Unchanged hot-path guard count — Still one guardWithTest, so baselines stay flat (H1).
  4. Same-type / hierarchy still correctly charged — Prevents “fast but wrong” semantics (H4 + 76 tests).
  5. Category remains bulk — No second hot-path guard; category visibility stays simple and correct; cost matches the old global path (measured ~1×).
  6. Indexed fan-out (f48ab19) — Typed MetaClass invalidation walks only registered descendants, not every loaded class. Correctness for Object[] / interface arrays / multi-dim arrays is restored without reintroducing a global SP.

Same-type still improves ~ on HEAD vs parent, not because re-link is skipped, but because the invalidation surface shrinks from “all process sites” to “this class hierarchy” and batch invalidateAll is cheaper—a secondary benefit, not the primary goal.


6. Risks and boundaries

Topic Assessment
Hot-path regression Not observed (baselines 0.96–1.10×)
Semantic regression 76/76 tests for this tip; Category / hierarchy / per-instance MC / array lattice covered
Dense Category enter/leave Still expensive (by design); Category-heavy workloads do not get faster
Hierarchy baseline 0.85× Same full-speed band as parent; treat as noise on shared 6-vCPU host, not a design regression
Non-final parent MetaClass change Fan-out is O(|indexed subtypes|) after f48ab19 (was full ClassInfo walk in ae8e497)
Category bulk Still walks all loaded ClassInfos; detach is a cheap no-op when no SP was allocated
Legacy IndyInterface.switchPoint Rotated for binary compatibility (bulk paths only, under lock again); no longer the real call-site MOP guard
Environment noise Shared/container CPUs; conclusions use order-of-magnitude gaps (10¹–10²×), robust to ±10% noise
Not covered here Concurrent multi-thread churn throughput, full Grails end-to-end, JDK 17/21 matrix (recommended for CI)

7. Hypothesis scorecard

Hypothesis Result Evidence
H1 baseline parity Holds Scoped baseline 1.03×; CallSite baselines 0.96–1.10×
H2 cross-type speedup Holds ≈197× thrpt; ≈190× avgt (@1000)
H3 post-unrelated-burst full speed Holds afterUnrelatedBurst ≥ baseline on HEAD
H4 necessary invalidation still paid Holds same-type / hierarchy / category ≪ baseline
H5 correctness Holds 76/76 tests (incl. ClassHierarchyIndexTest)

8. Conclusions and recommendations

Verdict

ae8e497 + f48ab19 (GROOVY-12191, PR #2736) keep a single-guard monomorphic hot path while scoping indy MOP invalidation to per-class domains (+ indexed hierarchy / array lattice). For unrelated-type MetaClass churn with hot monomorphic call sites—a Grails/framework-shaped load—measured improvement is on the order of 10¹–10²× (primary rows ≈ 190–197× on this host). Steady-state undisturbed paths show no material regression. Correctness costs for Category and same-type changes remain visible and intentional. Review follow-ups (lock restore, docs, array fan-out, subtype index) do not undo the win.

Performance verification: PASS.

Recommendations

  1. Keep ScopedInvalidationBench and the cross-type / burst rows of CallSiteInvalidationBench in regular JMH regression (README already documents the suite) so a return to a global SwitchPoint cannot land silently.
  2. In release notes, state clearly that Category and unscoped MetaClass events may still bulk-invalidate; the win is type-local MetaClass change.
  3. Document migration away from reading IndyInterface.switchPoint (field is @Deprecated; bulk-only rotation).
  4. Optionally re-run CallSite crossType@1000 and baselines on JDK 17/21 to confirm cross-LTS consistency.
  5. CI “Performance Alert” on throughput benches that report higher ops/ms as “worse” remains a bot false-positive class; prefer calibrated geomean summaries for PR triage.

Appendix A — Reproduction commands

# Correctness
./gradlew :test --tests Groovy12191 \
  --tests org.apache.groovy.runtime.indy.IndyInvalidationTest \
  --tests org.apache.groovy.runtime.indy.SwitchPointInvalidatorTest \
  --tests org.apache.groovy.runtime.indy.ClassHierarchyIndexTest \
  --tests org.codehaus.groovy.vmplugin.v8.IndyScopedSwitchPointTest

# Performance (HEAD @ f48ab19)
./gradlew :perf:jmh -PbenchInclude=ScopedInvalidation -PjmhResultFormat=JSON
./gradlew :perf:jmh -PbenchInclude=CallSiteInvalidation -PjmhResultFormat=JSON

# Performance (parent @ 0ca665da; same ScopedInvalidationBench + CallSiteInvalidationBench sources)
cd /path/to/parent && ./gradlew :perf:jmh -PbenchInclude=ScopedInvalidation -PjmhResultFormat=JSON
cd /path/to/parent && ./gradlew :perf:jmh -PbenchInclude=CallSiteInvalidation -PjmhResultFormat=JSON

Appendix B — Files touched (combined)

ae8e497 — scoping core

  • New: org.apache.groovy.runtime.indy.IndyInvalidation, SwitchPointInvalidator, package-info
  • Updated: ClassInfo, IndyInterface, Selector, ColdReflectiveMethodHandleWrapper, IndyCompoundAssign
  • Tests: Groovy12191, IndyInvalidationTest, SwitchPointInvalidatorTest, IndyScopedSwitchPointTest
  • Benchmarks: ScopedInvalidationBench; docs: subprojects/performance/README.adoc

f48ab19 — review follow-ups + index

  • New: ClassHierarchyIndex, ClassHierarchyIndexTest
  • Updated: IndyInvalidation (index-backed fan-out), ClassInfo (register), IndyInterface (sync + deprecation metadata), docs/tests for array / incVersion / bulk paths

*Report generated from local JMH A/B measurements on 2026-07-26 (re-run on tip f48ab19 vs baseline 0ca665da).

@blackdrag blackdrag left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one point that escapes me right now. Why do we need the hierarchy at all? I don´t think this is for MetaClassImpl cases.

*/
protected static SwitchPoint switchPoint = new SwitchPoint();
@Deprecated(since = "6.0.0", forRemoval = false)
protected static volatile SwitchPoint switchPoint = new SwitchPoint();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all under vmplugin is supposed to be Internal, thus removing instead of deprecating is imho very much allowed. Also why do you make it volatile if you deprecate it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — thank you.

org.codehaus.groovy.vmplugin.* is internal-by-intent. Keeping a dead process-wide SwitchPoint only for binary compatibility was weaker than a clean removal: after scoping, the field was no longer the MOP guard and was rotated only on bulk paths, which was easy to misread.

Action taken

  • Removed protected static SwitchPoint switchPoint entirely.
  • invalidateSwitchPoints() now only bulk-retires per-class domains via IndyInvalidation.invalidateCategory() (no legacy field rotation, no volatile, no extra lock).
  • Docs/package-info updated; tests no longer assert on the removed field.

The earlier volatile was only there to make concurrent bulk rotation of that legacy field observable; with the field gone, that concern disappears.

*/
static MethodHandle applyMopSwitchPoints(final MethodHandle handle, final MethodHandle fallback, final Object receiver) {
return IndyInvalidation.guardWithMopSwitchPoints(handle, fallback, receiver);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the rationale for having the guard creation in IndyInvalidation instead of here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question — the split is intentional, and the previous one-line delegate made it look accidental.

Rationale

Concern Owner
Domain resolution (receiver → class → ClassInfo SwitchPoint) IndyInvalidation (also used by ClassInfo, cold tier, tests)
Invalidation policy (class / hierarchy / category / unscoped) IndyInvalidation
Link-time guardWithTest install on production call sites IndyInterface.applyMopSwitchPoints (Selector, IndyCompoundAssign)

Putting invalidation policy next to bootstrap wiring would grow IndyInterface further and pull ClassInfo toward vmplugin. Putting only the bind step in IndyInterface keeps the bytecode surface as the place that wires handles, while the runtime.indy package owns domains and retire rules.

Action taken

  • applyMopSwitchPoints now performs the bind itself:

    IndyInvalidation.classSwitchPointFor(receiver).guardWithTest(handle, fallback)

  • IndyInvalidation.guardWithMopSwitchPoints remains the public / test entry (same semantics), documented as such.

  • Javadoc on both sides spells out the division of responsibility.

MethodHandleWrapper full = fallback(cold.callSite, cold.sender, cold.methodName, cold.callID,
cold.safeNavigation, cold.thisCall, cold.spreadCall, 1, arguments, false);
cold.callSite.put(receiverCacheKey(arguments[0]),
full.isCanSetTarget() ? full : NULL_METHOD_HANDLE_WRAPPER);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is NULL_METHOD_HANDLE_WRAPPER the fallback for if cannot set target? It may be because of the code path to here, but frankly I think this needs at least a comment as of why this is the only valid version.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that this needed an explicit comment — the policy is shared with fromCacheHandle / selectMethodHandle, not cold-tier-specific invention.

Why the sentinel is the only safe PIC value when !canSetTarget

  1. canSetTarget == false means the selection is not class-keyed-cacheable (typical causes: per-instance MetaClass, spread-call → selector.cache == false).
  2. Storing that real wrapper under the receiver-class PIC key would pin a selection that is only valid for one instance or one spread shape, and the next hit would skip re-selection.
  3. NULL_METHOD_HANDLE_WRAPPER is the existing PIC sentinel meaning “do not cache this receiver shape; re-run fallback on the next hit” (see fromCacheHandle).
  4. The current invocation still uses full.getCachedMethodHandle() — the sentinel only shapes the PIC, not the return of this call.

Action taken

  • Expanded the inline comment at the cold-path PIC write to state the above.
  • Added a unit test that asserts the uncacheable → sentinel PIC write policy.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that was part of the old source, but because of its use in this context I would propose renaming NULL_METHOD_HANDLE_WRAPPER.

…tion

Remove the legacy process-wide IndyInterface.switchPoint (vmplugin is
internal-by-intent), bind MOP guards at link time in applyMopSwitchPoints,
document hierarchy fan-out for cross-class MOP visibility, and clarify the
cold-path PIC sentinel policy. Extend unit coverage for these paths.
@daniellansun

Copy link
Copy Markdown
Contributor Author

There is one point that escapes me right now. Why do we need the hierarchy at all? I don´t think this is for MetaClassImpl cases.

You are right that this is not because MetaClassImpl shares one method table up the hierarchy. Each class still has its own MetaClass / ClassInfo domain.

Hierarchy fan-out exists for cross-class MOP visibility that already-linked subtype sites must re-observe:

  1. Parent ExpandoMetaClass / registry mutation
    Parent.metaClass.foo = { … } must force sites linked for Child to re-select so new Child().foo() sees the method. Without retiring Child’s SwitchPoint, a previously linked miss (or older target) stays installed.

  2. Interface MetaClass changes
    Same rule for implementors.

  3. Array covariance
    Object[] / interface arrays are MOP-relevant supertypes of reference arrays even though array classes are final — a pure “exact class only” domain would leave String[] sites stale after Object[] MetaClass changes.

So: fan-out is about inherited / covariant MOP state, not about rewriting MetaClassImpl internals. Category enter/leave remains a separate bulk path and does not rely on this index for semantics.

Action taken

  • Documented this rationale on IndyInvalidation, ClassHierarchyIndex, and package-info.
  • Kept / extended tests:
    • parent EMC → child SwitchPoint + runtime visibility
    • parent MetaClassImpl replacement still fans out (registry change can still affect subtype dispatch)
    • unrelated-type churn does not touch an unrelated hierarchy
    • array / interface-array lattice cases (from earlier review)

Happy to discuss a narrower fan-out (e.g. EMC-only) as a follow-up optimization; the current rule prefers correctness and matches pre-6.0 “something in the MOP above me changed → re-link” behaviour, but scoped to the subtype cone instead of the whole process.

@daniellansun
daniellansun requested a review from blackdrag July 27, 2026 13:08
@blackdrag

Copy link
Copy Markdown
Contributor

There is one point that escapes me right now. Why do we need the hierarchy at all? I don´t think this is for MetaClassImpl cases.

You are right that this is not because MetaClassImpl shares one method table up the hierarchy. Each class still has its own MetaClass / ClassInfo domain.

Hierarchy fan-out exists for cross-class MOP visibility that already-linked subtype sites must re-observe:

1. **Parent ExpandoMetaClass / registry mutation**
   `Parent.metaClass.foo = { … }` must force sites linked for `Child` to re-select so `new Child().foo()` sees the method. Without retiring Child’s SwitchPoint, a previously linked miss (or older target) stays installed.

2. **Interface MetaClass changes**
   Same rule for implementors.

3. **Array covariance**
   `Object[]` / interface arrays are MOP-relevant supertypes of reference arrays even though array classes are `final` — a pure “exact class only” domain would leave `String[]` sites stale after `Object[]` MetaClass changes.

This all sounds all like there is some problem deeper down.
I would have to recheck the code, but why do we have to do the hierarchy if no ExpandoMetaClass is in use. And even if we use a single EMC I question if that is needed. I think this is only the case if EMC is enabled globally. Regarding the general argumentation I think the meta class should provide the guard or switchpoint, since this is logic specific to an potentially unknown meta class. And I think the array case shows this even more. Who says that if the MetaClass of Object[] changes, the callsite on a receiver String[] has become invalid? And if we really have a hierarchy of meta classes where when I add a method to the parent, I have it in the child, then this might not be a replacement of the meta-class, but an update. This is what the version was for in the past, especially to also cover the per-instance meta-class cases. Most likely this can be done by the switchpoints as well, but then you have them per instance, not per class. Also... Assume the following case in pseudo code:

  class A extends B
  class B extends C { def fooC() }
  def call(C x){ x.fooC() }
  def c = new C()
  call(c)
  B.metaClass = new EMC
  B.metaClass.fooB0 = {}
  call(c)
  B.metaClass = null
  call(c)
  enableEMCglobally()
  call(c)
  c.metaClass = null
  call(c)
  def metaClassB = B.metaClass
  B.metaClass = new MetaClassImpl()
  call(c)
  metaClassB.fooB1 = {...}
  call(c)  
  A.fooA = {}
  call(c)
  def b = new B()
  b.metaClass = new EMC
  b.metaClass.fooB2 = {}
  call(c)

Technically none of these operations would have to invalidate the callsite, since fooC is unchanged, but of course that is not how it works in reality. But the question is when do we invalidate and why. For me this is not yet the stage to propose a follow-up, first I would like to better understand the old and new behavior and think about if the difference matters. Also of interest is when and if the those foo methods becomes visible on the instance c as well as the metaClass of C.

…name

Narrow hierarchy invalidation to an allow-list of cross-class MOP cases
(EMC, modified custom MetaClass, interface/array, global EMC) while pure
MetaClassImpl replaces and per-instance MetaClass stay exact-class. Rename
NULL_METHOD_HANDLE_WRAPPER to UNCACHEABLE_PIC_SENTINEL and document the
invalidation matrix. Extend unit coverage for blackdrag's scenario and
the refined policy.
@daniellansun

Copy link
Copy Markdown
Contributor Author

@blackdrag Thank you for pressing on the “when / why” of hierarchy fan-out. That is the right question, and the previous reply was still too coarse. Below is the refined model after re-checking MOP selection against your scenario.

1. When we invalidate (and when we do not)

Monomorphic indy sites guard a class-domain SwitchPoint on the receiver’s runtime class (via ClassInfo). They re-link only when that domain is retired.

For your ladder A extends B extends C and call(C x) { x.fooC() } with c = new C():

Operation Retires C’s domain? foo* visible on c / C.metaClass?
B.metaClass = EMC + fooB0 No (C is a supertype of B) No
B.metaClass = null No No
enableEMCglobally() Only if a bulk path fires; not via typed B change Global EMC changes creation, not C’s linked target by itself
c.metaClass = null Per-instance path → exact C only if a class-level SP was tied to that path Restores class-level MC for c
B.metaClass = new MetaClassImpl() No for pure MetaClassImpl (exact B only) No
Detached EMC fooB1 after replace Depends on whether that EMC is still the registered MC No on c
A.fooA = {} No for C No on c
Per-instance EMC on b + fooB2 No for C No on c

So for call(c) / fooC, the new scoped model already keeps C’s site linked across the operations that only touch B/A. That is the main behavioural difference vs the old process-wide SwitchPoint (which retired everything).

Regression coverage for this matrix is in Groovy12191.blackdragScenario_callOnC_notInvalidatedBySubclassMetaClassChurn.

2. Why hierarchy exists at all (and when it does not)

Hierarchy fan-out is notMetaClassImpl shares one table up the hierarchy”. Each class still has its own MetaClass / ClassInfo domain.

It exists because selection can observe ancestor MetaClass state:

  • MetaClassImpl.findMethodInClassHierarchy only opens a real walk when some strong MetaClass in the hierarchy is a modified MutableMetaClass (EMC is the common case).
  • Empirically, Parent.metaClass.hello = {…} is visible on new Child().hello() without enableGlobally().
  • Empirically, Object[].metaClass.arrHello = {…} is visible on String[] — that is why the array lattice is indexed.

So hierarchy is required for cross-class MOP visibility, not for ordinary MetaClassImpl method tables.

Refined policy (exact-class is an allow-list)

Event Width
EMC install / remove / in-place update (incVersion) class + loaded subtypes
Global EMC creation handle active class + hierarchy
Interface or array MetaClass change class + hierarchy (array lattice)
Modified custom MetaClassImpl subclass (isModified()==true) class + hierarchy
Pure unmodified MetaClassImplMetaClassImpl (or null) exact class only
Per-instance MetaClass change exact class only
Category enter/leave, unscoped registry event all loaded class domains

This matches your intuition: without EMC (and without a modified mutable MetaClass), hierarchy is unnecessary. Pure MetaClassImpl parent replace no longer fans out to children. Unknown/custom MetaClass kinds still fan out (correctness-first).

3. “Should the MetaClass own the SwitchPoint?”

Agreed in principle. The current class-domain SP is a stand-in for class-level MetaClass generation, not a claim that every MetaClass kind shares the same invalidation logic.

A MetaClass-owned guard would be the natural place for:

  • MetaClass-specific update vs replace semantics,
  • true per-instance domains (today: uncacheable PIC + exact class retire),
  • custom MetaClass implementations that do not fit EMC / MetaClassImpl.

That is a larger redesign than this PR. For 6.0 we kept a single monomorphic hot-path guard and made invalidation MetaClass-aware at the registry boundary. MetaClass-owned SwitchPoints remain a follow-up once the “when / why” matrix above is agreed.

4. Update vs replace, and version

You are right that parent method addition is often an update of the same MetaClass, not a registry replace. That path is ClassInfo.incVersion() (from EMC), which still does hierarchy fan-out — because subtype sites may have linked a miss (or an older target) and must re-select to see the new expando method.

Per-instance MetaClass continues to use the existing uncacheable PIC path (canSetTarget == false); we do not introduce per-instance SwitchPoints in this PR.

5. PIC sentinel rename

Per discussion_r3664025383:

  • NULL_METHOD_HANDLE_WRAPPERUNCACHEABLE_PIC_SENTINEL
  • meaning: “do not store this receiver shape under a class-keyed PIC entry”
  • deprecated alias kept as getNullMethodHandleWrapper() for in-tree callers during the rename

6. What we are not claiming

  • We do not claim minimal invalidation at method-name granularity (fooC unchanged ⇒ never re-link). The monomorphic SwitchPoint still retires the whole class domain when that class’s MOP generation changes — same shape as pre-6.0, but scoped.
  • We do not claim MetaClass-owned SwitchPoints are done; only that policy is now MetaClass-aware on top of class domains.

Happy to adjust further if you want the exact-only allow-list tightened or loosened (e.g. EMC-only fan-out even for interfaces/arrays without EMC).

@sonarqubecloud

Copy link
Copy Markdown

@blackdrag

Copy link
Copy Markdown
Contributor

@blackdrag Thank you for pressing on the “when / why” of hierarchy fan-out. That is the right question, and the previous reply was still too coarse. Below is the refined model after re-checking MOP selection against your scenario.

Now I feel kind of bad, because I made a major mistake in my example. not
A extends B extends C and call(C x) { x.fooC() } with c = new C(): In my head the hierarchy was reversed. The goal was specifically to see how changes in the metaclasses of super classes affect the child metaclass. Sorry for that.

[...]

2. Why hierarchy exists at all (and when it does not)

Hierarchy fan-out is notMetaClassImpl shares one table up the hierarchy”. Each class still has its own MetaClass / ClassInfo domain.

It exists because selection can observe ancestor MetaClass state:

* `MetaClassImpl.findMethodInClassHierarchy` only opens a real walk when some strong MetaClass in the hierarchy is a **modified** `MutableMetaClass` (EMC is the common case).

* Empirically, `Parent.metaClass.hello = {…}` is visible on `new Child().hello()` **without** `enableGlobally()`.

* Empirically, `Object[].metaClass.arrHello = {…}` is visible on `String[]` — that is why the array lattice is indexed.

but the important point here is that this happens only for a missing method. And it is actually why I think we are having a major semantic gap here, that we have to decide on:

class A{}
class B extends A{
  def m1(){1}
  def m2(Integer x){2}
}
class C extends A {
  def m2(x) { 40 }
}
def c = new C()

// normal calls
def b = new B()
assert b.m1() == 1
assert b.m2(0) == 2

// methods added using EMC, including overload
A.metaClass.m1 = {-> -1}
A.metaClass.m2 = {Integer x-> -2}
A.metaClass.m2 = {String x-> -3}
def a = new A()
assert a.m1() == -1
assert a.m2(0) == -2
assert a.m2("") == -3

// setting metaclass again, but no change
b.metaClass = null
assert b.m1() == 1
assert b.m2(0) == 2

// this method did not exist before, now found by
// MetaClassImpl.findMethodinClassHierarchy
// BUT no valid callsite for this before did exist -> no invalidation case
assert b.m2("") == -3

// even though m2(Integer) and m2 (String) exist in the metaclass for A
// they are not used for C, because the metaclass for C existed before
// they have been added!
assert c.m2(null) == 40
assert c.m2(0) == 40
assert c.m2("") == 40

// reset meta class, still the same, because MetaClassImpl is cached
c = new C()
assert c.m2(null) == 40
assert c.m2(0) == 40
assert c.m2("") == 40

// is now EMC
c.metaClass.m1 = {-> -40}
assert c.m1() == -40
assert c.m2(null) == 40
assert c.m2(0) == -2
assert c.m2("") == -3

// same structure as C, but instance exists *after* A has been modified!
// D is still MetaClassImpl
class D extends A {
  def m2(x) { 50 }
}
d = new D()
assert d.m2(null) == 50
assert d.m2(0) == -2
assert d.m2("") == -3

The case of C and D show how broken the current system is. The point of time a MetaClassImpl instance comes into existence should not decide about what it sees. In fact my POV is that MetaClassImpl should be immune to changes of other meta classes. This means especially that no hierarchy check is needed for it and that find method should not exist on MetaClassImpl. Now to produce a version with callsite caching:

import org.codehaus.groovy.runtime.metaclass.*
class A{}
class C extends A {
  def m2(x) { 40 }
}
def call(x,y) {
  x.m2(y)
}
def c = new C()

A.metaClass.m2 = {Integer x-> -2}
A.metaClass.m2 = {String x-> -3}

// no methods from A visible
assert call(c,0) == 40
c = new C()
assert call(c,0) == 40

def mc = C.metaClass.delegate
assert mc instanceof MetaClassImpl

// is now EMC enforces methods from A
c.metaClass.m1 = {-> -40}
assert call(c,0) == -2

// reset to old MC
c.metaClass = mc
assert call(c,0) == 40

// simulate late init, still MetaClassImpl, but now change is visible
def registry = MetaClassRegistryImpl.getInstance(0)
registry.removeMetaClass(C)
c.metaClass = null
assert call(c,0) == -2
def newMc = c.metaClass.delegate
assert newMc instanceof MetaClassImpl
assert newMc != mc

// reset meta class of A, but old methods from A still visible
A.metaClass = null
assert call(c,0) == -2

// show that A does not know m2
def catched = false
try {
  new A().m2(0)
} catch (Exception e) {
  catched = true
}
assert catched

// now we switch to the old emc, which is clueless about A
c.metaClass = emc
call(c,0) == 40

Most of this should not be caught by the switch point, but by the fact, that the metaclass changed. But I also think I captured cases where a hierarchy version would break the code in case of MetaClassImpl. Also I think it should behave the same if classes used are Java based and not from Groovy.

[...]

This matches your intuition: without EMC (and without a modified mutable MetaClass), hierarchy is unnecessary. Pure MetaClassImpl parent replace no longer fans out to children. Unknown/custom MetaClass kinds still fan out (correctness-first).

+1

3. “Should the MetaClass own the SwitchPoint?”

Agreed in principle. The current class-domain SP is a stand-in for class-level MetaClass generation, not a claim that every MetaClass kind shares the same invalidation logic.

A MetaClass-owned guard would be the natural place for:

* MetaClass-specific update vs replace semantics,
* true per-instance domains (today: uncacheable PIC + exact class retire),
* custom MetaClass implementations that do not fit EMC / `MetaClassImpl`.

That is a larger redesign than this PR. For 6.0 we kept a single monomorphic hot-path guard and made invalidation MetaClass-aware at the registry boundary. MetaClass-owned SwitchPoints remain a follow-up once the “when / why” matrix above is agreed.

ok, agreed... if a follow-up issue is created

@testlens-app

testlens-app Bot commented Jul 28, 2026

Copy link
Copy Markdown

✅ All tests passed ✅

🏷️ Commit: 0303da7
▶️ Tests: 107498 executed
⚪️ Checks: 31/31 completed


Learn more about TestLens at testlens.app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants