[
https://issues.apache.org/jira/browse/GROOVY-12281?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18106494#comment-18106494
]
Paul King commented on GROOVY-12281:
------------------------------------
Thanks [~blackdrag] - the reverse memory pressure question was the important
one: it found a real leak, now fixed, and the fix turned out to also be the
answer to two of your inline comments.
*The reverse test.* I implemented it as you described (runtime stays alive,
script loaders come and go, each script installs an EMC on a class it created,
then memory pressure). Under the prototype as reviewed it *failed*: 0/20
loaders collected, where the default mode collects 20/20. The cause was the
global strong root set - its justifying comment ("everything in it dies with
Groovy's own loader, which is the lifetime every ClassInfo has today") is true
for immortal platform keys but wrong for collectible ones: a script class's
today-lifetime is _its class's_ lifetime, because a plain ClassValue
association is an ephemeron and dies with its key. The set extended every
EMC-dirty script class (root -> ClassInfo -> strongMetaClass -> theClass ->
loader) to the runtime's lifetime. Control runs isolated it exactly: clean
script classes collected fine, and {{ClassInfo.remove()}} unrooted correctly.
The fix moves the pin _into the association_: the store's per-Class slot (a
bootstrap {{AtomicReference}}) holds either a bootstrap {{SoftReference}}
(reclaimable) or the value itself (pinned). The strong hold is then reachable
only from the key class, so a pinned ClassInfo has exactly a plain ClassValue
association's lifetime - immortal platform keys retain it (they must - the
state is not reconstructible), dropped script classes release it with their
loader. With the fix the reverse probe collects 20/20 under soft, same as the
default, and it is now a scenario in {{ClassInfoSoftModeProbe}}. (Two
probe-construction notes recorded in the assessment: the scenario is
{{@CompileStatic}} and nulls its loop locals on purpose - indy call-site guards
in a long-lived caller and stale frame slots retain script classes in *every*
mode, receiver-side effects distinct from the association lifetime under test.)
One incidental find while running the matrix: {{groovy.use.classvalue=false}}
on current master *also* fails the reverse scenario (0/20), while released
5.0.6 and 6.0.0-beta-2 collect 20/20 - the reworked escape hatch's
weak-key/strong-value map is not an ephemeron, so a value that reaches its own
key revives it forever. That predates this PR; I'll file it separately.
*{{isSoftMode()}} coupling.* Agreed, and the leak fix supplied the right
abstraction. The property {{ClassInfo}} actually depends on is "can a value be
collected while its key class is still alive?" - false for the strong
ClassValue *and* for the map (whose values die with their class), true only for
soft. {{GroovyClassValue}} now exposes that as a capability
({{valuesReclaimable()}}, default false) plus default-no-op {{pin}}/{{unpin}};
{{ClassInfo}} asks its own store, and {{isSoftMode()}} is gone from the
factory's surface.
*"Always do it or simplify".* With {{pin}}/{{unpin}} on the interface,
{{updateReclaimability()}} now runs unconditionally in every mode (no-ops
elsewhere), and {{ClassInfo.remove()}} lost its store-specific branch entirely
- the pin travels with the association. Always maintaining the previous *set*
in all modes would have been wrong, for what it's worth: in {{=false}} mode it
would add pinning that mode exists to avoid, and in default mode it was pure
overhead.
*Architecture.* Agreed it was under-documented, and your layered reading is the
intended one: {{SwitchPointInvalidator}} is the policy-free mechanism (one
domain's SwitchPoint lifecycle plus the live registry), {{IndyInvalidation}}
the policy layer (width, reasons, anchoring, per-Class continuity), and
{{ClassInfo}} the only other supported consumer (it owns domain instances and
performs the local operations directly). Java visibility can't enforce that
across packages, so both classes now carry a "Layering" javadoc section naming
the supported consumers and which guarantees exist at which level, and both are
annotated {{@Internal}}. (Most of that machinery is GROOVY-12191's - the
question really spans both changes.)
*Boolean for the mode.* Done - the factory stores parsed booleans and the mode
string never leaves it.
*Hybrid.* It was never intended to land (measured and declined), so I've
restructured the PR to make that explicit: the branch now carries only the
master-bound payload - the soft commit rebuilt without the hybrid arm (factory
is {{true}}/{{soft}}/{{false}} only), plus the review-response delta as a
separate commit for your re-review, to be squashed at landing. The
investigation artifacts (hybrid, guide quantification, assessment, spikes)
moved to {{groovy12281-investigation}}. Apologies for the close/reopen noise on
the PR - force-pushing the restructure to both repos briefly made head and base
identical, which auto-closed it; the base branch is now parked at the fork
point. Your inline comments will show as outdated because of the rebuild, but
they're all addressed above.
*Soft vs weak.* Soft is deliberate: the canonical side map is weak-valued, so
with weak slot references an otherwise-unreferenced ClassInfo would be cleared
at every minor GC and platform-receiver ClassInfos would churn through
recreation between collections - losing exactly the cache behaviour the mode
exists to preserve. Soft approximates "collect only under pressure", which is
also the ticket's acceptance criterion. Agreed it can stay; with the slot
design the strength choice is localized in {{GroovyClassValueSoft}} and could
become a policy parameter later if we ever want it.
State: unit tests now cover the capability and pin/unpin lifecycle (9), the
probe has 8 scenarios including the reverse one, and the reflection package,
metaclass suites and the real-GC stress test are green. The assessment is at v3
with the full review record in section 12.
> 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)
