[
https://issues.apache.org/jira/browse/GROOVY-12223?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18101558#comment-18101558
]
ASF GitHub Bot commented on GROOVY-12223:
-----------------------------------------
paulk-asert commented on PR #2755:
URL: https://github.com/apache/groovy/pull/2755#issuecomment-5173819415
I haven't done a proper review yet, but as part of some other work, I
assessed whether the PR impacts potential GraalVM support if we try harder to
support that in the future. It came back with below, I'm not sure we want to do
what is says yet - but just wanted to capture it somewhere for now:
> Two native-image observations from exercising this branch alongside the
packed-closure work (GROOVY-12227). Both are small; the first is a genuine easy
win.
>
> ### 1. The soft-fail contract doesn't hold under native image
>
> `tryDefineNestmate` catches `IllegalAccessException | SecurityException |
LinkageError` (plus `IllegalArgumentException` and
`IndexOutOfBoundsException`), and the class javadoc documents the intent as
returning `null` on the expected failure modes "so call sites fall back to
`ClassLoader#defineClass` with one null check".
>
> GraalVM signals "this runtime cannot define classes" with
`com.oracle.svm.core.jdk.UnsupportedFeatureError`, which extends
`java.lang.Error` **directly**:
>
> ```
> $ javap com/oracle/svm/core/jdk/UnsupportedFeatureError.class # from
lib/svm/builder/svm.jar, GraalVM CE 25.2.4
> public class com.oracle.svm.core.jdk.UnsupportedFeatureError extends
java.lang.Error {
> ```
>
> It is not a `LinkageError`, so it escapes the catch and propagates out of
the `try*` method — in precisely the environment where the fallback matters
most.
>
> A blanket `catch (Throwable)` would contradict the javadoc's "unexpected
failures ... are not swallowed as a blanket `RuntimeException`", so something
targeted is probably wanted, e.g.:
>
> ```java
> } catch (Error e) {
> // GraalVM native image: runtime class definition is unsupported.
Name-checked
> // to avoid a build-time dependency on org.graalvm.
> if
("com.oracle.svm.core.jdk.UnsupportedFeatureError".equals(e.getClass().getName()))
{
> return null;
> }
> throw e;
> }
> ```
>
> ### 2. The kill switch is baked in at build time
>
> ```java
> public static final boolean HIDDEN_CLASSES_DISABLED =
> SystemUtil.getBooleanSafe(PROPERTY_DISABLE, false);
> ```
>
> `static final`, documented as "evaluated once at class-init so hot paths
pay no property-lookup cost". Under native image this class is very likely
initialized at *build* time, so the value captured is the build JVM's, and
`-Dgroovy.hidden.classes.disable=true` at run time silently does nothing — the
one escape hatch a native user would reach for.
>
> This is the same trap I hit in GROOVY-12227: I ended up evaluating the
equivalent check per link rather than caching it in a static, because a
build-time-initialized class bakes in the wrong answer (the image-code property
reports `buildtime` there, not `runtime`).
>
> ### Possibly one fix for both
>
> If `isEnabled()` did a per-call check that also returned `false` when
>
`"runtime".equals(System.getProperty("org.graalvm.nativeimage.imagecode"))`,
then the native path would never attempt the definition at all, the kill switch
would work at run time, and (1) becomes belt-and-braces rather than
load-bearing. If the hot-path cost of the property read is the concern, a
non-final holder initialised on first *use* rather than at class-init keeps
both properties.
>
> ### Caveat on incidence
>
> I have not observed (1) fire in practice — the coercion I tested (`[run: {
... }] as Runnable`) goes through `java.lang.reflect.Proxy` and works natively
on both master and this branch, so it does not reach `HiddenClassDefiner`. This
is from reading the code plus confirming the class hierarchy, not from a
reproduced failure. Worth a targeted test if you think the proxy/reflector
paths are reachable in a native image.
> Introduce hidden class support
> ------------------------------
>
> Key: GROOVY-12223
> URL: https://issues.apache.org/jira/browse/GROOVY-12223
> Project: Groovy
> Issue Type: Improvement
> Reporter: Daniel Sun
> Priority: Major
>
> h2. Background
> Groovy generates many short-lived synthetic classes at runtime, including:
> * map/interface proxies ({{ProxyGeneratorAdapter}})
> * reflection dispatch helpers ({{Reflector}} / {{ReflectorLoader}})
> * per-class meta-method artifacts ({{ClassLoaderForClassArtifacts}})
> Today these are defined with {{ClassLoader#defineClass}} as ordinary *named*
> classes. That has three practical downsides:
> # *Name pollution* — the synthetic types are discoverable via
> {{Class.forName}} / {{ClassLoader#loadClass}}.
> # *Metaspace pressure* — their lifetime is tied to the defining class loader;
> long-running applications that generate many artifacts retain them until the
> loader itself is collected.
> # *Access friction* — without nest membership, generated code cannot share
> private access with the host class the way a true nestmate can.
> JDK 15 introduced *hidden classes* ([JEP 371|https://openjdk.org/jeps/371]):
> classes defined through {{Lookup#defineHiddenClass}} that are
> non-discoverable by name, may join an access-control nest ({{NESTMATE}}), and
> may be unloaded independently of the defining loader when not marked
> {{STRONG}}.
> Groovy 6 requires JDK 17+, so the API is always present on supported runtimes.
> h2. Proposal
> Centralise hidden-class definition behind a single utility and prefer it for
> the dynamic class-generation sites listed above, with a transparent fallback
> to the existing {{ClassLoader#defineClass}} path.
> h3. New API
> {{org.apache.groovy.util.HiddenClassDefiner}} — the only call-site that
> invokes {{Lookup#defineHiddenClass}}:
> * {{defineHiddenClass(lookup, bytes, initialize, nestmate, strong)}} — full
> control
> * {{defineNestmateClass(lookup, bytes, initialize)}} — nestmate + weak
> lifecycle (default for proxies / reflectors / artifacts)
> * {{defineStrongHiddenClass(lookup, bytes, initialize)}} — non-discoverable,
> loader-tied lifetime
> * helpers: {{privateLookupIn(hostClass)}}, {{findConstructor(hiddenClass,
> ...parameterTypes)}}
> Kill-switch (evaluated once at class-init for hot-path cost):
> {noformat}
> -Dgroovy.hidden.classes.disable=true
> {noformat}
> When disabled (or when private lookup / definition fails), callers fall back
> to defining a normal visible class.
> h3. Integration points
> || Site || Nest host || Preferred options || Fallback ||
> | {{ClassLoaderForClassArtifacts#define}} | target (klazz) | nestmate, weak |
> {{ClassLoader#defineClass}} + protection domain |
> | {{ProxyGeneratorAdapter}} | non-{{Object}} superclass if present; else
> {{ProxyGeneratorAdapter}} | nestmate, weak | {{InnerLoader#defineClass}} |
> | {{ReflectorLoader#defineClass}} | {{Reflector}} | nestmate, weak |
> {{ClassLoader#defineClass}} + protection domain |
> Behaviour for callers of these generators is unchanged: proxies still
> implement the requested interfaces, reflectors still dispatch, artifacts
> still construct. The only observable differences when the hidden path
> succeeds are the synthetic name form (contains {{/}}) and {{Class#isHidden()
> == true}}.
> h2. Benefits
> * Non-discoverable synthetic types (cleaner class-space / tooling view).
> * Nestmate private access where the nest host can be opened for private
> lookup.
> * Eager unloading of weak hidden classes reduces long-run metaspace retention
> for short-lived proxies and artifacts.
> * One policy / upgrade point if future JDKs add further
> {{Lookup.ClassOption}} values.
> h2. Compatibility
> * Default-on when the JVM can obtain a full-privilege lookup for the chosen
> nest host; silent fallback otherwise (e.g. sealed / unopened module packages).
> * Opt-out: {{-Dgroovy.hidden.classes.disable=true}}.
> * No public language-surface change; no change to successful proxy /
> reflector / artifact *behaviour*, only to how the {{Class}} is defined.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
