[
https://issues.apache.org/jira/browse/GROOVY-12325?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18110404#comment-18110404
]
ASF GitHub Bot commented on GROOVY-12325:
-----------------------------------------
Copilot commented on code in PR #2852:
URL: https://github.com/apache/groovy/pull/2852#discussion_r3907564203
##########
src/main/java/org/apache/groovy/internal/runtime/invoke/InvokerFactory.java:
##########
@@ -0,0 +1,337 @@
+/*
+ * 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 groovy.transform.Internal;
+import org.apache.groovy.util.HiddenClassDefiner;
+import org.apache.groovy.util.SystemUtil;
+import org.codehaus.groovy.reflection.CachedMethod;
+import org.codehaus.groovy.reflection.ClassLoaderForClassArtifacts;
+import org.codehaus.groovy.reflection.android.AndroidSupport;
+
+import java.lang.invoke.MethodHandle;
+import java.lang.invoke.MethodHandles;
+import java.lang.invoke.MethodHandles.Lookup;
+import java.lang.invoke.MethodType;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+
+/**
+ * Factory for {@link DirectInvoker} instances bound to one {@link
CachedMethod}.
+ *
+ * <p>Not user API. {@code CachedMethod.invoke} is the only production caller.
+ * Package-visible helpers exist so tests can drive generation without going
+ * through the hit-count hook.
+ *
+ * <p>Define path (one order):
+ * <ol>
+ * <li>InvokerFactory nestmate + {@code INVOKE*} when the member is publicly
+ * invocable from this class and every type is resolvable here
+ * ({@code String.startsWith}).</li>
+ * <li>Declaring-class nestmate + {@code INVOKE*} when
+ * {@code privateLookupIn} is worth attempting and the host can resolve
+ * {@link DirectInvoker}.</li>
+ * <li>InvokerFactory nestmate + classData {@code MethodHandle} when this
+ * loader can resolve the erased {@code invokeExact} types.</li>
+ * <li>{@link ClassLoaderForClassArtifacts} visible class, only when that
+ * loader can resolve {@link DirectInvoker} — never for bootstrap
hosts.</li>
+ * </ol>
+ *
+ * Failures sticky-return {@code null}; they must not propagate to
+ * {@code CachedMethod.invoke}.
+ *
+ * @since 6.0.0
+ */
+@Internal
+public final class InvokerFactory {
+
+ /**
+ * Hits before generation. Default 100 — below
+ * {@code groovy.indy.optimize.threshold} (1000) so cold indy still sits on
+ * {@code doMethodInvoke} when the trampoline appears.
+ */
+ public static final String PROPERTY_THRESHOLD =
"groovy.cachedmethod.invoker.threshold";
+
+ /** Kill switch. Default {@code false}. */
+ public static final String PROPERTY_DISABLE =
"groovy.cachedmethod.invoker.disable";
+
+ /**
+ * Full-privilege lookup for <em>this</em> class — production nest host for
+ * Steps 1 and 3. Caller-sensitive: must be captured here, not in
+ * {@link HiddenClassDefiner}.
+ */
+ static final Lookup LOOKUP = MethodHandles.lookup();
+
+ private InvokerFactory() {
+ }
+
+ /**
+ * Attempts to bind a trampoline for {@code method}. Returns {@code null}
+ * on any failure (sticky-fail). Public so {@code CachedMethod} in another
+ * package can call it; not user API ({@link Internal}, {@code internal}
+ * package). Tests in this package also drive generation through this
+ * method without going through the hit-count hook.
+ *
+ * @param method the cached method to bind
+ * @return a trampoline, or {@code null}
+ */
+ public static DirectInvoker tryCreate(final CachedMethod method) {
+ if (method == null || !generationAllowed()) {
+ return null;
+ }
+ if (method.isCallerSensitive() ||
Modifier.isAbstract(method.getModifiers())) {
+ return null;
+ }
+ try {
+ final Method m = method.getCachedMethod();
+ final Class<?> declaring = m.getDeclaringClass();
+ final Class<?>[] params = m.getParameterTypes();
+ final Class<?> returnType = m.getReturnType();
+
+ // Step 1: InvokerFactory nestmate + INVOKE* (String.startsWith).
+ if (isPubliclyInvocableFromInvokerFactory(method)
+ && canResolveInvokeTypes(LOOKUP.lookupClass(), declaring,
params, returnType)) {
+ final DirectInvoker step1 = defineInvokeStarOnLookup(m,
LOOKUP);
+ if (step1 != null) {
+ return step1;
+ }
+ }
+
+ // Step 2: declaring-class nestmate + INVOKE*.
+ if (HiddenClassDefiner.canAttemptPrivateLookup(declaring)
+ && loaderCanResolve(declaring, DirectInvoker.class)) {
+ final DirectInvoker step2 =
defineInvokeStarOnDeclaringClass(m, declaring);
+ if (step2 != null) {
+ return step2;
+ }
+ }
+
+ // Step 3: InvokerFactory nestmate + classData MH.
+ if (canResolveInvokeTypes(LOOKUP.lookupClass(), declaring, params,
returnType)) {
+ final DirectInvoker step3 = tryCreateClassData(method);
+ if (step3 != null) {
+ return step3;
+ }
+ }
+
+ // Step 4: visible artifact, never for bootstrap hosts.
+ if (declaring.getClassLoader() != null
+ && loaderCanResolve(declaring, DirectInvoker.class)
+ && isPubliclyInvocableFromInvokerFactory(method)) {
+ return defineVisibleArtifact(method, m);
Review Comment:
The visible-artifact fallback is not exercised by the added tests.
`testGroovyClassLoaderPublicMethodIsDeclaringClassNestmate` runs in unnamed
modules where `privateLookupIn` succeeds, so it deterministically returns from
Step 2 and never reaches this branch. Please add a case whose public declaring
class is resolvable only from its own loader while private lookup is
unavailable, and assert that the resulting invoker is a non-hidden Step 4
artifact.
This issue also appears on line 141 of the same file.
##########
subprojects/performance/src/jmh/java/org/apache/groovy/bench/CachedMethodInvokerBench.java:
##########
@@ -0,0 +1,151 @@
+/*
+ * 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 org.openjdk.jmh.infra.Blackhole;
+
+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";
Review Comment:
These compile-time constants let HotSpot inline and constant-fold
`RECEIVER.startsWith(PREFIX)` to `true`, so `startsWith_java` does not measure
a Java direct call as documented. Source the operands from mutable JMH state
(for example, `@Param` fields) so the baseline retains the actual call without
adding unrelated allocation or volatile-access costs.
> 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)
