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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
