[
https://issues.apache.org/jira/browse/GROOVY-12227?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18101550#comment-18101550
]
ASF GitHub Bot commented on GROOVY-12227:
-----------------------------------------
codecov-commenter commented on PR #2762:
URL: https://github.com/apache/groovy/pull/2762#issuecomment-5173465343
##
[Codecov](https://app.codecov.io/gh/apache/groovy/pull/2762?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
Report
:x: Patch coverage is `57.14286%` with `15 lines` in your changes missing
coverage. Please review.
:white_check_mark: Project coverage is 69.9715%. Comparing base
([`7a34d45`](https://app.codecov.io/gh/apache/groovy/commit/7a34d4507db3a588e9ae413231f26ba4b564355a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
to head
([`6464647`](https://app.codecov.io/gh/apache/groovy/commit/6464647da354b8c1832b4550db64f076bec8d57f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 5 commits behind head on master.
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/2762?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Patch % | Lines |
|---|---|---|
|
[...g/codehaus/groovy/runtime/GeneratedDispatcher.java](https://app.codecov.io/gh/apache/groovy/pull/2762?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fruntime%2FGeneratedDispatcher.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3J1bnRpbWUvR2VuZXJhdGVkRGlzcGF0Y2hlci5qYXZh)
| 54.5454% | [14 Missing and 1 partial :warning:
](https://app.codecov.io/gh/apache/groovy/pull/2762?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/2762?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #2762 +/- ##
==================================================
- Coverage 69.9738% 69.9715% -0.0023%
- Complexity 35505 35515 +10
==================================================
Files 1557 1557
Lines 131675 131705 +30
Branches 24169 24170 +1
==================================================
+ Hits 92138 92156 +18
- Misses 31193 31209 +16
+ Partials 8344 8340 -4
```
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/2762?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Coverage Δ | |
|---|---|---|
|
[...rg/codehaus/groovy/classgen/asm/ClosureWriter.java](https://app.codecov.io/gh/apache/groovy/pull/2762?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2Fasm%2FClosureWriter.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL2FzbS9DbG9zdXJlV3JpdGVyLmphdmE=)
| `89.7931% <100.0000%> (ø)` | |
|
[...org/codehaus/groovy/vmplugin/v8/IndyInterface.java](https://app.codecov.io/gh/apache/groovy/pull/2762?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fvmplugin%2Fv8%2FIndyInterface.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3ZtcGx1Z2luL3Y4L0luZHlJbnRlcmZhY2UuamF2YQ==)
| `83.5165% <100.0000%> (-0.4614%)` | :arrow_down: |
|
[...g/codehaus/groovy/runtime/GeneratedDispatcher.java](https://app.codecov.io/gh/apache/groovy/pull/2762?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fruntime%2FGeneratedDispatcher.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3J1bnRpbWUvR2VuZXJhdGVkRGlzcGF0Y2hlci5qYXZh)
| `57.6923% <54.5454%> (-29.2642%)` | :arrow_down: |
... and [6 files with indirect coverage
changes](https://app.codecov.io/gh/apache/groovy/pull/2762/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test
Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests,
report on failures, and find test suite problems.
- :package: [JS Bundle
Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save
yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
> GeneratedDispatcher: avoid runtime class definition so packed closures work
> in native images
> ---------------------------------------------------------------------------------------------
>
> Key: GROOVY-12227
> URL: https://issues.apache.org/jira/browse/GROOVY-12227
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> Packed closures (GROOVY-12151, GEP-27) cannot be used in a GraalVM native
> image. A class compiled with {{groovy.target.closure.pack=true}} links its
> dispatch tables on first closure creation, and that linkage fails inside an
> image — in two layers.
> Without agent-recorded metadata, the bootstrap's method lookup fails:
> {noformat}
> java.lang.BootstrapMethodError: java.lang.NoSuchMethodException:
> no such method: B.$packedDispatch$(int,Object[])Object/invokeStatic
> {noformat}
> With agent-recorded metadata the lookup succeeds, and the real wall appears —
> the bootstrap spins hidden classes at run time, which an image forbids:
> {noformat}
> Exception in thread "main" java.lang.BootstrapMethodError:
> java.lang.InternalError:
> com.oracle.svm.core.jdk.UnsupportedFeatureError: Classes cannot be defined at
> runtime
> by default when using ahead-of-time Native Image compilation. Tried to define
> class:
> B$$LambdadLbm0tKUrZ8kVMiSpulnyK
> {noformat}
> The failure mode is the problem as much as the failure: packing compiles
> cleanly, the image builds cleanly, and the process then dies on the first
> closure created. There is no diagnostic pointing at the flag that caused it.
> "Just don't pack in native images" is reasonable advice for some workloads
> (see Performance below), but it cannot be the whole answer: the compiler has
> no way to know a native image will be built later — the class files are
> identical either way — so the guidance cannot be enforced at compile time.
> The realistic path into this is enabling packing globally for its JVM benefit
> and adding a native build afterwards. That leaves only a better runtime error
> (same detection point, comparable effort, strictly less capability) or the
> fix below.
> h2. Reproducing
> {code:java}
> import groovy.transform.CompileStatic
> @CompileStatic
> class B {
> static int work(int n) {
> def xs = (1..n).toList()
> int total = 0
> total += xs.collect { int v -> v * 2 }.size()
> total += xs.findAll { int v -> v % 2 == 0 }.size()
> total += (xs.inject(0) { int a, int b -> a + b } as int)
> total
> }
> static void main(String[] args) { println "result=${work(20)}" }
> }
> {code}
> {noformat}
> JAVA_OPTS="-Dgroovy.target.indy=false -Dgroovy.target.closure.pack=true" \
> groovyc -cp groovy-callsite.jar B.groovy # emits a single class:
> B.class
> java -agentlib:native-image-agent=config-output-dir=cfg -cp
> "groovy.jar:groovy-callsite.jar:." B
> native-image --no-fallback -H:ConfigurationFileDirectories=cfg \
>
> -H:IncludeResources='META-INF/dgminfo|META-INF/groovy/.*|META-INF/services/.*'
> \
> -cp "groovy.jar:groovy-callsite.jar:." B
> ./b # fails as above
> {noformat}
> Reproduced with GraalVM CE 25.2.4 (native-image 25.0.4 — the first release
> whose Groovy substitution guard is fixed for Groovy 5+, see
> oracle/graal#13096; earlier releases fail before reaching this).
> Note that indy must already be off, with {{groovy-callsite}} on the *compile*
> classpath: Groovy's own {{IndyInterface}} dispatch uses mutable call sites,
> which native image does not support, and it fails upstream of anything here.
> So this ticket only affects users who have already crossed that larger hurdle.
> h2. Root cause
> {{GeneratedDispatcher.bootstrap}} does two things an image cannot support:
> # {{caller.findStatic(host, "$packedDispatch$", ...)}} — a *runtime*
> method-handle lookup, which under native image needs per-user-class
> reflection metadata. Groovy's own jar cannot ship that, so every user would
> need an agent run.
> # Three programmatic
> {{LambdaMetafactory.metafactory(...).getTarget().invokeExact()}} calls, each
> defining a hidden class at run time. Bytecode-level LMF invokedynamic is
> pre-processed at image build time and works fine; *programmatic* metafactory
> calls inside a bootstrap are runtime class definition, which is forbidden.
> The surrounding design is already image-friendly: the result is wrapped in a
> {{ConstantCallSite}} and never re-linked.
> h2. Proposed fix (prototyped)
> Two layered changes. JVM semantics are unchanged.
> *1. Constant bootstrap arguments (ClosureWriter).* Emit the three dispatch
> tables as {{CONSTANT_MethodHandle}} bootstrap arguments, as javac does for
> LMF's {{implMethod}}:
> {noformat}
> invokedynamic packedDispatchers()Object
> bsm = IndyInterface.packedDispatchers(Lookup, String, MethodType,
> MethodHandle, MethodHandle,
> MethodHandle)
> args = [ MH(invokestatic host.$packedDispatch$), MH(...1$), MH(...2$) ]
> {noformat}
> Constant-pool method handles are resolved by the VM (and pre-resolved at
> image build time), so linking needs no {{findStatic}} and no metadata.
> Verified: the tracing agent records *zero* {{packedDispatch}} entries for the
> new bytecode. The existing 3-arg bootstrap is kept for class files emitted by
> earlier 6.0 snapshots.
> *2. Method-handle bundle fallback (GeneratedDispatcher).* On a regular JVM
> the {{LambdaMetafactory}} hidden-class adapters are kept — their interface
> call inlines under the JIT, which is the documented rationale. Where classes
> cannot be defined at run time, the tables are wrapped in
> method-handle-invoking adapters instead: ordinary Java lambdas of
> {{GeneratedDispatcher}} itself, whose bytecode-level LMF sites are
> AOT-compiled into the image, so nothing is defined at run time.
> Detection is
> {{"runtime".equals(System.getProperty("org.graalvm.nativeimage.imagecode"))}},
> evaluated *per link* rather than cached in a static — under native image the
> class may be build-time initialized, where the property reports
> {{buildtime}}, and caching would bake the wrong answer into the image heap. A
> catch-based fallback around the metafactory calls covers AOT runtimes the
> property probe misses.
> One subtlety: dispatch targets may throw checked exceptions the dispatcher
> interfaces do not declare, and the hidden-class path propagates them
> transparently. The wrapper path rethrows via {{UncheckedThrow}} to match; a
> dedicated test covers this.
> h2. Opt-in / opt-out / permanent?
> Permanent and automatic — no user-facing flag:
> * JVM behaviour is byte-identical; the fallback engages only where the
> hidden-class path *cannot work*. A flag would choose between "works" and
> "crash".
> * The gate already exists one level up: packing is itself opt-in
> ({{groovy.target.closure.pack}} / {{@PackedClosures}}) and experimental under
> GEP-27.
> * {{-Dgroovy.packed.dispatch.handles=true}} exists as a *diagnostic* knob
> only, forcing the wrapper path on a JVM so CI can assert parity without a
> native build.
> h2. Correctness verification
> ||check||result||
> |{{PackedDispatcherHandleBundleTest}} (new): parity across all three dispatch
> shapes and the GDK; undeclared checked-exception propagation|2/2 pass|
> |existing packed/closure suites ({{PackedClosuresTransformTest}},
> {{ClosurePackCapabilityTest}}, {{PackedClosureBoundariesTest}},
> {{PackedClosureMetaClassTest}}, {{PackedClosureDebugMetadataTest}},
> {{ClosureAndInnerClassNodeStructureTest}}, {{ClosureCallMopGuardTest}})|57
> tests green|
> |repro, unpatched master, native|{{Classes cannot be defined at runtime}}|
> |repro, patched, native|runs correctly, exit 0, no exceptions|
> |agent metadata for the dispatcher|0 entries (was: per-class method
> registrations)|
> h2. Performance
> Measured on GraalVM CE 25.2.4 / native-image 25.0.4 across three workloads: a
> 5-closure program, a 120-closure program, and a dispatch-heavy loop. *The
> honest summary: this fix is cheap, but packing in a native image is a
> startup-versus-dispatch trade that only pays off above a closure-class
> threshold. This ticket makes that choice available rather than fatal; it does
> not make packing universally advisable.*
> *Cost of the fix itself.* Isolated by building the same packed sources
> against unpatched master (which builds, then fails at run time) and against
> the patch:
> ||build||image size||delta||
> |unpacked|29,138,784 bytes| |
> |packed, unpatched master (does not run)|30,528,208 bytes|+1.39 MB|
> |packed, patched (runs)|30,643,936 bytes|+1.51 MB|
> So the fix adds *~113 KB (+0.4%)*. The larger +1.39 MB is pre-existing
> packing overhead, not introduced here: {{jdk.internal.classfile.impl}} (561
> KiB) is pulled in by the {{LambdaMetafactory}} reference that master already
> had.
> *Image size.* Packing makes the image ~1.4 MB *bigger*, not smaller. The
> 121-to-1 class collapse does not translate into image savings — native image
> already handles many small classes efficiently, and the {{PackedClosure}}
> runtime is a fixed cost. (An earlier draft of this ticket asserted the
> opposite; the measurements above correct it.)
> The overhead is *fixed, not proportional* — identical at both ends of the
> range, which matters for the threshold below:
> ||closure classes||packing delta||
> |5|+1.44 MB|
> |120|+1.44 MB|
> *Steady-state dispatch.*
> ||workload||native||JVM||
> |unpacked|20–21 ms|18–21 ms|
> |packed|*35–41 ms (~2x slower)*|19–20 ms|
> |packed, MH path forced on JVM|—|20.2–20.4 ms (~7% over hidden-class)|
> The JVM is unaffected: the wrapper costs ~7% there and is not used anyway. In
> an image there is no JIT to fold the {{invokeExact}}, so the indirection
> shows up in full on this deliberately dispatch-heavy loop; ordinary code will
> see less.
> *Startup, and the crossover.* Packing's win comes from eliminating class
> initializations, so it scales with how many closure classes collapse. Process
> wall-clock, median of 41 runs, A/B interleaved to cancel machine drift:
> ||closure classes||unpacked||packed||change||
> |5 -> 1|13.6 ms|13.7 ms|-1.4% (noise; p5 12.7 vs 12.8)|
> |120 -> 1|23.9 ms|*13.2 ms*|*+44.8%*|
> In-process work time was identical (6 ms) in the 5-closure case, confirming
> that difference is image init rather than dispatch.
> *What this means in practice.* The two effects run opposite ways, and the
> fixed +1.44 MB sits on the cost side regardless:
> * *Few closure classes* — no startup win to collect, so you pay the dispatch
> overhead and the image size for nothing. Do not pack.
> * *Many closure classes* — class-init savings dominate and startup nearly
> halves, which is the metric native images exist to optimize. Packing is worth
> it unless the process is long-lived and dispatch-bound.
> The threshold is workload-specific; the honest advice is to measure both ways
> rather than assume. Note also that the dispatch cost is provisional — see the
> {{tableSwitch}} follow-up below, which would remove the cost side of this
> trade entirely.
> h2. Follow-ups (not this ticket)
> * *Drop the LMF reference on the native path* so
> {{jdk.internal.classfile.impl}} (561 KiB) falls out of the image, removing
> most of packing's size overhead.
> * *A class-free dispatch that is not method-handle-based* — for example
> {{MethodHandles.tableSwitch}}, or a generated switch — to close the ~2x
> steady-state gap. That would turn native packing from a trade into a win.
> * *Ship Groovy's own reachability metadata* (VMPluginFactory reflection,
> {{dgm$NNN}} classes, {{META-INF/dgminfo}}) in the jar, so the agent step
> disappears for everyone. App-independent and pre-existing; unrelated to
> packing.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
