GROOVY-12151 closure packing with per-class dispatch tables and full MOP integration (GEP-27 S1)#2709
GROOVY-12151 closure packing with per-class dispatch tables and full MOP integration (GEP-27 S1)#2709paulk-asert wants to merge 3 commits into
Conversation
paulk-asert
changed the title
1408a63 to
2c94089
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2709 +/- ##
==================================================
+ Coverage 69.1087% 69.1628% +0.0542%
- Complexity 34271 34452 +181
==================================================
Files 1537 1542 +5
Lines 129483 130252 +769
Branches 23552 23735 +183
==================================================
+ Hits 89484 90086 +602
- Misses 31969 32064 +95
- Partials 8030 8102 +72
🚀 New features to boost your workflow:
|
2c94089 to
1e9460c
Compare
paulk-asert
changed the title
c9f5f77 to
393a578
Compare
…les and full MOP integration Under @PackedClosures (dynamic trust path) or -Dgroovy.target.closure.pack=true under @CompileStatic (where the type checker PROVES delegate-independence), an eligible closure literal's body is hoisted into a synthetic private method on the enclosing class and the literal is replaced by a shared PackedClosure adapter — removing the per-closure generated class (and the nested $_closure1$_closure2 name explosion) while the value stays a real groovy.lang.Closure. Ineligible closures (escaping, delegate-dependent, default-param, nested-in-generated-function) keep their class exactly as today; opt-out via @PackedClosures(mode = DISABLED).
One branch to an out-of-line callGeneral keeps the hot lane small (the argument-wrapping fallbacks allocate, so they leave the lane) and its standalone compilation lean. Assessed against Grails-shaped packed workloads (findAll/collect/groupBy pipelines, map-iteration validation): no measurable in-situ effect -- per-dispatch cost is already invisible under real body work -- and ~5-9% on the adverse non-capturing microbenchmark rows with the capturing rows unchanged. Kept as lane hygiene; further lane tuning is demonstrably not worth pursuing.
…h opt-in default true)
393a578 to
dd8f600
Compare
✅ All tests passed ✅🏷️ Commit: dd8f600 Learn more about TestLens at testlens.app. |
| // zero-parameter form ({ -> ... }), which must report arity 0 (GString's writer-vs-call | ||
| // branch and SAM matching key on it), exactly as its generated class would. | ||
| Parameter[] declared = expression.getParameters(); | ||
| boolean implicitParam = (declared != null && declared.length == 0); |
There was a problem hiding this comment.
what is declared==null? Why not boolean implicitParam = (declared == null || declared.length == 0);
| // types, so the body's load then carries the type the use sites were compiled | ||
| // against — where a plain Object parameter loads unverifiably wide (the classed | ||
| // emission gets the equivalent cast on its shared-Reference dereference). | ||
| ClassNode capturedType = typedBody ? capturedTypes.get(cn) : ClassHelper.OBJECT_TYPE; |
There was a problem hiding this comment.
I am using this line to attach the comment, but it is not limited to this. I see here still a mix of static and dynamic compiler code. For me the dynamic case should be the general one and static compilation should be a specialization, separated from the dynamic code generator. Why like this? Because if we change something for static compilation it is in one class and if we change something for dynamic compilation it is in another class. Andy dynamic>static because that is how we do it in other places as well, just trying to keep the style.
| ClassNode capturedType = typedBody ? capturedTypes.get(cn) : ClassHelper.OBJECT_TYPE; | ||
| Variable v = capturedVars.get(cn); | ||
| if (typedBody && v instanceof org.codehaus.groovy.ast.ASTNode) { | ||
| Object inferred = ((org.codehaus.groovy.ast.ASTNode) v) |
There was a problem hiding this comment.
2 Points here... Why the fully qualified name? (This is in other places as well) and can we not use x instanceof T t for this? It is a minor point. Just mentioning it because the long lines stick out.
| if (implicitParam) pc = pc + "$FixedIt"; | ||
| else if (arity <= 4 && !varargShape) pc = pc + "$Fixed" + arity; | ||
| else pc = pc + "$FixedN"; | ||
| mv.visitTypeInsn(NEW, pc); |
There was a problem hiding this comment.
should we start supporting default parameters, this kind of logic will no work. Not a blocker, just a note.
| os.box(); | ||
| } | ||
| mv.visitInsn(AASTORE); | ||
| os.remove(3); |
There was a problem hiding this comment.
I don`t think we need "os" here, except for loading the variable and boxing it. But you could directly remove 1 after that and otherwise remove the os-code. That is only 4 lines of code less of course. Just irritates me a bit the way it is here.
| typesDescriptor.append(")V"); | ||
| mv.visitLdcInsn(new org.objectweb.asm.ConstantDynamic("paramTypes", "[Ljava/lang/Class;", | ||
| new org.objectweb.asm.Handle(org.objectweb.asm.Opcodes.H_INVOKESTATIC, DISPATCHER_TYPE, "paramTypes", | ||
| "(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/Class;Ljava/lang/String;)[Ljava/lang/Class;", |
There was a problem hiding this comment.
We should consider the interface surface with bytecode. Right now we have SBC, DTT and IndyInterface mostly, but not only. I would prefer this in a method in IndyInterface, to have a central point of methods we have to keep caring about. The same about the classes we expose in bytecode... would it be a problem to have a closureFactory on one of the already exposed classes instead? I mean to produce instances of Bundle, PackedClosure, PackedClosure.Fixed and so on. It is slightly less efficient I imagine, but I doubt the impact would be major compared to the object creation cost.
GROOVY-12151: closure packing (GEP-27 S1) with per-class dispatch tables and full MOP integration
Under
@PackedClosures(dynamic trust path) or-Dgroovy.target.closure.pack=trueunder@CompileStatic(where the type checker PROVES delegate-independence), an eligible closure literal's body is hoisted into a synthetic private method on the enclosing class and the literal is replaced by a sharedPackedClosureadapter — removing the per-closure generated class (and the nested$_closure1$_closure2name explosion) while the value stays a realgroovy.lang.Closure. Ineligible closures (escaping, delegate-dependent, default-param, nested-in-generated-function) keep their class exactly as today; opt-out via@PackedClosures(mode = DISABLED).Dispatch architecture
Each hosting class gets compiler-generated dispatch tables: a general
$packedDispatch$(int id, Object[])covering every hoisted target, plus array-free per-arity tables ($packedDispatch1$/$packedDispatch2$) for targets taking one or two values beyond the receiver (captures count as values — the overwhelmingly common shapes), each case casting to the target's declared parameter types and invoking directly. Tables observe the JIT's inline budget via two-level chunking. Oneinvokedynamicaccessor per class links all three throughGeneratedDispatcher.bootstrapinto a constantBundle, lazily on first adapter creation; each literal's parameter-typeClass[]is a condy constant resolved once per site. The whole chain is ordinary bytecode — no reflection, and no per-instanceMethodHandle(not constant-foldable, hence many times slower). The id binds the exact hoisted method, so an inherited packed closure can never misdispatch to a subclass's same-named method.MOP integration (from the dev@ closure-dispatch discussion)
Closure.call's cache (GROOVY-12165): the direct path is taken only when the instance's metaclass is stock and no category is active; anything MOP-relevant routes throughinvokeMethod. Interception via a per-instance metaclass is honoured identically on the dynamic and Java/GDK paths (pinned by tests, matching generated closure classes).PackedClosureMetaClassis the registered stock metaclass for the adapter (the analog ofClosureMetaClassfor generated closures): reflection-freecall/doCalldispatch,respondsTofaithful to the instance's declared parameter types, and the same category stance asClosureMetaClass(which also fixes a latent packed-vs-classed divergence underuse{}).@CompileStaticclosure class.Documented residual semantic differences (all packed closures share one adapter class): per-literal class identity is gone (
closure.getClass()distinctness, serialization); class-levelPackedClosure.metaClasschanges are global to packed closures (per-instancesetMetaClassis fully honoured via the guard).JPMS note
Packed dispatch uses the capability model Java lambdas use: the hosting class's own
invokedynamicbootstrap captures its lookup, and handing the closure across a module boundary is the grant — so packed closures work under strict encapsulation with noopens, where reflective dispatch of closure classes does not. This moves enforcement from invocation time to creation time, deliberately.Performance (JMH
ClosurePackBench, 2 forks, ops/ms, vs current master incl. GROOVY-12164/12165)each)collect)Capturing shapes are faster packed — and the win holds under megamorphism; the tight non-capturing
collecttrails, the cost being the MOP-transparency guard (loop-invariant loads the JIT hoists when the chain inlines; the same checks the classic call-site caches make) plus the per-creation adapter instance (a per-literal singleton is unsound:Closurecarries mutable per-instance state). Allocation is at or below the closure-class baseline on capturing shapes (e.g.times-accumulator 160 vs 368 bytes/call) and within one adapter instance of it on non-capturing ones. Earlier figures posted in this thread predate the MOP guard and the 12164/12165 baseline improvements; this table supersedes them.In-situ check: Grails-shaped workloads
To verify the micro table translates to realistic code, a Grails-patterned fixture (mirroring the
GrailsWorkloadBenchshapes: an employeefindAll/collect/groupBy/collectEntriespipeline and a map-iteration validation cycle with a written-capture accumulator,@PackedClosurestrust path — every literal packs) was measured classes-vs-packed on the same build, interleaved rounds:(Both compilations produce identical outputs for these workloads, so the comparison is two implementations of the same computation; the broader behavioural-equivalence evidence is the corpus parity and test suite below.) The blend is the point: realistic pipelines mix capturing, multi-argument and non-capturing dispatches with real body work, and the mix nets strongly positive — the micro table's adverse non-capturing row is real but dilutes out in situ (further lane micro-tuning was assessed against these shapes and found to have no in-situ effect, so it is deliberately not pursued).
Code size on the 29-closure measurement corpus: 30 -> 2 classes, -73% bytecode bytes; corpus transcripts byte-identical with the flag on and off; full suite 15,780 green.
Commits
Packing base (capability analysis, hoisting, escape/decline rules,
@PackedClosuresmodes) -> per-class-table optimisation (general + per-arity tables, condy parameter types,ClosurePackBench) -> MOP integration slices (dispatch guard; doCall alignment;PackedClosureMetaClass). Rebased on master after GROOVY-12165 merged; theClosure.javadelta of this PR is a single line (the stock-metaclass set gainsPackedClosureMetaClass).