[
https://issues.apache.org/jira/browse/GROOVY-12151?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18096099#comment-18096099
]
ASF GitHub Bot commented on GROOVY-12151:
-----------------------------------------
Copilot commented on code in PR #2709:
URL: https://github.com/apache/groovy/pull/2709#discussion_r3576679993
##########
subprojects/performance/src/jmh/groovy/org/apache/groovy/bench/ClosurePackBench.java:
##########
@@ -0,0 +1,154 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.groovy.bench;
+
+import groovy.lang.GroovyClassLoader;
+import org.openjdk.jmh.annotations.Benchmark;
+import org.openjdk.jmh.annotations.BenchmarkMode;
+import org.openjdk.jmh.annotations.Fork;
+import org.openjdk.jmh.annotations.Level;
+import org.openjdk.jmh.annotations.Measurement;
+import org.openjdk.jmh.annotations.Mode;
+import org.openjdk.jmh.annotations.OutputTimeUnit;
+import org.openjdk.jmh.annotations.Param;
+import org.openjdk.jmh.annotations.Scope;
+import org.openjdk.jmh.annotations.Setup;
+import org.openjdk.jmh.annotations.State;
+import org.openjdk.jmh.annotations.Warmup;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+
+/**
+ * Steady-state closure invocation under closure packing (GROOVY-12151,
+ * {@code groovy.target.closure.pack}) versus generated closure classes, for
the
+ * two hot closure shapes: a non-capturing {@code collect} body and a
+ * written-capture {@code each} accumulator.
+ * <p>
+ * The flag is read at closure-emission time, so {@link #setup} compiles the
+ * fixture classes with a fresh {@link GroovyClassLoader} after setting it from
+ * the {@code pack} param — JMH runs each param value in its own forks, so the
+ * two states never share a JVM or its profile. A vacuity guard asserts the
flag
+ * really flipped the emission (no {@code _closure} classes when packing, some
+ * when not).
+ * <p>
+ * Each shape runs twice: {@code mono} always invokes one fixture class, the
+ * best case for JIT inlining; {@code mega} rotates over {@link #CLASSES}
+ * fixture classes, making the shared {@code PackedClosure} adapter's
dispatcher
+ * call site megamorphic (each class has its own hidden dispatcher class) — the
+ * realistic many-classes regime, and the honest comparison against generated
+ * closure classes, whose {@code call} sites in DGM are equally megamorphic.
+ * Single-class microbenches overstate packed dispatch costs (a deep
monomorphic
+ * inline of the shared adapter trips the JIT's {@code InlineSmallCode}
+ * heuristic); this pair brackets the real-world range instead.
+ */
+@Warmup(iterations = 5, time = 1)
+@Measurement(iterations = 5, time = 1)
+@Fork(2)
+@BenchmarkMode(Mode.Throughput)
+@OutputTimeUnit(TimeUnit.MILLISECONDS)
+@State(Scope.Benchmark)
+public class ClosurePackBench {
+
+ /** Implemented by every compiled fixture class, so invocation needs no
reflection. */
+ public interface PackWorkload {
+ Object squared(List<Integer> xs);
+ int sum(List<Integer> xs);
+ }
+
+ private static final int CLASSES = 16; // power of two: mega rotation uses
a mask
+
+ /** Compile-time flag: {@code true} packs eligible closures, {@code false}
= closure classes. */
+ @Param({"false", "true"})
+ public String pack;
+
+ private final PackWorkload[] fixtures = new PackWorkload[CLASSES];
+ private List<Integer> xs;
+ private int idx;
+
+ @Setup(Level.Trial)
+ public void setup() throws Exception {
+ System.setProperty("groovy.target.closure.pack", pack);
+ try (GroovyClassLoader loader = new
GroovyClassLoader(getClass().getClassLoader())) {
+ for (int i = 0; i < CLASSES; i += 1) {
+ String source =
+ "@groovy.transform.CompileStatic\n" +
+ "class Fix" + i + " implements
org.apache.groovy.bench.ClosurePackBench.PackWorkload {\n" +
+ " Object squared(List<Integer> xs) { xs.collect {
it * it } }\n" +
+ " int sum(List<Integer> xs) { int s = 0; xs.each {
Integer x -> s += x }; s }\n" +
+ "}\n";
+ Class<?> c = loader.parseClass(source, "Fix" + i + ".groovy");
+ fixtures[i] = (PackWorkload)
c.getDeclaredConstructor().newInstance();
+ }
+ // vacuity guard: the flag must actually gate the emission
+ boolean closureClasses = false;
+ for (Class<?> c : loader.getLoadedClasses()) {
+ if (c.getName().contains("_closure")) closureClasses = true;
+ }
+ if (Boolean.parseBoolean(pack) == closureClasses) {
+ throw new IllegalStateException("closure.pack=" + pack + " but
closure classes "
+ + (closureClasses ? "were" : "were not") + "
generated");
+ }
+ }
+ xs = new ArrayList<>();
Review Comment:
setup() sets the global system property groovy.target.closure.pack but never
restores it. In a single JMH fork, benchmarks run in the same JVM, so this can
leak into other benchmarks (and into any Groovy compilation they do), skewing
results.
##########
src/main/java/org/codehaus/groovy/classgen/asm/ClosureWriter.java:
##########
@@ -158,6 +237,644 @@ public void writeClosure(final ClosureExpression
expression) {
controller.getOperandStack().replace(ClassHelper.CLOSURE_TYPE,
localVariableParams.length);
}
+ /**
+ * GEP-27 capability analysis: choose the compilation strategy for a
closure literal.
+ * <p>
+ * A closure is packed (S1, {@link PackStrategy#PACKED_ADAPTER}) when it
is <em>triggered</em> —
+ * either the enclosing scope opts in via {@code @PackedClosures} (the
dynamic trust path), or the
+ * capability analysis <em>proves</em> it delegate-independent under
{@code @CompileStatic} (the
+ * automatic path, see {@link #isDelegateIndependent}) — and it is in a
packable position:
+ * it needs no real {@code Closure} semantics we cannot reproduce ({@link
#isPackable}), it is
+ * written directly in a method rather than nested in another closure
(owner-retargeting for
+ * nested closures is later work), and it does not visibly escape its
method. Otherwise it stays a
+ * full generated class (S0, {@link PackStrategy#FULL_CLASS}) exactly as
today.
+ */
+ private PackStrategy chooseStrategy(final ClosureExpression expression) {
+ if (!isPackable(expression)) return PackStrategy.FULL_CLASS;
+ // The most-specific @PackedClosures mode in scope (method wins over
class), or null if not
+ // annotated. DISABLED is a fine-grained opt-out that beats an
enclosing opt-in AND the flag.
+ PackMode mode = annotatedMode();
+ if (mode == PackMode.DISABLED) return PackStrategy.FULL_CLASS;
+ boolean annotated = (mode != null); // LENIENT/WARN/STRICT all opt in
(DISABLED handled above)
+ if (!isPackTriggered(expression, annotated)) return
PackStrategy.FULL_CLASS;
+ if (controller.isInGeneratedFunction()) return PackStrategy.FULL_CLASS;
+ if (escapesEnclosingMethod(expression)) return PackStrategy.FULL_CLASS;
+ return PackStrategy.PACKED_ADAPTER;
+ }
+
+ /**
+ * Whether a packable, non-escaping closure literal should actually be
packed. Dynamic compilation
+ * has no proof of delegate-independence, so the only trigger is the
{@code @PackedClosures} trust
+ * assertion; {@code StaticTypesClosureWriter} overrides this to add the
+ * {@code groovy.target.closure.pack} flag and the delegate-independence
proof.
+ *
+ * @param annotated whether a non-DISABLED {@code @PackedClosures} is in
scope
+ */
+ protected boolean isPackTriggered(final ClosureExpression expression,
final boolean annotated) {
+ return annotated;
+ }
+
+ /**
+ * A trigger-specific decline reason for {@link #declineReason}, or {@code
null} if the trigger did
+ * not decline this closure. Only the static writer has one (a
delegate-resolved body); the dynamic
+ * path trusts the annotation and never declines on trigger grounds.
+ */
+ protected String triggerDeclineReason(final ClosureExpression expression) {
+ return null;
+ }
+
+ /** Whether the hoisted body is compiled statically (true only for {@code
@CompileStatic}). */
+ protected boolean compilesHoistedBodyStatically() {
+ return false;
+ }
+
+ /**
+ * Whether the packed adapter installs the runtime delegate guard. The
dynamic trust path needs it
+ * (an unverifiable assertion must fail fast on misuse); the static writer
proved independence, so
+ * it overrides this to {@code false} and a caller-set delegate is then
stored and ignored.
+ */
+ protected boolean packedClosureUsesDelegateGuard() {
+ return true;
+ }
+
+ /** The closure's inferred return type, normalised the same way {@code
createClosureClass} does for doCall. */
+ private static ClassNode inferredClosureReturnType(final ClosureExpression
expression) {
+ ClassNode returnType =
expression.getNodeMetaData(INFERRED_RETURN_TYPE);
+ if (returnType == null) returnType = ClassHelper.OBJECT_TYPE;
+ else if (returnType.isPrimaryClassNode()) returnType =
returnType.getPlainNodeReference();
+ else if (ClassHelper.isPrimitiveType(returnType)) returnType =
ClassHelper.getWrapper(returnType);
+ else if (GenericsUtils.hasUnresolvedGenerics(returnType)) returnType =
GenericsUtils.nonGeneric(returnType);
+ return returnType;
+ }
+
+ /**
+ * The erasure of a captured variable's type, safe to declare on the
hoisted method: generic
+ * placeholders are replaced by their bound (the hoisted method does not
declare the enclosing
+ * method's type variables) and type arguments are dropped ({@code
List<T>} → {@code List}).
+ */
+ private static ClassNode erasedType(final ClassNode type) {
+ ClassNode t = type;
+ if (t == null) return ClassHelper.OBJECT_TYPE;
+ if (t.isGenericsPlaceHolder()) t = t.redirect();
+ return t.getPlainNodeReference();
+ }
+
+ /**
+ * Reports a top-level closure that was NOT packed, per the {@code mode}
of the enclosing
+ * {@code @PackedClosures} annotation (WARN => compiler warning, STRICT =>
compiler error). Only the
+ * annotation opts in; the automatic {@code groovy.target.closure.pack}
path is always lenient. A
+ * nested closure is a structural decline (its enclosing context is a
generated function, not the
+ * owner class), so it is not reported.
+ */
+ private void reportUnpacked(final ClosureExpression expression) {
+ if (controller.isInGeneratedFunction()) return;
+ PackMode mode = annotatedMode();
+ // LENIENT/DISABLED are both silent
> hoist eligible closure bodies to a shared adapter @PackedClosures/flag
> (GEP-27 S1)
> ----------------------------------------------------------------------------------
>
> Key: GROOVY-12151
> URL: https://issues.apache.org/jira/browse/GROOVY-12151
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
