GROOVY-12281: opt-in soft GroovyClassValue mode — unpins class loaders while keeping the ClassValue fast path - #2820
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## groovy12281 #2820 +/- ##
================================================
Coverage ? 70.2671%
Complexity ? 36198
================================================
Files ? 1571
Lines ? 133613
Branches ? 24592
================================================
Hits ? 93886
Misses ? 31236
Partials ? 8491
🚀 New features to boost your workflow:
|
blackdrag
left a comment
blackdrag
left a comment
There was a problem hiding this comment.
I think besides the architecture question I have one more... What happens if you do the reverse memory pressure test? I mean spinning up many GroovyShells and execute some scripts in there, that for example set EMC on classes they create. The expectation would be that on memory pressure the old script instances and their meta classes can be collected. I called it reverse, because in this case the Groovy runtime stays alive.
| * | ||
| * @since 6.0.0 | ||
| */ | ||
| public final class IndyInvalidation { |
There was a problem hiding this comment.
The architectural relationship between IndyInvalidation and SwitchPointInvalidator makes me wonder. The issue is not only that IndyInvalidation is tightly coupled to SwitchPointInvalidator, but also which of the two is supposed to be the abstraction boundary?
-
If
IndyInvalidationis a wrapper/facade aroundSwitchPointInvalidator, thenSwitchPointInvalidatoris effectively an implementation detail of the higher-level invalidation mechanism. In that case, it would be surprising forSwitchPointInvalidatorto remain independently exposed and used. Consumers should normally go throughIndyInvalidation, otherwise the wrapper does not really define the abstraction boundary. -
If
SwitchPointInvalidatoris intended to be an independently usable abstraction, then the two classes should be considered independent components which happen to cooperate. In that case, their respective responsibilities, lifecycle guarantees and invariants need to be clearly defined. In particular, it should be clear which guaranteesSwitchPointInvalidatorprovides on its own and which only exist when it is managed throughIndyInvalidation.
At the moment, the implementation seems somewhat in between these two models. IndyInvalidation directly manages SwitchPointInvalidator instances and depends on its lifecycle and global state, which makes the relationship look more like a layered abstraction than two independent components. At the same time, SwitchPointInvalidator is still exposed and can be used independently.
This should be document, otherwise it is difficult to determine if the current coupling is intentional or simply an implementation dependency that has become externally visible.
If SwitchPointInvalidator can be used independently as well as through IndyInvalidation, then the correctness of the overall invalidation machinery cannot simply rely on invariants established by IndyInvalidation. The independent use cases need to obey the same assumptions, or the boundary between the two needs to enforce those assumptions.
If SwitchPointInvalidator is really only the low-level mechanism behind IndyInvalidation, hiding it behind that abstraction would make the ownership and lifecycle model much easier to reason about and would also leave room to change the underlying invalidation mechanism later.
I think the important point is not simply whether the classes are coupled, but whether the current API and usage actually match the intended abstraction boundary. If they are one layered subsystem, that relationship should be explicit; if they are independent abstractions, their independence and invariants need to be justified.
| * strong roots for non-reconstructible state ({@link #nonReclaimableRoots}) | ||
| * and per-Class indy domain continuity (see {@link #indyDomain()}). | ||
| */ | ||
| private static final boolean SOFT_CLASS_VALUES = GroovyClassValueFactory.isSoftMode(); |
There was a problem hiding this comment.
This isSoftMode() is a bit suspicious. Again it produces a tight coupling to a certain implementation. Why is a ClassValue with SoftReferences a soft mode, but a ManagedMap based version not?
| * {@code null} unless soft mode is active. | ||
| */ | ||
| private static final Set<ClassInfo> nonReclaimableRoots = | ||
| SOFT_CLASS_VALUES ? ConcurrentHashMap.newKeySet() : null; |
There was a problem hiding this comment.
What happens if we always do that and not only in soft mode? This is also not the only place. Either we have to do it, then this is missing an abstraction in my eyes. Or we do not, well, then we should simplify the code
| * than kept local to {@link #createGroovyClassValue}. | ||
| */ | ||
| static boolean isSoftMode() { | ||
| return "soft".equalsIgnoreCase(CLASSVALUE_MODE); |
There was a problem hiding this comment.
why not store soft mode as boolean?
| // pin the value's loader while user classes keep the per-class fast path (measured, | ||
| // declined); "soft" keeps ClassValue for all keys but holds values softly with | ||
| // resurrection, so immortal keys hold no strong chain to the value's loader. | ||
| if ("hybrid".equalsIgnoreCase(CLASSVALUE_MODE)) { |
There was a problem hiding this comment.
not part of the PR really, but if you do a boolean for soft mode, you could also do one for hybrid then. Or you switch to a model where you do not use the boolean, but a provider GroovyClassValueHybrid::new and so on. Just a thought
| private final ClassValue<SoftReference<T>> store = new ClassValue<SoftReference<T>>() { | ||
| @Override | ||
| protected SoftReference<T> computeValue(final Class<?> type) { | ||
| return new SoftReference<>(canonical(type)); |
There was a problem hiding this comment.
I actually no wonder... does the ClassInfo case really need a SoftReference or could it be a WeakReference? The SoftReference is maybe the more "general", "cache friendly" variant, but I have now second thoughts of if it should be defined at this level, also considering the factory. But it could stay like this for now
…ion (opt-in) Adds -Dgroovy.use.classvalue=soft: java.lang.ClassValue keeps the per-Class fast path for all keys, but each association stores a bootstrap-loaded SoftReference to the ClassInfo, cutting the only strong chain from immortal platform classes to Groovy's loader (JDK-8136353 / GROOVY-12142). Correctness pieces, addressing the review findings on the v1 assessment: - Resurrection: a weak-key/weak-value side map is the identity authority; computeValue re-associates a still-reachable value instead of replacing it, so a fresh instance can only exist once no guard can observe the old one (kills the split-brain sequence; classic call-site version guards stay sound, so legacy groovy-callsite jars on the classpath are unaffected). - Dirty roots: ClassInfos carrying non-reconstructible state (installed MetaClass, per-instance MetaClasses, registry-written MOP arrays) are strong-rooted Groovy-side; the MOP-write hook also enforces the registry-rooting invariant (non-empty DGM arrays => never collectible) by construction. ClassInfo.remove() unroots, keeping hard-detach undeploy semantics. - Per-Class indy domain continuity: in soft mode SwitchPoint domains are adopted from a weak-Class-keyed map, so a successor ClassInfo shares its predecessor's domain and mutations deterministically retire guards that captured only the SwitchPoint (POJO direct dispatch); domain reclaim moves from ClassInfo-death to Class-death. Indy-only; default mode unchanged. Tests: GroovyClassValueSoftTest (unit); ClassInfoSoftModeTest/Probe (child JVM, deterministic forced clearing: resurrection identity+version, DGM rooting, EMC and per-instance survival, true collection + fresh dispatch, classic CallSiteArray soundness across clear + EMC change, predecessor SwitchPoint retirement); ClassInfoSoftModeStressTest/Probe (child JVM, real GC clearing: 128MB heap + SoftRefLRUPolicyMSPerMB=0, concurrent indy + classic dispatch racing the collector through ~130 EMC generations — 66M dispatches, 33/36 platform-receiver ClassInfos collected and recreated mid-run, zero invariant violations). build-logic forwards -Pgroovy.use.classvalue to test JVMs so whole suites can soak a non-default mode; the full core suite passes under soft mode (16,800 tests).
association; expose value reclaimability as a store capability Jochen's reverse memory-pressure question on PR apache#2820 found a real leak: the global strong root set extended EMC-dirty script-created classes (and their loaders) to the runtime's lifetime, where the default mode releases them with their class (probe: 0/20 loaders collected vs 20/20). The root set's justifying comment was true for immortal platform keys but false for collectible ones. - GroovyClassValue gains valuesReclaimable() plus default-no-op pin/unpin: callers never consult the mode; they ask the store for its capabilities. - GroovyClassValueSoft stores a per-Class slot (bootstrap AtomicReference) holding either a bootstrap SoftReference (reclaimable) or the value itself (pinned). The strong hold lives inside the association — an ephemeron — so a pinned ClassInfo lives exactly as long as its class: an immortal platform key retains it, a dropped script class releases it together with its loader. pin() retries across a concurrent remove-and-recompute; unpin() is a CAS so a foreign value cannot downgrade the slot. - ClassInfo drops nonReclaimableRoots and the isSoftMode() coupling; updateReclaimability() pins/unpins unconditionally (no-ops outside soft mode) and remove() needs no store-specific unroot: the pin travels with the association. - GroovyClassValueFactory stores the parsed mode as booleans; the mode string never leaves the factory. - IndyInvalidation / SwitchPointInvalidator document their layering (mechanism vs policy, with ClassInfo the only other supported consumer) and are annotated @internal. Tests: GroovyClassValueSoftTest adds the capability, pin-until-unpin, remove-releases-pin and foreign-unpin-noop cases; ClassInfoSoftModeProbe gains the reverse scenario (an EMC-dirty script class dies with its loader under memory pressure) and asserts pinned slots directly — the stronger claim, since a pinned slot has no clearable reference. The reverse scenario is @CompileStatic and nulls its loop locals on purpose: indy call-site guards in the long-lived probe class and stale frame slots retain script classes in every mode — receiver-side effects distinct from the association lifetime under test. The classic call-site scenario moves to a clean POJO receiver, since a pinned receiver like String no longer has a clearable reference. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
a429dd7 to
f242c84
Compare
2e451ce to
f242c84
Compare
✅ All tests passed ✅Test SummaryBuild and test / lts (17, ubuntu-latest) > :test
Build and test / lts (17, windows-latest, 1) > :test
🏷️ Commit: f242c84 Learn more about TestLens at testlens.app/docs. |
|
@blackdrag Response to your questions in the issue. |
3 participants
What this PR is
The master-bound payload of the GROOVY-12281 investigation, now exactly two commits:
GroovyClassValuemode — soft values with resurrection (opt-in): the reviewedchange, rebuilt without the declined hybrid prototype (the factory has only
true/soft/falsearms);review below, kept as a separate commit so it can be re-reviewed against the earlier
state. Intended to be squashed into the first commit at landing — the earlier
global-root-set design should not exist in master history (it has the reverse-scenario
leak described below).
The investigation artifacts (hybrid prototype, escape-hatch cost quantification for the
guide, assessment document, loader/perf spike harnesses) now live on the companion branch
groovy12281-investigation:GROOVY-12281-assessment.html
(v3; §12 records the review round and response).
Problem
A
java.lang.ClassValueassociation lives as long as its key class, soClassInfo.globalClassValueentries on immortal platform classes (String, Integer, …) hold astrong chain to the ClassInfo and through it to Groovy's class loader — leaking every Groovy
copy a container deploys and undeploys (JDK-8136353 / GROOVY-12142). The existing remedy,
-Dgroovy.use.classvalue=false, gives up the per-Class fast path for every key(~4× on the raw lookup, +4.8% geomean on classic dispatch, hot platform idioms to 1.38×).
Change
-Dgroovy.use.classvalue=softkeepsClassValuefor all keys but stores each value in aper-Class slot (a bootstrap
AtomicReference) holding a bootstrapSoftReference, cuttingthe only strong chain from immortal keys to the Groovy island. Three cooperating pieces make
that safe (details and file references in the assessment):
reachable anywhere is re-associated, never replaced, so a fresh instance can only exist once
no guard can still observe the old one. Classic call-site version guards therefore stay
sound: legacy-compiled jars using
groovy-callsitefrom the classpath are unaffected.per-instance MetaClasses, registry-written DGM/extension arrays) are pinned inside their
own association: the slot flips from soft to strong, so a pinned instance lives exactly as
long as its class — an immortal platform key retains it, a dropped script class releases it
together with its loader (the "reverse" scenario from review). The store advertises this
via a
GroovyClassValuecapability (valuesReclaimable()+ default-no-oppin/unpin),so
ClassInfonever consults the mode;ClassInfo.remove()keeps its hard-detach undeploysemantics with no store-specific handling — the pin travels with the association.
Classin softmode, so a successor ClassInfo adopts its predecessor's domain and mutations
deterministically retire guards that captured only the SwitchPoint (POJO direct dispatch).
Indy-only; the default mode's behaviour is unchanged. The
IndyInvalidation/SwitchPointInvalidatorlayering (policy vs mechanism, withClassInfothe only othersupported consumer) is now documented on both classes, which are
@Internal.Default-mode behaviour is byte-identical apart from no-op hook calls; all soft-mode structures
are unallocated unless the flag is set.
Evidence
pinned forever under the default and collected under soft mode once pressure clears
soft references.
go, each installing an EMC on a class it created — under soft mode the dropped loaders are
collected on memory pressure exactly as under the default (20/20; the earlier root-set
design leaked 0/20). Now a probe scenario.
(resurrection identity/version, pinned-slot assertions for DGM/EMC/per-instance state,
EMC removal unpins, collection + recreation dispatch, classic
CallSiteArraysoundness,predecessor-SwitchPoint retirement, the reverse scenario); a real-GC concurrency
stress probe (128 MB heap,
SoftRefLRUPolicyMSPerMB=0): 66M dispatches, ~130 EMCgenerations, 33/36 platform-receiver ClassInfos collected and recreated mid-run, zero
invariant violations; full core suite passes under soft mode (16,800 tests).
getClassInfo1.2 → 1.7 ns; compiler harness +0.0%; classic-bytecode JMH gate0 of 37 significant (geomean +1.7%; the declined map default failed the same rule with 12,
worst 1.38×); classic polymorphic miss traffic ≈ +3.7% pooled, flagged for the idiom-suite
sweep before any default-flip discussion.
Open questions for review
Assessment §9 consolidates the resulting option landscape (
true/soft/false×static/dynamic × indy/classic, with guidance); §10 lists the open items: adoption path
(opt-in now — recommended — vs eventual default), whether domain re-homing should become
unconditional rather than soft-gated, and container soak experience. §12 records the PR
review round. One separate finding will get its own JIRA:
=falseon current master failsthe reverse scenario (0/20 collected vs 20/20 on released 5.0.6/6.0.0-beta-2) — the
escape-hatch rework's weak-key/strong-value map is not an ephemeron.
JIRA: https://issues.apache.org/jira/browse/GROOVY-12281