GROOVY-12151: hoist eligible closure bodies to a shared adapter @Pack…#2692
GROOVY-12151: hoist eligible closure bodies to a shared adapter @Pack…#2692paulk-asert wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2692 +/- ##
==================================================
+ Coverage 69.1077% 69.1286% +0.0209%
- Complexity 34235 34317 +82
==================================================
Files 1537 1540 +3
Lines 129356 129706 +350
Branches 23503 23581 +78
==================================================
+ Hits 89395 89664 +269
- Misses 31941 31984 +43
- Partials 8020 8058 +38
🚀 New features to boost your workflow:
|
paulk-asert
force-pushed
the
groovy12151
branch
2 times, most recently
from
f257944 to
2a11bb5
Compare
| * @see PackedClosures#mode() | ||
| */ | ||
| @Incubating | ||
| public enum PackMode { |
There was a problem hiding this comment.
suggestion: make this part of PackedClosure
| if (mode == PackMode.DISABLED) return PackStrategy.FULL_CLASS; | ||
| boolean annotated = (mode != null); // LENIENT/WARN/STRICT all opt in (DISABLED handled above) | ||
| boolean triggered; | ||
| if (isStaticCompilation()) { |
There was a problem hiding this comment.
You have a class and method that is special only for the subclass, while basically encoding the name and resulting logic of that subclass in the parent class. I think the name should be more general and the logic should be moved into that method call. For example: triggered = annotated || triggerClosurePacking(expression) This class would then return false and StaticTypesClosureWriter would return the SystemUtil and isDelegateIndependent call. Also isDelegateIndependent exist only for this purpose here, so it should be moved as well
| // be Reference-threaded if it is written ANYWHERE in the enclosing method, not just inside the | ||
| // closure -- e.g. `def fib; fib = { n -> ... fib(n-1) ... }` writes fib after the closure is | ||
| // constructed, so a by-value capture would see the stale (null) value. | ||
| private final Map<MethodNode, Set<String>> writtenNamesByMethod = new HashMap<>(); |
There was a problem hiding this comment.
field in between code. You should maybe add to avoid this as rule to your AI or it is going to continue doing this.
| private static final Set<String> FORBIDDEN_CLOSURE_CALLS = | ||
| new HashSet<>(java.util.Arrays.asList("getOwner", "getDelegate", "getThisObject", "getDirective", | ||
| "getResolveStrategy", "setDelegate", "setDirective", "setResolveStrategy", | ||
| "getMaximumNumberOfParameters", "getParameterTypes", "getMetaClass", "setMetaClass")); |
There was a problem hiding this comment.
how about invokeMethod, get/setProperty?
| // static writer needs (DIRECT_METHOD_CALL_TARGET, inferred types) -- including types that were | ||
| // only inferred (@ClosureParams, implicit it), which arrive for free via checkcasts. Written | ||
| // captures ride the holder machinery, so their bodies compile statically too. | ||
| hoisted.putNodeMetaData(STATIC_COMPILE_NODE, typedBody ? Boolean.TRUE : Boolean.FALSE); |
There was a problem hiding this comment.
This is a strong indicator for code that should be in StaticTypesClosureWriter. You should investigate if this method and actually all the methods added in this class really should be in this class or in StaticTypesClosureWriter
| os.push(ClassHelper.boolean_TYPE); | ||
| mv.visitMethodInsn(INVOKESPECIAL, pc, "<init>", | ||
| "(Ljava/lang/Object;Ljava/lang/String;[Ljava/lang/Object;[Ljava/lang/Class;Z)V", false); | ||
| os.replace(ClassHelper.CLOSURE_TYPE, 5); // owner, name, captured[], paramTypes[], strict -> Closure |
There was a problem hiding this comment.
OperandStack is not merely for book keeping. It's main purpose is to support casting, provide a more easy way to handleswap with different types, that you do not really know and with casts for types you do not know. If all you do is creating some Object[] and load references into it then I doubt it is needed. So my question here is: If you remove the usages of os, what would break?
| } | ||
| // strict guard only for the dynamic trust path; a statically PROVEN delegate-independent | ||
| // closure stores-and-ignores a caller-set delegate, like a @CompileStatic closure class. | ||
| mv.visitInsn(isStaticCompilation() ? org.objectweb.asm.Opcodes.ICONST_0 : org.objectweb.asm.Opcodes.ICONST_1); |
There was a problem hiding this comment.
Again, this looks like mixing up responsibilities. Plus, why is strict mode SC only and default for SC? Why is not default in general?
|
|
||
| /** S3 — neither escapes nor needs an object; elide it entirely. */ | ||
| ELIDE | ||
| } |
There was a problem hiding this comment.
Are the last two even used?
| int m(List<Integer> xs) { need(xs.collect { Integer it -> it * 2 }) } | ||
| }''' | ||
| assertEquals(['Z'], generatedClassNames(passedAsArg)) // closure packs, no class remains | ||
| assertEquals(12, instance(passedAsArg + '\n new Z()').m([1, 2, 3])) |
There was a problem hiding this comment.
why is it compilestatic only and if it is, then we should consider splitting the test in a static version and non-static version. Of course same for the other tests in here
| try { | ||
| return doCall(args); | ||
| } catch (InvokerInvocationException e) { | ||
| org.apache.groovy.internal.util.UncheckedThrow.rethrow(e.getCause()); |
There was a problem hiding this comment.
Why can this Exception happen and if it does, why do we have to unpack it here? This looks kind of wrong to me
| all[captured.length + i] = (i < provided.length) ? provided[i] : null; | ||
| } | ||
| return invokeHoisted(all); | ||
| } |
There was a problem hiding this comment.
This doCall method is an inlining nightmare. The idea of doCall was to provide a fast way for the MOP to shortcut the call to doCall without ballast in doCall itself. a dynamic call to call will not even dispatch to call and instead go directly got to doCall in case of a GeneratedClosure. There is even a special MetaClass for this with a simplified method selection. And all of this static available information in here is now done at runtime?
| return invokeHoisted(all); | ||
| } | ||
|
|
||
| private transient java.lang.reflect.Method target; |
There was a problem hiding this comment.
again field/constant randomly in the class body ;)
| private static void rebindCaptured(final Statement body, final Map<String, Parameter> capturedParams) { | ||
| body.visit(new CodeVisitorSupport() { | ||
| @Override public void visitVariableExpression(final VariableExpression ve) { | ||
| Parameter p = capturedParams.get(ve.getName()); | ||
| if (p != null) { | ||
| ve.setAccessedVariable(p); | ||
| // read-only captures become plain by-value params; written captures stay | ||
| // closure-shared so the ASM generator routes them through the holder | ||
| ve.setClosureSharedVariable(p.isClosureSharedVariable()); | ||
| } | ||
| } | ||
| }); |
| public Object doCall(final Object... args) { | ||
| Object[] provided = (args != null) ? args : EMPTY; | ||
| int arity = getMaximumNumberOfParameters(); | ||
| // Fast path: the caller supplied exactly the declared number of arguments (the common | ||
| // each()/collect() call). Skip the List-destructuring, correctArguments and per-arg coercion | ||
| // adaptation -- the canonical handle's asType does any primitive (un)boxing -- and dispatch. | ||
| if (provided.length == arity) { | ||
| Class<?>[] types = getParameterTypes(); | ||
| Object[] a = provided; | ||
| for (int i = 0; i < arity; i++) { |
…edClosures/flag off by default (GEP-27 S1) Add closure packing on top of the SAM-lambda hoisting: an eligible closure literal's body is hoisted into a synthetic 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. Capability analysis (PackStrategy, ClosureWriter.chooseStrategy): - a single decision point selects FULL_CLASS (S0) vs PACKED_ADAPTER (S1); - triggered by the @PackedClosures annotation (trust path, dynamic or static), or automatically under @CompileStatic when the type checker PROVES the closure delegate-independent -- every implicitly-received name resolved against the owner, read from STC's IMPLICIT_RECEIVER paths (a path ending in "owner" is safe; "delegate", from @DelegatesTo/with, is not). Behind groovy.target.closure.pack. Typed static dispatch (phase 4): the hoisted body reuses the expressions StaticCompilationVisitor already annotated, so under @CompileStatic it compiles statically -- read-only captures pass as typed leading parameters, inferred types (@ClosureParams, implicit it) arrive via metadata, and the method takes the closure's inferred return type. Packed bodies are statically dispatched with zero invokedynamic, often fewer indy sites than a closure class (which Reference-boxes captures as Object). Written captures ride the compiler's holder machinery: the shared Reference is passed as a parameter (originType/closure-shared/UseExistingReference), so the body is not rewritten, compiles statically, and supports every write form (compound ops beyond +=, postfix value semantics). Soundness for the dynamic trust path: - PackedClosure runtime guard fails fast if a delegate / delegate-consulting resolveStrategy is set on a trust-packed closure; a statically proven closure tolerates it as a no-op (as a @CompileStatic closure class does today); - compile-time escape decline keeps closures that visibly escape (stored to a field/property, returned, appended, in a collection literal) as classes. Mechanism fidelity so a packed closure behaves like a generated closure class: correct owner/static dispatch in static methods; getParameterTypes() carries the real declared types; arguments are adapted as reflective dispatch would (vararg collection, single List/Tuple destructuring, per-arg coercion Closure->SAM and GString->String); call() dispatches straight to doCall with invoker-exception unwrapping; dispatch via a cached reflective Method (metaclass coercion would auto-dereference Reference holder arguments). ClassNode.visitMethods: dedup newly-added methods by identity via an IdentityHashMap set (O(1)) instead of ArrayList.removeAll (O(n^2)). Visiting a method during code generation can add more (constructor-ref synthetic, lambda $deserializeLambda$, a hoisted body that hoists a nested one); the O(1) fixpoint visits them all and fixes a compile-time freeze the O(n^2) scan caused against method-generating visitors (e.g. groovy-contracts). Also use the fluent AstQuery API (GROOVY-12116) for the lambda nested-function check, and skip pre-pack bytecode-shape tests under the flag via @DisabledIfSystemProperty. Tests: ClosurePackCapabilityTest and PackedClosuresTransformTest (proof, declines, auto-pack, zero-indy typed bodies, write forms, destructuring). Flag-on stress net 0 failures across LambdaTest + all *Closure*/*DelegatesTo* static-compile suites + ReproducibleBytecodeBugs; flag-off byte-identical (778 green); groovy-contracts compiles and tests clean.
✅ All tests passed ✅🏷️ Commit: 43ec873 Learn more about TestLens at testlens.app. |
…edClosures/flag off by default (GEP-27 S1)
Add closure packing on top of the SAM-lambda hoisting: an eligible closure literal's body is hoisted into a synthetic 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.
Capability analysis (PackStrategy, ClosureWriter.chooseStrategy):
Typed static dispatch (phase 4): the hoisted body reuses the expressions StaticCompilationVisitor already annotated, so under @CompileStatic it compiles statically -- read-only captures pass as typed leading parameters, inferred types (@ClosureParams, implicit it) arrive via metadata, and the method takes the closure's inferred return type. Packed bodies are statically dispatched with zero invokedynamic, often fewer indy sites than a closure class (which Reference-boxes captures as Object).
Written captures ride the compiler's holder machinery: the shared Reference is passed as a parameter (originType/closure-shared/UseExistingReference), so the body is not rewritten, compiles statically, and supports every write form (compound ops beyond +=, postfix value semantics).
Soundness for the dynamic trust path:
Mechanism fidelity so a packed closure behaves like a generated closure class: correct owner/static dispatch in static methods; getParameterTypes() carries the real declared types; arguments are adapted as reflective dispatch would (vararg collection, single List/Tuple destructuring, per-arg coercion Closure->SAM and GString->String); call() dispatches straight to doCall with invoker-exception unwrapping; dispatch via a cached reflective Method (metaclass coercion would auto-dereference Reference holder arguments).
ClassNode.visitMethods: dedup newly-added methods by identity via an IdentityHashMap set (O(1)) instead of ArrayList.removeAll (O(n^2)). Visiting a method during code generation can add more (constructor-ref synthetic, lambda$deserializeLambda$ , a hoisted body that hoists a nested one); the O(1) fixpoint visits them all and fixes a compile-time freeze the O(n^2) scan caused against method-generating visitors (e.g. groovy-contracts).
Tests: ClosurePackCapabilityTest and PackedClosuresTransformTest (proof, declines, auto-pack, zero-indy typed bodies, write forms, destructuring). Flag-on stress net 0 failures across LambdaTest + all Closure/DelegatesTo static-compile suites + ReproducibleBytecodeBugs; flag-off byte-identical (778 green); groovy-contracts compiles and tests clean.