GROOVY-12259: Make category/bulk call-site invalidation O(live SwitchPoint domains) instead of O(loaded classes) (includes GROOVY-12258)

[paulk-asert]

No description provided.

[blackdrag]

Does this replace #2786 ?

[daniellansun]

Summary

The three commits form a coherent progression:

1. Skip the all-ClassInfo walk when this process has never allocated a SwitchPoint.
2. Replace that walk with a process-wide registry of live SwitchPoints, so category enter/leave is O(live domains).
3. Stop that registry from retaining entries after a domain is discarded without an explicit detach.

I think the destination is the right one. The part I would like to discuss is commit 3’s orphan-reaper protocol: it is correct as far as I can follow, but it turns SwitchPointInvalidator from a small lazy cell into a process-wide GC protocol, and it pumps that protocol from getSwitchPoint(), which is the MOP link path. I wonder whether the same leak- and staleness-freedom can be obtained by giving ClassInfo a class-level domain that outlives the MetaClass object — analogous to the pending domain it already keeps — and leaving the live set as the straightforward map introduced in commit 2.

None of this is a claim that the present code is wrong. It is a suggestion that a different ownership boundary might delete a whole layer of mechanism.

---

What I think is working well

• Register-before-publish. An empty registry observation really does mean there was nothing a bulk path needed to retire. That is a clean replacement for the monotonic flag in 5013c5f.
• Keying the registry by SwitchPoint, not by invalidator. A single-use SwitchPoint cannot ABA-clobber a successor the way an invalidator-keyed remove could. The comment that explains this is worth keeping, wherever the map ends up.
• Two-argument remove for a single claimant. Drain and reaper cannot both retire the same orphan. That handshake is easy to follow.
• Replacing isAnySwitchPointAllocated with hasLiveSwitchPoints(). The flag never re-armed, so a process that linked once would pay the walk forever. The registry check is the better model.
• The tests around concurrent get / detach / drain (registryInvariant_underConcurrentGetDetachAndDrain, the CAS-loss leak bound) give a reader something concrete to trust.

---

1. Would it be possible to keep domain lifetime on ClassInfo?

SwitchPointInvalidator previously did one job: allocate a SwitchPoint lazily, detach it, invalidate it. After commit 3 it also owns:

• a process-wide ConcurrentHashMap<SwitchPoint, OwnerRef>
• a ReferenceQueue and a WeakReference subclass
• opportunistic reaping
• bulk drain, including a second “owner already dead” mode
• a test hook (clearOwnerRefForTesting) that deliberately breaks the weak-ref invariant

The comments have to restate the same JVM fact several times — guardWithTest keeps only the SwitchPoint’s internal invoker, not the SwitchPoint object — which I took as a sign that the lifetime model is no longer obvious from the types.

The two production owners are not actually the same kind of thing:

Owner	What is collected	What I believe we need
ClassInfo.pendingIndySwitchPoint	a discarded script Class / ClassInfo	the call sites die with the class; the registry entry should not linger
IndyInvalidation.DOMAINS	a soft/weak MetaClass collected while the Class is still live	exact-class invalidation must still be able to find that domain

Commit 3 treats both as “owner died, so invalidate the orphan from a global queue.” That is a conservative and understandable choice. I am not sure it is the smallest one, and I am not sure it fully closes the second case.

Exact-class retirement still goes through ClassInfo:

// IndyInvalidation
public static void collectLiveForClass(final Class<?> type, final List<SwitchPoint> out) {
    ClassInfo.getClassInfo(type).collectLiveIndySwitchPoints(out);
}

// ClassInfo
public void collectLiveIndySwitchPoints(final List<SwitchPoint> out) {
    IndyInvalidation.collectLiveForMetaClass(getMetaClassForClass(), out);
    SwitchPoint pending = pendingIndySwitchPoint.detachLive();
    if (pending != null) {
        out.add(pending);
    }
}

After a soft MetaClass is collected, getMetaClassForClass() returns null and hasClassLevelMetaClass() is false, so the next install is treated as a first install. invalidateClass, incVersion, and a stock registry replace then have nothing to detach. GroovyObject sites typically pin the MetaClass via SAME_MC.bindTo(mc); an optimised POJO handle often does not. Those POJO sites can remain on a still-valid SwitchPoint that exact-class invalidation can no longer see.

The reaper runs only from getSwitchPoint() and drainLive(). A warmed-up process whose sites are already linked, whose MetaClass has been softly collected, and which is not using categories, calls neither. That is close to the “latent staleness window” described in the commit 3 message. The protocol will close the window once something else links or a category use runs; it will not close it from invalidateClass itself.

A direction you have already used, and that I would be grateful if you would consider extending:

• ClassInfo already keeps pendingIndySwitchPoint for the pre-MC generation.
• A class-level invalidator that outlives the MetaClass object the same way would let invalidateClass find the domain after a soft collection.
• “Weak MC gone, installing a new one” could be treated as replace, not as first install.
• The live set could remain the commit 2 map (SwitchPoint → SwitchPointInvalidator), without weak values.
• Discarded-script cleanup could live on ClassInfo collection. finalizeReference() already exists, though as far as I can see it is not invoked from ClassValue / globalClassSet. Wiring that, or accepting that a dead class’s pending entry sits until the next bulk drain, would avoid a third lifetime world on the cell.

If that is workable, OwnerRef, ORPHANS, reapOrphans(), the drain-time orphan branch, and clearOwnerRefForTesting could all go away, and exact-class invalidation would see the domain again. If it is not workable — for example if a class-level handle would pin something you have been careful not to pin — I would very much like to understand that constraint. I may simply have the reachability wrong.

---

2. getSwitchPoint() as the reaper pump

public SwitchPoint getSwitchPoint() {
    // Allocation is the operation churn-heavy processes keep performing,
    // so it doubles as the reaper pump; a no-op while the queue is empty.
    reapOrphans();
    for (;;) {
        SwitchPoint sp = current.get();
        if (sp != null) {
            return sp;

The comment describes allocation as the pump. The poll happens before the live-current hit, so every MOP link (classSwitchPointFor → getSwitchPoint()) pays a ReferenceQueue.poll(). That call is inexpensive when the queue is empty, but it is still a process-wide synchronised check. After a category use block — the path this series is making cheaper — every site re-links, and every re-link takes that lock. If the queue is not empty, the link also waits on one-at-a-time invalidateAlls.

I realise an empty poll is cheap, and that you called this out in the commit message (“Stable processes pay … an empty queue poll on the link path”). My hesitation is only whether that cost belongs on the link path at all, given that the series is otherwise moving work off the category / re-link path.

drainLive then does some of the same work twice, in two different styles:

static void drainLive(final List<SwitchPoint> out) {
    reapOrphans();
    LIVE.forEach((sp, ref) -> {
        SwitchPointInvalidator inv = ref.get();
        if (inv == null) {
            if (LIVE.remove(sp, ref)) {
                out.add(sp);
            }
        } else if (inv.detachIfCurrent(sp)) {
            out.add(sp);
        }
    });
}

reapOrphans() invalidates immediately. The forEach already claims ref.get() == null into out so that IndyInvalidation.retireAllLoadedDomains can use a single invalidateAll. Reaping first turns orphans that could have been batched into single invalidations, then hides them from the batch. After script churn plus use, the category path therefore does the slower thing first.

It also splits the contract of drainLive: some SwitchPoints are invalidated inside the method, others are only detached into out. retireAllLoadedDomains still reads as “drain, then invalidateBatch,” which is no longer the whole story.

If the orphan protocol remains, two modest adjustments would already make the control flow easier to follow:

1. Do not call reapOrphans() from drainLive(). The forEach is enough, and it keeps one invalidateAll.
2. Do not poll on the getSwitchPoint() hit path. If a pump is required when there is no drain, java.lang.ref.Cleaner registered at domain creation would at least keep it off the link path.

---

3. Small leftovers from the walk that the registry replaced

These are minor and only worth a pass if you are already editing the comments.

• retireAllLoadedDomains no longer walks loaded domains. A name such as retireLiveDomains would match what the method now does.
• ClassInfo.detachLiveIndySwitchPoint still says to prefer collectLiveIndySwitchPoints for bulk paths. Bulk paths now go through SwitchPointInvalidator.drainLive.
• hasLiveSwitchPoints() before drainLive() is a reasonable GROOVY-12258 fast path; an empty drainLive is already cheap. I do not feel strongly about keeping or folding it.

The public isAnySwitchPointAllocated() from 5013c5f is already gone in f549626, which seems right for an unreleased 6.0 surface.

---

4. Tests, if you pursue the lifetime change

The new unit tests (drain_claimsOrphanWhoseOwnerWasCollected, reaper_invalidatesOrphanedSwitchPointAfterOwnerGc) pin down the map / queue handshake clearly. If the design stays as it is, they are the right tests.

If you are willing to consider moving lifetime back onto ClassInfo, the behaviours I would most want a later reader to see are:

• After a soft MetaClass collection, invalidateClass / incVersion still retires a still-installed guard.
• A discarded script class does not leave hasLiveSwitchPoints() true indefinitely.
• A use block after script churn batch-invalidates; it does not single-invalidate via reapOrphans().

A smaller observation on the current tests, offered only as a consistency note:

• reaper_invalidatesOrphanedSwitchPointAfterOwnerGc waits on System.gc() for up to two seconds. That is reasonable if the design is “GC, then a later get.” A ClassInfo-owned domain would not need that test.
• drain_claimsOrphanWhoseOwnerWasCollected ends with inv.detachLive() because the owner is still alive and current is stale. getSwitchPoint() will return that already-invalidated SwitchPoint (current != null is sufficient). Production avoids that only because a true orphan’s owner is dead. The test hook therefore encodes “the owner is dead,” not “current is a live registered SwitchPoint.” That may be acceptable; I mention it only because it is a slightly fragile invariant for a later editor.

---

A possible shape, if you find it useful

I would personally be happy to see:

1. Commit 2’s registry, keyed by SwitchPoint, register-before-publish, deregister on detach — that is the GROOVY-12259 fix.
2. A class-level domain handle on ClassInfo (or a ManagedReference finalize on the weak MetaClass) so exact-class invalidation survives MetaClass collection.
3. Explicit ClassInfo cleanup for discarded scripts, so the live set cannot grow without bound.
4. No OwnerRef / ReferenceQueue / link-path pump.

File size is not a concern (SwitchPointInvalidator is 345 lines). The question is only cohesion: three retirement modes (owner detach, drain detach, reaper invalidate) versus one live set plus domain ownership on ClassInfo.

I may have under-estimated a pinning or AOT constraint that makes the class-level handle unattractive. If so, I would be glad to be corrected. Thank you for the careful write-up in the commit 3 message — it made the intended invariant much easier to review.

[paulk-asert]

Thanks Jochen and Daniel, the PR has been revised. AI description:

Thanks for this review — it's the most useful kind: a concrete alternative with a falsifiable claim. Rather than argue it in the abstract I implemented your shape, and it worked out well enough that it's now the PR itself: I've force-pushed this PR as a single squashed commit containing your design. The flag → registry → reaper progression you reviewed is collapsed — commits 1 and 3 no longer exist in the final tree, so keeping them seemed like history for its own sake (the previous head 3010843 is still reachable if you want to diff the two designs). Point-by-point below.

1. ClassInfo-owned domains — workable, and better

No hidden pinning or AOT constraint turned up. The implementation goes slightly further than you sketched: rather than a class-level handle alongside the MetaClass identity map, there is now one domain per class, owned by ClassInfo (the former pending field, renamed), covering the pre-MC link window and every installed MetaClass generation. DOMAINS, domainKey, domainFor and collectLiveForMetaClass are gone; switchPointForMetaClass resolves via getTheClass(). Your "weak MC gone → treat install as replace" falls out for free: install, replace, clear and per-instance changes all retire the same domain, and retiring an unallocated generation is a no-op — so the first-install special case (and hasClassLevelMetaClass) disappeared too.

Your staleness scenario is confirmed and now pinned by a deterministic test (invalidateClass_reachesDomainAfterMetaClassCollected): weak MC cleared, getMetaClassForClass() null, invalidateClass still retires the installed guard. Under the reaper design that guard was only reachable opportunistically; here it's reachable at the invalidation site, which is where it matters.

The registry keeps the shape from the middle commit you reviewed — strong values, single-mode drainLive, no OwnerRef/queue/reaper/pump/test hook. The invariants you called out as working (register-before-publish, SwitchPoint keying, two-arg remove) all survive unchanged.

Two things I learned on the way, one of which slightly reframes the problem:

• finalizeReference() is indeed dead wiring on the ClassValue path — GlobalClassSet's queue elements never call it, as you suspected. So discarded-class cleanup did need something: on first link the domain lazily anchors a ManagedReference<ClassInfo> (weak bundle) whose finalizeReference retires the domain — delivered by the existing shared ReferenceManager, pumped by managed-reference creation across the whole runtime rather than by my getSwitchPoint(). Reachability chain: registry → invalidator → anchor → (weak) ClassInfo, so cleanup is reachable exactly while there's something to clean, and the chain self-collects after firing. Classes that never link pay nothing.
• The retention we were both worried about is soft-bounded either way. A dropped script class is normally still softly reachable via CachedClass, so neither the reaper nor the anchor fires until soft refs clear under memory pressure — my GC test needed a pressure stage to pass. And GroovyClassLoader.close() already retires domains deterministically via removeClass, no GC involved. So the anchor is a backstop for undisciplined loader drops, not the primary cleanup path — which I think supports your instinct that this didn't deserve a bespoke protocol on the cell.

2. Pump on the link path / drainLive doing double work

Conceded, and moot now — the pump and the drain's second mode are gone. For the record you caught a real defect, not just a style issue: reaping at drain entry serially single-invalidated orphans the forEach would have batched, on exactly the path this series optimises. One small correction the other way: on current JDKs an empty ReferenceQueue.poll() is a null-check before the lock, not a synchronised call — but your placement objection stood regardless (it polled before the current hit check, so even non-allocating links paid it).

3. Leftovers

All done: retireAllLoadedDomains → retireLiveDomains, the stale detachLiveIndySwitchPoint javadoc rewritten, hasLiveSwitchPoints() kept ahead of drainLive() (it documents the GROOVY-12258 contract; classic-only processes still skip everything — classic categoryInLoop measures 58.8 ± 0.4 ms/op on the new head, same level as the flag and registry variants).

4. Tests

Your three behaviours are the new tests: exact-class invalidation after MC collection (deterministic, no GC); a discarded class's domain reclaimed after collection (with the soft-ref pressure caveat above); and batch-only bulk retirement, which is now structural — there is no single-invalidation mode left for churn to fall into. The two reaper tests and the clearOwnerRefForTesting hook you (rightly) flagged as encoding a fragile invariant are gone. Existing per-MC-identity tests were reworked to per-class semantics. Full root suite: 16,773 tests green.

Net size across SwitchPointInvalidator + IndyInvalidation + ClassInfo: 1552 lines at the registry commit → 1656 with the reaper → 1518 with your shape, despite the added reclaim anchor. Cohesion argument validated by wc -l.

Since the PR is effectively a new implementation, treat this as a fresh review request — the earlier inline comments are all addressed or mooted by the rewrite.

[paulk-asert]

Does this replace #2786 ?

Yes, #2786 was the quick win fallback but doesn't cover all of the cases - it was just trying to address the drop in classic performance noted in the jmh classic graph after GROOVY-12191 optimised indy:
https://apache.github.io/groovy/dev/bench/jmh/summary.html

[codecov-commenter]

Codecov Report

❌ Patch coverage is 94.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.1353%. Comparing base (ad907ac) to head (afd939d).
⚠️ Report is 18 commits behind head on master.

Files with missing lines	Patch %	Lines
...g/apache/groovy/runtime/indy/IndyInvalidation.java	88.2353%	1 Missing and 1 partial ⚠️
...he/groovy/runtime/indy/SwitchPointInvalidator.java	94.7368%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##               master      #2786        +/-   ##
==================================================
+ Coverage     70.1168%   70.1353%   +0.0185%     
- Complexity      35772      35799        +27     
==================================================
  Files            1561       1562         +1     
  Lines          132362     132387        +25     
  Branches        24331      24334         +3     
==================================================
+ Hits            92808      92850        +42     
+ Misses          31156      31139        -17     
  Partials         8398       8398                

Files with missing lines	Coverage Δ
...java/org/codehaus/groovy/reflection/ClassInfo.java	88.6256% <100.0000%> (+1.0719%)	⬆️
...he/groovy/runtime/indy/SwitchPointInvalidator.java	97.8723% <94.7368%> (-2.1277%)	⬇️
...g/apache/groovy/runtime/indy/IndyInvalidation.java	87.1287% <88.2353%> (-2.0605%)	⬇️

... and 15 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.

[testlens-app]

✅ All tests passed ✅

🏷️ Commit: afd939d
▶️ Tests: 110204 executed
⚪️ Checks: 31/31 completed

---

Learn more about TestLens at testlens.app.