[
https://issues.apache.org/jira/browse/GROOVY-12281?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18106071#comment-18106071
]
Jochen Theodorou commented on GROOVY-12281:
-------------------------------------------
I went through the current `ClassInfo` / `CachedClass` / MetaClass / call-site
machinery in more detail, and I think the assessment currently makes some
assumptions that should be separated from what has actually been demonstrated.
h3. 1. `ClassInfo` is not intrinsically immortal
The current design does not make `ClassInfo` intrinsically immortal.
`globalClassValue` is the canonical `Class -> ClassInfo` association, but
`ClassInfo` itself only weakly references the `Class`. The related runtime
objects are also managed caches:
{code:java}
MetaClassImpl -> CachedClass -> ClassInfo
ClassInfo - -soft-> CachedClass
ClassInfo - -soft/weak-> MetaClass
ClassInfo - -weak-> Class
{code}
`ClassInfo` keeps its `CachedClass` through a soft `LazyReference`, while the
non-strong MetaClass is held through a managed soft reference. Conversely,
`CachedClass` has a strong `classInfo` reference and `MetaClassImpl` has a
strong `CachedClass` reference.
This means that a `ClassInfo` can in principle become collectible when no
external runtime structure keeps this chain alive. A new `ClassInfo` is
therefore not conceptually impossible.
The existing code also deliberately supports removing the canonical association
through `ClassInfo.remove(Class)`, for example for application/class-loader
undeployment.
h3. 2. Legacy call sites can retain `ClassInfo`, but this makes the
SoftReference argument less straightforward
The old `groovy-callsite` implementation is an important special case.
For example, `PojoMetaClassSite` stores both:
{code:java}
private final ClassInfo classInfo;
private final MetaClass metaClass;
private final int version;
{code}
and validates the cached site using the captured `ClassInfo` version.
The other legacy sites such as `ClassMetaClassGetPropertySite`,
`MetaClassConstructorSite`, `PerInstancePojoMetaClassSite` and
`StaticMetaClassSite` use the same general caching machinery.
Thus a live legacy call site can indeed keep an old `ClassInfo` strongly
reachable.
However, this makes the SoftReference argument in §3.3 less straightforward
than it currently appears.
If a live call site strongly retains `ClassInfo A`, then A is strongly
reachable. GC-driven clearing of a `SoftReference<ClassInfo A>` therefore
cannot simply produce:
{code:java}
ClassValue -> ClassInfo B
live call site -> ClassInfo A
{code}
The assessment needs a concrete reachable-object sequence showing how the
canonical association can disappear while an old `ClassInfo` remains strongly
reachable and can still be used by a live call site.
Merely showing that two `ClassInfo` objects can coexist is not sufficient. They
can already coexist through explicit cache removal/recreation, so the relevant
question is whether the proposed implementation introduces a new unsafe way for
this to happen.
h3. 3. Indy and the legacy call-site implementation should be analysed
separately
The newer indy machinery appears to obtain the current `ClassInfo` through
`ClassInfo.getClassInfo(...)` in many places rather than capturing an old
`ClassInfo` in the same way as the legacy call sites.
If that is correct, the identity/guard argument needs to be demonstrated
independently for indy rather than generalized from the legacy call-site
implementation.
In particular, the relevant question is whether an indy call site can continue
to depend on an old `ClassInfo` after a new one has become canonical, and under
what exact reference graph that can happen.
h3. 4. "Pristine" and DGM state need a more precise definition
I am also not convinced by the current use of "pristine".
The fact that DGM state is present in `ClassInfo` does not by itself establish
that the `ClassInfo` is non-recomputable.
A trivial user class can have its `ClassInfo` created on demand and is still
affected by DGM through `Object`. Therefore the relevant question is not
whether DGM state is present, but whether that state can actually be
reconstructed from surviving global/runtime state.
The assessment currently states that `dgmMetaMethods` and `newMetaMethods` are
written during registry/extension-module registration and that there is no hook
for recomputation. I think the missing step is to establish that the
information required to reconstruct those arrays is genuinely unavailable after
the `ClassInfo` has been collected.
In other words:
{code:java}
"the current implementation writes this state once"
{code}
is not equivalent to:
{code:java}
"the state is fundamentally non-reconstructible"
{code}
Those should be demonstrated separately.
h3. 5. `MetaClassImpl` and `CachedClass` are themselves deliberately
replaceable/cache-like
The current `CachedClass` implementation also makes me hesitant to treat every
object reachable from `ClassInfo` as identity-bearing state.
`CachedClass` contains numerous lazy/managed references for methods, fields,
constructors, hierarchy, interfaces, superclass information, etc. These are
clearly cache structures.
Likewise, `CachedClass.setNewMopMethods()` and `addNewMopMethods()` can discard
an existing `MetaClassImpl`, update the MOP information, create a new
`MetaClassImpl`, initialize it and install it again.
So the runtime already deliberately recreates MetaClass state while retaining
the same `ClassInfo`.
This suggests that "state is recreated" and "state is identity-bound" need to
be distinguished more carefully.
h3. 6. `indySwitchPointDomain` should be treated as a separate, recent design
decision
The `indySwitchPointDomain` field is a relatively recent addition to
`ClassInfo`.
I would therefore not use its existence as evidence that `ClassInfo` has
historically had an absolute identity/lifetime invariant.
It may instead be a consequence of the current indy invalidation
implementation, in which the switch-point domain was deliberately attached to a
particular `ClassInfo` instance.
If so, an important design question is whether this state really has to be
owned by `ClassInfo`, or whether it should instead be associated with a more
stable per-`Class` identity.
h3. 7. Missing from the investigation
As I see it, the investigation has demonstrated that simply making `ClassInfo`
softly collectible is not obviously semantics-preserving. However, I do not
think it has yet demonstrated all of the claimed correctness failures.
In particular, I think the following questions still need explicit
investigation:
# For which `ClassInfo` state is recomputation genuinely impossible, as
opposed to merely unsupported by the current implementation?
# What strong references can keep an old `ClassInfo` alive after its cache
association has disappeared?
# Can the legacy call-site "old `ClassInfo`" scenario actually coincide with
GC-driven removal of the `ClassValue` association?
# Does the indy machinery have the same identity requirement, or does it
resolve the current `ClassInfo`?
# Is `indySwitchPointDomain` a fundamental requirement of `ClassInfo`
identity, or a consequence of the newer invalidation implementation?
# Can DGM/extension state be reconstructed from the registry/module state, or
does some genuinely non-reconstructible information exist only in the old
`ClassInfo`?
I think answering these questions would make the conclusion much stronger than
the current blanket statement that all drop-and-recompute schemes are
"structurally illegal".
At the moment, my conclusion is that the problem is clearly non-trivial, but
that the current assessment has not yet closed the recomputation avenue to the
extent claimed.
> 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
>
> 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)
