[
https://issues.apache.org/jira/browse/GROOVY-12259?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18105150#comment-18105150
]
ASF GitHub Bot commented on GROOVY-12259:
-----------------------------------------
paulk-asert commented on PR #2786:
URL: https://github.com/apache/groovy/pull/2786#issuecomment-5310469374
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.
> Make category/bulk call-site invalidation O(live SwitchPoint domains) instead
> of O(loaded classes)
> --------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12259
> URL: https://issues.apache.org/jira/browse/GROOVY-12259
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Priority: Major
>
> GROOVY-12258 made process-wide call-site invalidation a no-op in processes
> that never link an indy MOP guard (classic-only bytecode). This follow-up
> addresses the remaining cost for indy and mixed processes: once any indy site
> has linked, every category enter/leave (and custom-MetaClass / unattributed
> registry event) still retires domains by walking every loaded class.
> h3. Problem
> {{IndyInvalidation.retireAllLoadedDomains()}} iterates
> {{ClassInfo.getAllClassInfo()}} — O(all loaded classes), a weak-reference
> pointer chase over tens of thousands of entries in a framework-sized app —
> *twice per {{use}} block* (enter and leave), collecting the comparatively few
> domains that actually hold a live SwitchPoint.
> h3. Proposed fix
> Track live SwitchPoints in a process-wide registry so bulk retirement is
> O(live domains):
> * {{SwitchPointInvalidator}} gains a static {{ConcurrentHashMap<SwitchPoint,
> SwitchPointInvalidator>}} of every live SwitchPoint mapped to its owning
> invalidator. It is keyed by *SwitchPoint*, not invalidator: each SwitchPoint
> has a single-use lifecycle (allocated once, detached once), so a removal can
> never clobber a successor's entry the way an invalidator-keyed registry could
> (ABA on re-allocation).
> * {{getSwitchPoint()}} registers the new SwitchPoint *before* the publishing
> CAS (and deregisters on CAS loss). The GROOVY-12258 ordering argument carries
> over: a bulk path that finds no entry is guaranteed that SwitchPoint was not
> yet visible to any guard, so skipping it is safe.
> * Both detach paths ({{detachLive()}} and the new {{detachIfCurrent(sp)}})
> deregister on successful detach, so all maintenance funnels through the
> existing allocation/retirement choke points; per-class invalidation needs no
> changes.
> * {{retireAllLoadedDomains()}} drains the registry: each entry is claimed via
> {{detachIfCurrent}}, and an entry is removed *only on a successful claim*.
> Removing on a failed claim would strand a concurrently-publishing SwitchPoint
> permanently invisible to all future drains (a correctness trap: pre-publish
> entries must survive the drain); failed-claim entries are transient and are
> cleaned up by their owner.
> * The GROOVY-12258 monotonic flag is replaced by {{registry-is-empty}}, which
> is strictly stronger: it also skips after all domains have retired, and
> re-arms rather than being one-shot. The classic-only skip is preserved.
> The drain's weakly consistent iteration can miss a SwitchPoint published
> mid-drain — the same window as the previous all-classes walk; sites linking
> concurrently read the current category state at link time, unchanged. One
> trade-off to note: the registry holds keys strongly, so a domain whose
> MetaClass has died stays pinned (one small SwitchPoint + invalidator) until
> any bulk event sweeps it.
> h3. Measurements
> JMH ({{-PbenchInclude=CategoryBench.categoryInLoop :perf:jmh}}, 2 forks x 5
> iterations, JDK 21, same machine throughout):
> ||build||indy||classic||
> |Groovy 5 (5.1.x branch)|1489.8 ± 39.1 ms/op|163.2 ± 12.8 ms/op|
> |Groovy 6 pre-GROOVY-12191|1269.1 ± 45.5 ms/op|147.5 ± 12.0 ms/op|
> |master (post-GROOVY-12191)|2141.1 ± 97.8 ms/op|365.9 ± 18.5 ms/op|
> |+ GROOVY-12258|2141.1 ± 97.8 ms/op|59.9 ± 1.2 ms/op|
> |+ this change|1596.1 ± 57.1 ms/op|60.5 ± 1.1 ms/op|
> Indy improves 25% over master; classic keeps the GROOVY-12258 level. On this
> worst-case category-churn bench, indy remains ~26% behind pre-GROOVY-12191:
> the residual is the relink storm (scoped domains retire many per-MetaClass
> SwitchPoints where the old design retired one global one), not the walk.
> Closing that would need a further design change — e.g. a dedicated category
> SwitchPoint axis so category enter/leave stops retiring MetaClass domains —
> and is out of scope here.
> New unit coverage includes a get/detach/drain concurrency hammer asserting no
> live SwitchPoint is ever left unregistered, exactly-once claiming between
> owner detach and bulk drain, and CAS-loser cleanup. All existing
> indy/vmplugin and category runtime tests pass.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
