GROOVY-12223: Introduce hidden class support - #2755
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: d64841c | Previous: 2f85f42 | Ratio |
|---|---|---|---|
org.apache.groovy.bench.AryBench.java ( {"n":"100"} ) |
0.020284455721950537 ms/op |
0.011410769075782604 ms/op |
1.78 |
org.apache.groovy.bench.CalibrationBench.memoryPointerChase |
994.2828699711587 us/op |
573.8179240262663 us/op |
1.73 |
This comment was automatically generated by workflow using github-action-benchmark.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2755 +/- ##
==================================================
+ Coverage 69.9742% 70.0253% +0.0510%
- Complexity 35502 35607 +105
==================================================
Files 1557 1559 +2
Lines 131667 131821 +154
Branches 24166 24207 +41
==================================================
+ Hits 92133 92308 +175
+ Misses 31193 31162 -31
- Partials 8341 8351 +10
🚀 New features to boost your workflow:
|
JMH summary — classic (commit
|
| Group | Speedup | Calibrated | n |
|---|---|---|---|
| bench | 0.940 × | 0.995 × | 99 |
| core | 1.024 × | 1.054 × | 83 |
| grails | 0.994 × | 0.979 × | 80 |
Runner calibration (this run vs baseline hardware): bench 0.95× (26 rulers) · core-ag 0.98× (3 rulers) · core-hz 0.96× (3 rulers) · grails-ad 1.10× (3 rulers) · grails-ez 0.95× (3 rulers)
Baseline: dev/bench/jmh/<part>/classic/data.js on gh-pages, trailing 90 days. Daily dashboard · Per-suite raw data
JMH summary — indy (commit
|
| Group | Speedup | Calibrated | n |
|---|---|---|---|
| bench | 0.984 × | 1.005 × | 99 |
| core | 2.995 × | 2.874 × | 83 |
| grails | 4.198 × | 4.172 × | 80 |
Runner calibration (this run vs baseline hardware): bench 0.98× (26 rulers) · core-ag 1.12× (3 rulers) · core-hz 0.95× (3 rulers) · grails-ad 0.91× (3 rulers) · grails-ez 1.10× (3 rulers)
Baseline: dev/bench/jmh/<part>/indy/data.js on gh-pages, trailing 90 days. Daily dashboard · Per-suite raw data
70b0e73 to
a853b33
Compare
There was a problem hiding this comment.
- HiddenClassDefiner contains method used only in tests
- ProxyGeneratorAdapter and ReflectorLoader seems to duplicate work already done in HiddenClassDefiner, while still using HiddenClassDefiner
But the big point for me is MethodHandles.lookup(). It has the javadoc:
Returns a lookup object with full capabilities to emulate all supported bytecode behaviors of the caller. These capabilities include full privilege access to the caller. Factory methods on the lookup object can create direct method handles for any member that the caller has access to via bytecodes, including protected and private fields and methods. This lookup object is created by the original lookup class and has the ORIGINAL bit set. This lookup object is a capability which may be delegated to trusted agents. Do not store it in place where untrusted code can access it.
This method is caller sensitive, which means that it may return different values to different callers. In cases where MethodHandles.lookup is called from a context where there is no caller frame on the stack (e.g. when called directly from a JNI attached thread), IllegalCallerException is thrown. To obtain a lookup object in such a context, use an auxiliary class that will implicitly be identified as the caller, or use publicLookup() to obtain a low-privileged lookup instead.
So if I create a Lookup for HiddenClassDefinerTest in HiddenClassDefinerTest I will get full access. But if I create that in the same class for java.lang.String, I will not get full access. In practice this usually means that unless the class creates the lookup (for example during invokedynamic) it is useless for many things. So if you need private module access, then MethodHandles.lookup() is most likely not the way to go.
Which means I don`t think that the intend of HiddenClassDefiner.privateLookupIn and usuability go hand in hand here.
a853b33 to
58063be
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Introduces JEP 371 hidden-class support across Groovy’s dynamic class generators to reduce class-space pollution and improve unloading behavior, with comprehensive new tests validating hidden vs fallback paths.
Changes:
- Added
HiddenClassDefinerutility to define hidden nestmates with package alignment and a kill-switch property. - Updated
ReflectorLoader,ClassLoaderForClassArtifacts, andProxyGeneratorAdapterto prefer hidden-class definition with transparent fallback. - Added/expanded tests covering hidden-class behavior and name-generation rules.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/org/apache/groovy/util/HiddenClassDefiner.java | New central API for defining hidden classes (nestmate/strong), with package alignment + soft/strict entry points. |
| src/main/java/org/codehaus/groovy/runtime/metaclass/ReflectorLoader.java | Prefer defining generated Reflectors as hidden nestmates of Reflector, with fallback to defineClass. |
| src/main/java/org/codehaus/groovy/reflection/ClassLoaderForClassArtifacts.java | Prefer defining per-class artifacts as hidden nestmates of the target class; adds name-generation/docs refactor. |
| src/main/java/org/codehaus/groovy/runtime/ProxyGeneratorAdapter.java | Prefer hidden-class proxies when safe; adds host-candidate selection and isProxyHidden() API. |
| src/test/groovy/org/apache/groovy/util/HiddenClassDefinerTest.groovy | New unit tests for hidden-class definition, package alignment, and constructors. |
| src/test/groovy/org/codehaus/groovy/runtime/metaclass/ReflectorLoaderTest.groovy | New tests covering hidden-class path for ReflectorLoader. |
| src/test/groovy/org/codehaus/groovy/reflection/ClassLoaderForClassArtifactsTest.groovy | New tests validating hidden artifact definitions, constructor helper, and naming constraints. |
| src/test/groovy/groovy/util/ProxyGeneratorAdapterTest.groovy | Adds hidden-class specific proxy tests and fixes Map.Entry reference. |
| public Constructor defineClassAndGetConstructor( | ||
| final String name, | ||
| final byte[] bytes, | ||
| final Class<?>... parameterTypes) { | ||
| try { | ||
| final Class<?> cls = define(name, bytes); | ||
| return cls.getDeclaredConstructor(parameterTypes); | ||
| } catch (NoSuchMethodException e) { | ||
| return null; | ||
| } | ||
| } |
| public Class define(final String name, final byte[] bytes) { | ||
| final Class<?> host = klazz.get(); | ||
| if (host != null) { | ||
| final Class<?> hidden = HiddenClassDefiner.tryDefineNestmate(host, bytes, false); | ||
| if (hidden != null) { | ||
| return hidden; | ||
| } | ||
| } | ||
|
|
||
| // Fallback: visible class with the target's protection domain | ||
| final Class<?> cls = defineClass( | ||
| name, bytes, 0, bytes.length, | ||
| host != null ? host.getProtectionDomain() : null); |
| void testCreateClassNameUniquenessAndJavaPrefix() { | ||
| def loader = new ClassLoaderForClassArtifacts(Host) | ||
| String first = loader.createClassName('m') | ||
| String second = loader.createClassName('m') | ||
| assertTrue(first.contains(Host.name)) | ||
| assertTrue(first != second || second.endsWith('$0') || second.contains('$')) | ||
|
|
||
| def javaLoader = new ClassLoaderForClassArtifacts(String) |
| Class<?> proxyCls = adapter.proxy(map).getClass() | ||
| assertTrue(proxyCls.isHidden(), 'Proxy class must report isHidden() == true') | ||
| assertThrows(ClassNotFoundException) { | ||
| Class.forName(proxyCls.getName()) | ||
| } |
| @SuppressWarnings("unchecked") | ||
| public static <T> Constructor<T> findConstructor( | ||
| final Class<T> type, | ||
| final Class<?>... parameterTypes) { | ||
| try { | ||
| return type.getDeclaredConstructor(parameterTypes); | ||
| } catch (NoSuchMethodException e) { | ||
| throw new IllegalStateException( | ||
| "Class " + type.getName() + " is missing the expected constructor", e); | ||
| } | ||
| } |
| * @param name the fully qualified binary name of the Reflector class | ||
| * @param bytecode the bytecode of the Reflector class | ||
| * @param domain the protection domain for the class | ||
| * @param domain the protection domain for the fallback visible-class | ||
| * definition; not used when the hidden-class path succeeds |
58063be to
c35ffe8
Compare
|
@blackdrag Thanks for the careful review — those points were well taken. Here is how the current design addresses them. Test-only API on
|
c35ffe8 to
a90cc7d
Compare
| final Lookup lookup, | ||
| final byte[] bytes, | ||
| final boolean initialize) { | ||
| if (HIDDEN_CLASSES_DISABLED || lookup == null || bytes == null) { |
There was a problem hiding this comment.
Did I see it wrong or is this effectively called only with MethodHandles.lookup()? Does it really matter if it is one time HiddenClassDefiner and another time ReflectorLoader?
There was a problem hiding this comment.
You are not wrong that every production Lookup we pass in is obtained via MethodHandles.lookup() — we do not use another factory for that path.
What still matters is which class executes that call.
Lookup.lookupClass() becomes the nest host and fixes the hidden class’s defining loader, run-time package, and protection domain. So:
ReflectorLoader’sLOOKUP→ nestmates ofReflectorLoader(its package / loader)ProxyGeneratorAdapter’sLOOKUP→ nestmates ofProxyGeneratorAdapter- foreign hosts via
tryDefineNestmate(Class, …)→ nestmates of that user class (privateLookupIn)
Those are not interchangeable. Sharing only the define policy (package alignment, NESTMATE+weak, soft-fail) in HiddenClassDefiner does not make the lookups equivalent.
HiddenClassDefiner’s own LOOKUP is used only as the caller argument to privateLookupIn for the foreign-host overload — never as the nest host for those generators.
So: same API (lookup()), different ownership — and yes, that ownership still matters.
There was a problem hiding this comment.
My Point is that this code is always called from within the Groovy Runtime. Which means MethodHandles.lookup() will always return the same. It will be for the module the runtime is in and allow access to public methods in that module as well as accessible methods in other classes. And that later case is the potential problem, since if the runtime has no access to a different module the ProxyAdapter will not have that too. You can do that MethodHandles.lookup() anywhere in the Groovy runtime to get the same result. It also means the private lookup is restricted. What does it mean for nest mates? Since we need private access privileges for the target host class I have high doubts that this works all as intended in a module system. Just try to create a nest mate for String for example.
Looking at the usage I see ReflectorLoader. I found only a test, no actual code. Which actually means this class should be deprecated/removed.
ProxyGeneratorAdapter is used by InlinedASTCustomizerFactory and ProxyGenerator. InlinedASTCustomizerFactory is probably fine, but for ProxyGenerator: what if the interface is not accessible by the runtime? And I mean groovy can inspect the class, the methods are public? Map for example is no problem. It is in a different and restricted module, but Map itself is public API. The scenario I always keep in mind for such cases is that of 2 modules. Module A in Java, Module B in Groovy and Module C, the Groovy runtime. And B uses A. B has to give C access, but why should A give C access? And then all kinds of things may start failing. So I try to improve the code base step-by-step in a way that can handle such cases for the time when Groovy becomes a real module and has to handle Groovy programs in real modules.
Now you probably say something like that this is why we have the fallback. And you are right about that. But assume we are in the modules case, how useful is the nestmate variant then in the end? What would it cover outside of module B and C? And is that enough for us? If you think it is good enough feel free to resolve this conversation.
|
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:
|
|
@paulk-asert Thanks for capturing this — both points are real and we’ve addressed them in the current code. 1. Soft-fail under native imageAgreed that
2. Kill switch baked in at build timeAgreed. The old We removed that field.
So native users get a working escape hatch, and the soft-fail on Happy to adjust further if you hit a concrete native path that still reaches |
|
✅ All tests passed ✅🏷️ Commit: d64841c Learn more about TestLens at testlens.app. |
https://issues.apache.org/jira/browse/GROOVY-12223