[
https://issues.apache.org/jira/browse/GROOVY-12325?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18110393#comment-18110393
]
ASF GitHub Bot commented on GROOVY-12325:
-----------------------------------------
Copilot commented on code in PR #2852:
URL: https://github.com/apache/groovy/pull/2852#discussion_r3907176923
##########
src/main/java/org/codehaus/groovy/reflection/CachedMethod.java:
##########
@@ -82,6 +86,24 @@ public static CachedMethod find(final Method method) {
private boolean makeAccessibleDone;
private CachedMethod transformedMethod;
+ /**
+ * Installed JIT-constant trampoline, or {@code null} if not yet generated
+ * / sticky-failed. Volatile: the fast path reads it outside the generation
+ * lock.
+ */
+ private transient volatile DirectInvoker invoker;
+ /**
+ * Racy hit counter. {@code long} to match
+ * {@code IndyInterface.INDY_OPTIMIZE_THRESHOLD}. Over-counting delays
+ * generation slightly; under-counting generates slightly early. Both
harmless.
Review Comment:
The effects are reversed: over-counting reaches the threshold early, while
under-counting (such as lost increments) delays generation. Correcting this
avoids misleading readers about the intended race tolerance.
##########
src/main/java/org/apache/groovy/internal/runtime/invoke/InvokerBytecode.java:
##########
@@ -0,0 +1,232 @@
+/*
+ * 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.internal.runtime.invoke;
+
+import org.codehaus.groovy.classgen.asm.BytecodeHelper;
+import org.codehaus.groovy.classgen.asm.util.TypeUtil;
+import org.codehaus.groovy.control.CompilerConfiguration;
+import org.objectweb.asm.ClassWriter;
+import org.objectweb.asm.ConstantDynamic;
+import org.objectweb.asm.Handle;
+import org.objectweb.asm.MethodVisitor;
+import org.objectweb.asm.Opcodes;
+import org.objectweb.asm.Type;
+
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+import java.util.concurrent.atomic.AtomicInteger;
+
+/**
+ * Emits a {@link DirectInvoker} class for one {@link Method}.
+ *
+ * <p>Two encodings, same public shape ({@code public final} class, public
+ * no-arg {@code <init>}, {@code invoke(Object, Object[])}):
+ * <ul>
+ * <li>direct {@code INVOKE*} of the target (Steps 1, 2, 4)</li>
+ * <li>{@code ConstantDynamic} classData {@code MethodHandle} +
+ * {@code invokeExact} (Step 3)</li>
+ * </ul>
+ *
+ * Package-private. Dummy internal names live in this package so
+ * {@code HiddenClassDefiner.alignPackage} rewrites them onto the nest host.
+ */
+final class InvokerBytecode {
+
+ private static final String OBJECT = "java/lang/Object";
+ private static final String DIRECT_INVOKER =
Type.getInternalName(DirectInvoker.class);
+ private static final String INVOKE_DESC =
"(Ljava/lang/Object;[Ljava/lang/Object;)Ljava/lang/Object;";
+ private static final String METHOD_HANDLE_DESC =
"Ljava/lang/invoke/MethodHandle;";
+ private static final String METHOD_HANDLE_INTERNAL =
"java/lang/invoke/MethodHandle";
+ private static final String THROWABLE = "java/lang/Throwable";
+
+ /**
+ * BSM for {@link java.lang.invoke.MethodHandles#classData} — already has
+ * the {@code ConstantDynamic} bootstrap signature, so {@code <clinit>} is
+ * not required and there is no checked {@code IllegalAccessException}.
+ */
+ private static final Handle CLASS_DATA_BSM = new Handle(
+ Opcodes.H_INVOKESTATIC,
+ "java/lang/invoke/MethodHandles",
+ "classData",
+
"(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/Class;)Ljava/lang/Object;",
+ false);
+
+ private static final AtomicInteger NAMES = new AtomicInteger();
+
+ private InvokerBytecode() {
+ }
+
+ /**
+ * Unique dummy internal name in this package. Hidden-class define rewrites
+ * the package to the nest host; the suffix keeps {@code javap} dumps
readable.
+ */
+ static String nextInternalName() {
+ return DIRECT_INVOKER.substring(0, DIRECT_INVOKER.lastIndexOf('/') + 1)
+ + "MHInvoker$" + NAMES.getAndIncrement();
+ }
+
+ static byte[] emitInvokeStar(final Method method) {
+ return emitInvokeStar(method, nextInternalName());
+ }
+
+ static byte[] emitInvokeStar(final Method method, final String
internalName) {
+ return emit(method, internalName, false);
+ }
+
+ static byte[] emitClassData(final Method method) {
+ return emit(method, nextInternalName(), true);
+ }
+
+ private static byte[] emit(final Method method, final String internalName,
final boolean classData) {
+ final ClassWriter cw = new ClassWriter(ClassWriter.COMPUTE_MAXS |
ClassWriter.COMPUTE_FRAMES);
+ cw.visit(
+ CompilerConfiguration.DEFAULT.getBytecodeVersion(),
+ Opcodes.ACC_PUBLIC | Opcodes.ACC_FINAL | Opcodes.ACC_SUPER |
Opcodes.ACC_SYNTHETIC,
+ internalName,
+ null,
+ OBJECT,
+ new String[]{DIRECT_INVOKER});
+
+ emitConstructor(cw);
+ emitInvoke(cw, method, classData);
+
+ cw.visitEnd();
+ return cw.toByteArray();
+ }
+
+ private static void emitConstructor(final ClassWriter cw) {
+ final MethodVisitor mv = cw.visitMethod(Opcodes.ACC_PUBLIC, "<init>",
"()V", null, null);
+ mv.visitCode();
+ mv.visitVarInsn(Opcodes.ALOAD, 0);
+ mv.visitMethodInsn(Opcodes.INVOKESPECIAL, OBJECT, "<init>", "()V",
false);
+ mv.visitInsn(Opcodes.RETURN);
+ mv.visitMaxs(0, 0);
+ mv.visitEnd();
+ }
+
+ private static void emitInvoke(final ClassWriter cw, final Method method,
final boolean classData) {
+ final MethodVisitor mv = cw.visitMethod(
+ Opcodes.ACC_PUBLIC, "invoke", INVOKE_DESC, null, new
String[]{THROWABLE});
+ mv.visitCode();
+
+ if (classData) {
+ mv.visitLdcInsn(new ConstantDynamic("_", METHOD_HANDLE_DESC,
CLASS_DATA_BSM));
+ mv.visitTypeInsn(Opcodes.CHECKCAST, METHOD_HANDLE_INTERNAL);
+ }
+
+ loadReceiverAndArguments(mv, method);
+
+ if (classData) {
+ mv.visitMethodInsn(
+ Opcodes.INVOKEVIRTUAL,
+ METHOD_HANDLE_INTERNAL,
+ "invokeExact",
+ invokeExactDescriptor(method),
+ false);
+ } else {
+ final Class<?> declaring = method.getDeclaringClass();
+ mv.visitMethodInsn(
+ invokeOpcode(method),
+ BytecodeHelper.getClassInternalName(declaring),
+ method.getName(),
+ BytecodeHelper.getMethodDescriptor(method.getReturnType(),
method.getParameterTypes()),
+ declaring.isInterface());
+ }
+
+ boxAndReturn(mv, method.getReturnType());
+ mv.visitMaxs(0, 0);
+ mv.visitEnd();
+ }
+
+ /**
+ * Receiver (instance only) then each argument, with {@link
BytecodeHelper#doCast}.
+ * The Java wrapper already substitutes {@code EMPTY_ARRAY} for a null
+ * {@code arguments} local, so the bytecode assumes a non-null array.
+ */
+ private static void loadReceiverAndArguments(final MethodVisitor mv, final
Method method) {
+ final boolean isStatic = Modifier.isStatic(method.getModifiers());
+ if (!isStatic) {
+ mv.visitVarInsn(Opcodes.ALOAD, 1);
+ BytecodeHelper.doCast(mv, method.getDeclaringClass());
+ }
+ final Class<?>[] params = method.getParameterTypes();
+ for (int i = 0; i < params.length; i++) {
+ mv.visitVarInsn(Opcodes.ALOAD, 2);
+ BytecodeHelper.pushConstant(mv, i);
+ mv.visitInsn(Opcodes.AALOAD);
+ BytecodeHelper.doCast(mv, params[i]);
Review Comment:
The generated invoker does not preserve `Method.invoke`'s wrong-arity
behavior: it indexes only the declared parameters, so too few arguments throw
`ArrayIndexOutOfBoundsException`, while extra arguments are silently ignored
and the target executes. The previous reflective path rejects both cases with a
wrapped `IllegalArgumentException`. Validate the argument count before entering
the generated path (or fall back to reflection) and add regression coverage for
both cases.
##########
subprojects/performance/src/jmh/java/org/apache/groovy/bench/CachedMethodInvokerBench.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 org.codehaus.groovy.reflection.CachedMethod;
+import org.openjdk.jmh.annotations.Benchmark;
+import org.openjdk.jmh.annotations.BenchmarkMode;
+import org.openjdk.jmh.annotations.Fork;
+import org.openjdk.jmh.annotations.Measurement;
+import org.openjdk.jmh.annotations.Mode;
+import org.openjdk.jmh.annotations.OutputTimeUnit;
+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.concurrent.TimeUnit;
+
+/**
+ * Pipeline-split microbench for {@code CachedMethod.invoke}: Java direct call,
+ * reflective MOP invoke, and the generated {@code DirectInvoker} trampoline.
+ * <p>
+ * The generated path is the <em>monomorphic</em> best case (one
+ * {@code CachedMethod}, one trampoline class). Real {@code MetaClassImpl}
+ * dispatch across many types is megamorphic at the
+ * {@code DirectInvoker.invoke} call site; the {@code mega} rows exercise that.
+ * Guard ratios, not absolute nanoseconds. Run with
+ * {@code :perf:jmh -PbenchInclude=CachedMethodInvoker}.
+ * <p>
+ * Fork JVM args pin the generator: {@code threshold=0} installs on first
+ * invoke; {@code disable=true} stays on {@code Method.invoke}.
+ */
+@Warmup(iterations = 3, time = 1, timeUnit = TimeUnit.SECONDS)
+@Measurement(iterations = 5, time = 1, timeUnit = TimeUnit.SECONDS)
+@Fork(1)
+@BenchmarkMode(Mode.AverageTime)
+@OutputTimeUnit(TimeUnit.NANOSECONDS)
+@State(Scope.Thread)
+public class CachedMethodInvokerBench {
+
+ private static final String RECEIVER = "abcdef";
+ private static final String PREFIX = "abc";
+
+ private CachedMethod startsWith;
+ private CachedMethod[] mega;
+ private String[] megaReceivers;
+
+ /**
+ * Resolves the {@code CachedMethod}s used by the reflective / generated
rows.
+ */
+ @Setup
+ public void setUp() throws Exception {
+ startsWith = new CachedMethod(String.class.getMethod("startsWith",
String.class));
+ mega = new CachedMethod[]{
+ new CachedMethod(String.class.getMethod("startsWith",
String.class)),
+ new CachedMethod(String.class.getMethod("endsWith",
String.class)),
+ new CachedMethod(String.class.getMethod("contains",
CharSequence.class)),
+ new CachedMethod(String.class.getMethod("isEmpty")),
+ new CachedMethod(Integer.class.getMethod("toString")),
+ };
+ megaReceivers = new String[]{"abcdef", "xyz", "foo", ""};
+ // Warm generation when threshold=0 so measurement is steady-state.
+ startsWith.invoke(RECEIVER, new Object[]{PREFIX});
+ mega[0].invoke(megaReceivers[0], new Object[]{PREFIX});
+ mega[1].invoke(megaReceivers[1], new Object[]{"z"});
+ mega[2].invoke(megaReceivers[2], new Object[]{"oo"});
+ mega[3].invoke(megaReceivers[3], new Object[0]);
+ mega[4].invoke(Integer.valueOf(7), new Object[0]);
+ }
+
+ /**
+ * Java baseline: {@code String.startsWith}.
+ *
+ * @return whether the prefix matches
+ */
+ @Benchmark
+ public boolean startsWith_java() {
+ return RECEIVER.startsWith(PREFIX);
+ }
+
+ /**
+ * {@code CachedMethod.invoke} with generation disabled.
+ *
+ * @return boxed {@code Boolean}
+ */
+ @Benchmark
+ @Fork(value = 1, jvmArgsAppend =
"-Dgroovy.cachedmethod.invoker.disable=true")
+ public Object startsWith_reflective() {
+ return startsWith.invoke(RECEIVER, new Object[]{PREFIX});
+ }
+
+ /**
+ * {@code CachedMethod.invoke} after first-hit generation.
+ *
+ * @return boxed {@code Boolean}
+ */
+ @Benchmark
+ @Fork(value = 1, jvmArgsAppend =
"-Dgroovy.cachedmethod.invoker.threshold=0")
+ public Object startsWith_generated() {
+ return startsWith.invoke(RECEIVER, new Object[]{PREFIX});
+ }
+
+ /**
+ * Many distinct {@code CachedMethod}s through one {@code invoke} site,
+ * generation disabled.
+ *
+ * @return last boxed result
+ */
+ @Benchmark
+ @Fork(value = 1, jvmArgsAppend =
"-Dgroovy.cachedmethod.invoker.disable=true")
+ public Object mega_reflective() {
+ Object last = null;
+ last = mega[0].invoke(megaReceivers[0], new Object[]{PREFIX});
+ last = mega[1].invoke(megaReceivers[1], new Object[]{"z"});
+ last = mega[2].invoke(megaReceivers[2], new Object[]{"oo"});
+ last = mega[3].invoke(megaReceivers[3], new Object[0]);
+ last = mega[4].invoke(Integer.valueOf(7), new Object[0]);
Review Comment:
Only the final result is returned, so the first four computations are
unobservable and may be eliminated after JIT inlining. That can make this
benchmark measure fewer operations than intended and skew the
reflective/generated comparison. Consume every invocation result with a JMH
`Blackhole` (as the existing megamorphic dispatch benchmarks do).
This issue also appears on line 147 of the same file.
##########
src/test/java/org/codehaus/groovy/reflection/CachedMethodDirectInvokerTest.java:
##########
@@ -0,0 +1,231 @@
+/*
+ * 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.codehaus.groovy.reflection;
+
+import groovy.lang.MissingMethodException;
+import org.apache.groovy.internal.runtime.invoke.DirectInvokerSubjects;
+import org.apache.groovy.internal.runtime.invoke.InvokerFactory;
+import org.codehaus.groovy.runtime.InvokerInvocationException;
+import org.codehaus.groovy.runtime.MetaClassHelper;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.parallel.ResourceLock;
+import org.junit.jupiter.api.parallel.Resources;
+
+import java.io.IOException;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Hook tests for {@link CachedMethod#invoke} installing a {@code
DirectInvoker}.
+ *
+ * <p>Java (not Groovy) so {@code threshold=0} does not install trampolines on
+ * Groovy test closures. Each test uses {@code new CachedMethod(method)} so the
+ * interned MOP {@code CachedMethod} is not mutated.
+ */
+final class CachedMethodDirectInvokerTest {
+
+ @Test
+ @ResourceLock(Resources.SYSTEM_PROPERTIES)
+ void testThresholdZeroUsesGeneratedPath() throws Exception {
+ withThreshold(0L, () -> {
+ CachedMethod cm = new
CachedMethod(DirectInvokerSubjects.class.getMethod("ping"));
+ assertEquals("pong", cm.invoke(new DirectInvokerSubjects(), null));
+ assertEquals("pong", cm.invoke(new DirectInvokerSubjects(),
MetaClassHelper.EMPTY_ARRAY));
+ });
+ }
+
+ @Test
+ @ResourceLock(Resources.SYSTEM_PROPERTIES)
+ void testNullArgumentsAreEmptyArray() throws Exception {
+ withThreshold(0L, () -> {
+ CachedMethod cm = new
CachedMethod(DirectInvokerSubjects.class.getMethod("ping"));
+ assertEquals("pong", cm.invoke(new DirectInvokerSubjects(), null));
+ });
+ }
+
+ @Test
+ @ResourceLock(Resources.SYSTEM_PROPERTIES)
+ void testPrimitiveBoxingMatchesReflective() throws Exception {
+ withThreshold(0L, () -> {
+ CachedMethod generated = new CachedMethod(
+ DirectInvokerSubjects.class.getMethod("add", int.class,
int.class));
+ Object viaGenerated = generated.invoke(new
DirectInvokerSubjects(), new Object[]{2, 5});
+ withDisable(() -> {
+ CachedMethod reflective = new CachedMethod(
+ DirectInvokerSubjects.class.getMethod("add",
int.class, int.class));
+ Object viaReflect = reflective.invoke(new
DirectInvokerSubjects(), new Object[]{2, 5});
+ assertEquals(viaReflect, viaGenerated);
+ assertEquals(Integer.valueOf(7), viaGenerated);
+ });
+ });
+ }
+
+ @Test
+ @ResourceLock(Resources.SYSTEM_PROPERTIES)
+ void testRuntimeExceptionIsRethrown() throws Exception {
+ withThreshold(0L, () -> {
+ CachedMethod cm = new
CachedMethod(DirectInvokerSubjects.class.getMethod("boomRuntime"));
+ IllegalStateException ex =
assertThrows(IllegalStateException.class,
+ () -> cm.invoke(new DirectInvokerSubjects(), null));
+ assertEquals("runtime-boom", ex.getMessage());
+ });
+ }
+
+ @Test
+ @ResourceLock(Resources.SYSTEM_PROPERTIES)
+ void testMissingMethodExceptionIsWrapped() throws Exception {
+ withThreshold(0L, () -> {
+ CachedMethod cm = new
CachedMethod(DirectInvokerSubjects.class.getMethod("boomMme"));
+ InvokerInvocationException ex =
assertThrows(InvokerInvocationException.class,
+ () -> cm.invoke(new DirectInvokerSubjects(), null));
+ assertInstanceOf(MissingMethodException.class, ex.getCause());
+ });
+ }
+
+ @Test
+ @ResourceLock(Resources.SYSTEM_PROPERTIES)
+ void testErrorFromTargetIsWrapped() throws Exception {
+ withThreshold(0L, () -> {
+ CachedMethod cm = new
CachedMethod(DirectInvokerSubjects.class.getMethod("boomError"));
+ InvokerInvocationException ex =
assertThrows(InvokerInvocationException.class,
+ () -> cm.invoke(new DirectInvokerSubjects(), null));
+ assertInstanceOf(AssertionError.class, ex.getCause());
+ assertEquals("error-boom", ex.getCause().getMessage());
+ });
+ }
+
+ @Test
+ @ResourceLock(Resources.SYSTEM_PROPERTIES)
+ void testCheckedExceptionIsWrapped() throws Exception {
+ withThreshold(0L, () -> {
+ CachedMethod cm = new
CachedMethod(DirectInvokerSubjects.class.getMethod("boomChecked"));
+ InvokerInvocationException ex =
assertThrows(InvokerInvocationException.class,
+ () -> cm.invoke(new DirectInvokerSubjects(), null));
+ assertInstanceOf(IOException.class, ex.getCause());
+ assertEquals("checked-boom", ex.getCause().getMessage());
+ });
+ }
+
+ @Test
+ @ResourceLock(Resources.SYSTEM_PROPERTIES)
+ void testReflectivePathStillWrapsIllegalArgumentException() throws
Exception {
+ withDisable(() -> {
+ CachedMethod cm = new CachedMethod(
+ DirectInvokerSubjects.class.getMethod("echo",
String.class));
+ InvokerInvocationException ex =
assertThrows(InvokerInvocationException.class,
+ () -> cm.invoke(new DirectInvokerSubjects(), new
Object[]{1}));
+ assertInstanceOf(IllegalArgumentException.class, ex.getCause());
+ });
+ }
+
+ @Test
+ @ResourceLock(Resources.SYSTEM_PROPERTIES)
+ void testGeneratedPathRethrowsClassCastOnBadArgs() throws Exception {
+ withThreshold(0L, () -> {
+ CachedMethod cm = new CachedMethod(
+ DirectInvokerSubjects.class.getMethod("echo",
String.class));
+ assertThrows(ClassCastException.class,
+ () -> cm.invoke(new DirectInvokerSubjects(), new
Object[]{1}));
+ });
+ }
+
+ @Test
+ @ResourceLock(Resources.SYSTEM_PROPERTIES)
+ void testCallerSensitiveStaysReflective() throws Exception {
+ withThreshold(0L, () -> {
+ CachedMethod cm = new
CachedMethod(Class.class.getMethod("forName", String.class));
+ assertTrue(cm.isCallerSensitive());
+ Class<?> loaded = (Class<?>) cm.invoke(null, new
Object[]{String.class.getName()});
+ assertEquals(String.class, loaded);
+ });
+ }
+
+ @Test
+ @ResourceLock(Resources.SYSTEM_PROPERTIES)
+ void testKillSwitchUsesReflectivePath() throws Exception {
+ withDisable(() -> {
+ CachedMethod cm = new
CachedMethod(DirectInvokerSubjects.class.getMethod("ping"));
+ assertEquals("pong", cm.invoke(new DirectInvokerSubjects(), null));
+ });
+ }
+
+ @Test
+ @ResourceLock(Resources.SYSTEM_PROPERTIES)
+ void testDefaultThresholdDoesNotGenerateOnFirstCall() throws Exception {
+ String previous =
System.getProperty(InvokerFactory.PROPERTY_THRESHOLD);
+ try {
+ System.clearProperty(InvokerFactory.PROPERTY_THRESHOLD);
+ CachedMethod cm = new
CachedMethod(DirectInvokerSubjects.class.getMethod("ping"));
+ assertEquals("pong", cm.invoke(new DirectInvokerSubjects(), null));
Review Comment:
This assertion passes whether the first call is reflective or generated, so
it does not test the threshold behavior named by the test. Use an invocation
whose outcomes distinguish the paths—for example, `echo(String)` with an
invalid argument should still produce the reflective path's wrapped
`IllegalArgumentException` on the first call.
> Speed up CachedMethod.invoke with a generated JIT-constant trampoline
> ---------------------------------------------------------------------
>
> Key: GROOVY-12325
> URL: https://issues.apache.org/jira/browse/GROOVY-12325
> Project: Groovy
> Issue Type: Improvement
> Reporter: Daniel Sun
> Priority: Major
>
> h2. Problem
> {{CachedMethod.invoke}} is the MOP/Java fallback used by {{MetaClassImpl}},
> classic uncompiled call sites, and the default indy cold tier
> ({{invokeColdReflective}} -> {{doMethodInvoke}}).
> That path still calls {{java.lang.reflect.Method.invoke}}. A {{MethodHandle}}
> held in an instance field is in the same performance band. After C2, only a
> JIT-constant callee (direct {{invokevirtual}} / {{invokestatic}} /
> {{invokeinterface}} in generated bytecode, or {{invokeExact}} of a {{static
> final}} / classData handle, or a linked {{invokedynamic}} CallSite) runs like
> a Java direct call.
> Hot monomorphic indy and {{@CompileStatic}} already have that shape.
> {{CachedMethod.invoke}} does not.
> h2. Approach
> After {{groovy.cachedmethod.invoker.threshold}} hits (default 100, below
> {{groovy.indy.optimize.threshold}} of 1000 so cold indy is still on
> {{doMethodInvoke}} when the trampoline appears), install a generated
> {{DirectInvoker}} behind {{CachedMethod.invoke}} only.
> Internal types live in {{org.apache.groovy.internal.runtime.invoke}}
> (japicmp-excluded). Definition reuses {{HiddenClassDefiner}} (GROOVY-12223)
> and {{ClassLoaderForClassArtifacts}}.
> Define order:
> # InvokerFactory nestmate + direct invoke when the member is publicly
> invocable from that class ({{String.startsWith}}).
> # Declaring-class nestmate + direct invoke when {{privateLookupIn}} is
> possible. Private class methods use {{invokevirtual}}; private interface
> methods use {{invokeinterface}} (hidden nestmates do not subclass the host,
> so {{invokespecial}} fails verification).
> # InvokerFactory nestmate + classData {{MethodHandle}} + {{invokeExact}} when
> types are still resolvable from the runtime loader.
> # {{ClassLoaderForClassArtifacts}} when the host loader can resolve
> {{DirectInvoker}} — never for bootstrap types.
> Failures sticky-return {{null}}; {{CachedMethod.invoke}} keeps
> {{Method.invoke}}. Generation is skipped for caller-sensitive and abstract
> methods, Android, native image, and when hidden classes are disabled.
> This is the MOP "Groovy as caller" path ({{makeAccessible}}). Indy continues
> to {{unreflect}} with the call-site {{Lookup}} and must not be fed the
> trampoline.
> h2. Configuration
> {noformat}
> -Dgroovy.cachedmethod.invoker.threshold=100
> -Dgroovy.cachedmethod.invoker.disable=true
> {noformat}
> The existing {{-Dgroovy.hidden.classes.disable=true}} also turns generation
> off.
> h2. Compatibility
> * No change to the {{MetaMethod.invoke}} / {{CachedMethod.invoke}} signatures.
> * Selection (categories, EMC, interceptable, per-instance MetaClass) is
> unchanged; the trampoline is bound to the Java {{Method}}, not to a
> {{MetaMethod}} wrapper.
> * Wrong-argument type on the generated path is {{ClassCastException}}
> (rethrown), matching DGM / {{CallSiteGenerator}}. The reflective path still
> wraps {{IllegalArgumentException}} in {{InvokerInvocationException}}.
> * Opt-out: {{-Dgroovy.cachedmethod.invoker.disable=true}}.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
