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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
