[
https://issues.apache.org/jira/browse/GROOVY-12281?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18106128#comment-18106128
]
Paul King commented on GROOVY-12281:
------------------------------------
Thanks Jochen — you were right that the "structurally illegal" conclusion
overreached, and re-grounding each claim in the code has been productive: the
recomputation avenue is not just open, it now has a working prototype (branch
groovy12281, assessment updated to v2). Taking your points in order:
*1 / 5 (ClassInfo not intrinsically immortal; caches deliberately recreated)* —
conceded. The runtime already recreates MetaClass/CachedClass state while
retaining the ClassInfo shell, and DomainReclaim already treats ClassInfo death
as an anticipated event. The identity-bearing residue is much smaller than v1
claimed: version (guard stamps), the strong/per-instance MetaClass slots, and
the MOP method arrays.
*4 / 6 ("written once" vs "non-reconstructible")* — conceded, and it resolves
better than reconstruction: the registry permanently retains every registered
MetaMethod ({{MetaClassRegistryImpl.instanceMethods/staticMethods}}, extension
modules included), and every registration path stores the _same CachedClass
instance_ the methods are written into ({{GeneratedMetaMethod.declaringClass}};
{{NewMetaMethod.bytecodeParameterTypes[0]}}; dgmimpl static fields). Since
CachedClass holds classInfo strongly, *any ClassInfo with non-empty DGM arrays
is permanently strongly rooted by the registry* — it can never be
soft-collected, so a recreated instance never needs to rebuild those arrays
(they are always empty for collectible classes, with hierarchy dispatch
supplying inherited DGM methods, which is the normal path). The prototype
enforces this invariant by construction rather than by audit: a MOP-array write
roots the ClassInfo ({{CachedClass.updateSetNewMopMethods}} →
{{ClassInfo.updateReclaimability}}), which also covers hypothetical third-party
MetaMethod implementations.
*2 (concrete reachable-object sequence for split-brain)* — here is one, and it
is sharper than v1's version: classic call sites are reachable only through the
caller's {{static SoftReference $callSiteArray}} (CallSiteWriter), so "strong
retention by a live call site" is itself only _soft_ reachability.
Soft-reference clearing is per-referent, and a linked classic site never
touches the ClassValue on its fast path (the guard reads version from the
captured instance), so the ClassValue's {{SoftReference<ClassInfo>}} goes
LRU-stale precisely while the site is hottest. A collector may therefore clear
the association while the site's chain survives: next getClassInfo creates B,
an EMC installed via B bumps B's version, and the surviving site guards A
forever. So naive soft values are indeed unsafe — but the fix is structural:
*resurrection*. A weak-key/weak-value side map is the identity authority;
computeValue consults it first, so a value still reachable _anywhere_ is
re-associated rather than replaced. A fresh instance can only be created once
the old one is weakly unreachable — at which point no guard can still reference
it, so two live generations of one association cannot arise, and version
continuity is automatic. remove(Class) stays a hard detach (undeploy semantics
unchanged).
*3 (indy analysed separately)* — done. Indy resolves ClassInfo fresh at link
time and invalidation routes through the canonical instance, but installed
guards keep only the SwitchPoint's internal invoker alive, and POJO
direct-dispatch chains capture no Groovy object at all (GroovyObject/MOP paths
do capture mc). So an indy guard _can_ outlive its ClassInfo, and under soft
values the lazy DomainReclaim pump would leave an unbounded stale-guard window.
*6 (indySwitchPointDomain ownership)* — your instinct was right: attaching the
domain to a stable per-Class identity is the correct fix for the above. In soft
mode the domain is adopted from a weak-Class-keyed map
({{IndyInvalidation.classDomainFor}}), so a successor ClassInfo shares its
predecessor's domain and a mutation applied through the successor
deterministically retires guards linked under the predecessor. This is
deliberately an indy-side improvement; classic sites need no equivalent because
resurrection preserves the instance their guards captured — relevant given the
deprecation path for the classic callsite code, which must keep working from
the classpath but shouldn't constrain indy design.
*Results* (prototype = {{-Dgroovy.use.classvalue=soft}}, all details and
file:line references in the v2 assessment):
* Correctness: unit tests + a child-JVM probe covering resurrection
identity/version continuity, DGM rooting, EMC and per-instance-MetaClass
survival under forced clearing, true collection + fresh-instance dispatch,
classic CallSiteArray behaviour across clear + EMC change, and
predecessor-SwitchPoint retirement across recreation — all pass;
reflection/indy/callsite/groovy.lang suites (1200+ tests) pass unchanged in
default mode, and the full core suite (16,800 tests) passes when run entirely
under soft mode. A real-GC concurrency stress probe (small heap,
SoftRefLRUPolicyMSPerMB=0, concurrent indy + classic dispatch racing the
collector against 129 EMC generations) sustained 66M dispatches with 33/36
platform-receiver ClassInfos genuinely collected and recreated mid-run and zero
invariant violations. A side-finding worth noting: generated Groovy classes
self-pin their ClassInfo while the class lives ({{$staticMetaClass}} + the POGO
metaClass field), so the collectible population under soft mode is
platform-class ClassInfos and dead-Class Groovy islands — which further shrinks
the recreate-risk surface.
* Acceptance (the test the ticket exists for): a dropped child-loader Groovy
copy is *PINNED forever under today's default ClassValue*, and *collected under
soft mode* once memory pressure clears soft references (map mode likewise;
spike kept on the branch).
* Cost: +≈0.6 ns on a raw getClassInfo lookup (1.2 → 1.7 ns; the map escape
hatch is ≈4.7 ns); dynamic dispatch macro loops and the compiler harness
(+0.0%) at parity. The JMH classic-bytecode gate (37 benchmarks, same
disjoint-CI rule that declined the map default with 12 significant
regressions): 0 of 37 significant, geomean +1.7%. Focused re-measurement of the
one directional outlier puts classic _polymorphic_ dispatch at a pooled +3.7% —
consistent with one SoftReference.get per classic inline-cache miss, irrelevant
to indy bytecode, and flagged for the idiom-suite sweep before any default-flip
discussion.
So v1's B/D verdicts stand, but its blanket recompute verdict is withdrawn:
soft-with-resurrection is the first option that demonstrably unpins the loader
while keeping the ClassValue fast path. The consolidated option landscape
(three modes × static/dynamic × indy/classic, with guidance) is assessment §9;
remaining questions for review are §10 (JDK ClassValue entry internals verified
for 17/21/23; soft values linger until pressure by design; adoption path —
opt-in first vs default).
https://github.com/apache/groovy/blob/groovy12281/GROOVY-12281-assessment.html
> Investigate mitigating class-loader pinning by ClassInfo.globalClassValue
> under the default ClassValue implementation
> ---------------------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12281
> URL: https://issues.apache.org/jira/browse/GROOVY-12281
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Priority: Major
> Attachments: GROOVY-12281-Assessment.pdf
>
>
> Follow-up from review discussion on [PR
> #2798|https://github.com/apache/groovy/pull/2798] (GROOVY-12142), preserving
> the analysis from that review. A {{ClassValue}} realizes the chain _key class
> → association → value → everything reachable from it_, and the association
> lives as long as the key class
> ([JDK-8136353|https://bugs.openjdk.org/browse/JDK-8136353], working as
> intended). The {{ClassValue}} implementation class itself prevents no
> unloading; what matters is the key's origin: a Groovy-loaded key class dies
> with Groovy's loader (fine), but a JDK/platform key class is effectively
> immortal, so any value reachable from it that was loaded by Groovy's loader
> pins that loader forever. Indirection counts — a JDK-typed value (e.g. an
> {{ArrayList}}) whose _elements_ are Groovy-loaded re-creates the pin one
> level down.
> {{ClassInfo.globalClassValue}} is static with unknown keys, including
> platform classes ({{String}} receives a {{ClassInfo}} in essentially every
> Groovy program), so the default {{ClassValue}} path pins the loader; today's
> only remedy is the global {{groovy.use.classvalue=false}} escape hatch, which
> trades away the per-class fast path for all keys.
> The general mitigation — {{SoftReference}}-wrapped values with a
> check-remove-recompute protocol — requires that recomputation be legal, and
> for {{ClassInfo}} it is not in general: a {{ClassInfo}} can carry
> non-recomputable state (modified metaclasses, category state), so a
> softly-collected value could silently discard user metaclass customizations.
> Approaches to investigate:
> * soft values while a {{ClassInfo}} is pristine, hardening the reference on
> first mutation — only classes with customized metaclasses would then pin, a
> far smaller set;
> * per-key policy: platform-loader keys get soft/map treatment, Groovy-loader
> keys stay strong (the key-origin rule applied mechanically);
> * splitting {{ClassInfo}} into recomputable and stateful parts;
> * revisiting whether the map-based implementation should become the default,
> with {{ClassValue}} as the opt-in fast path.
> Recompute cost and dispatch-path performance need measurement for any
> candidate.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
