Copilot commented on code in PR #2473: URL: https://github.com/apache/groovy/pull/2473#discussion_r3106007706
########## subprojects/performance/src/jmh/groovy/org/apache/groovy/bench/StaticMethodCallIndyBench.java: ########## @@ -0,0 +1,157 @@ +/* + * 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.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; + +/** + * Benchmarks for the early {@code setTarget} optimisation on static method + * call sites in the invokedynamic dispatch path. + * <p> + * When the receiver of an {@code invokedynamic} call is a {@code Class} object + * and the resolved method is {@code static}, the call site target is set + * immediately — bypassing the normal hit-count threshold — so that the JIT + * compiler can inline the target method handle sooner. + * <p> + * This benchmark compares: + * <ul> + * <li><b>Java</b> — direct {@code invokestatic}, the theoretical optimum</li> + * <li><b>Groovy dynamic</b> — {@code invokedynamic} with early setTarget</li> + * <li><b>Groovy {@code @CompileStatic}</b> — direct {@code invokestatic}</li> + * <li><b>Groovy instance</b> — {@code invokedynamic} without early setTarget (control group)</li> + * </ul> Review Comment: This Javadoc claims the benchmark compares 4 variants (including Groovy instance control group and a Groovy `@CompileStatic` baseline), but the actual benchmark methods don’t cover all variants for each scenario (e.g., no instance Fibonacci benchmark and no `@CompileStatic` chained-call benchmark). Please align the list with what’s actually measured, or add the missing benchmark methods so the documentation matches the results. ```suggestion * The benchmarks in this class cover selected comparisons between: * <ul> * <li><b>Java</b> — direct {@code invokestatic}, the theoretical optimum</li> * <li><b>Groovy dynamic</b> — {@code invokedynamic} with early setTarget</li> * <li><b>Groovy {@code @CompileStatic}</b> — direct {@code invokestatic}</li> * <li><b>Groovy instance</b> — {@code invokedynamic} without early setTarget (control group)</li> * </ul> * Not every scenario includes every variant; each benchmark method measures * only the implementations defined for that specific case. ``` ########## src/main/java/org/codehaus/groovy/vmplugin/v8/IndyInterface.java: ########## @@ -339,6 +339,14 @@ private static MethodHandle fromCacheHandle(CacheableCallSite callSite, Class<?> mhw = fallbackSupplier.get(); } + // GROOVY-11935: Set invokedynamic call site target immediately for static method calls to enable earlier JIT inlining + if (receiver instanceof Class) { + var method = mhw.getMethod(); + if (method != null && Modifier.isStatic(method.getModifiers())) { + callSite.setTarget(mhw.getTargetMethodHandle()); Review Comment: The early callSite.setTarget() path for static calls does not check mhw.isCanSetTarget(). If Selector.cache is false (e.g., UNCACHED_CALL / guardless paths), this can install an unguarded target handle and bypass switchpoint/arg guards, changing semantics and potentially breaking invalidation. Gate this optimization on mhw.isCanSetTarget() (and ideally also avoid resetting the target if it’s already the desired one). ```suggestion if (mhw.isCanSetTarget() && receiver instanceof Class) { var method = mhw.getMethod(); if (method != null && Modifier.isStatic(method.getModifiers())) { var targetMethodHandle = mhw.getTargetMethodHandle(); if (callSite.getTarget() != targetMethodHandle) { callSite.setTarget(targetMethodHandle); } ``` ########## src/main/java/org/codehaus/groovy/vmplugin/v8/IndyInterface.java: ########## @@ -339,6 +339,14 @@ private static MethodHandle fromCacheHandle(CacheableCallSite callSite, Class<?> mhw = fallbackSupplier.get(); } + // GROOVY-11935: Set invokedynamic call site target immediately for static method calls to enable earlier JIT inlining + if (receiver instanceof Class) { + var method = mhw.getMethod(); + if (method != null && Modifier.isStatic(method.getModifiers())) { + callSite.setTarget(mhw.getTargetMethodHandle()); + } + } Review Comment: The new GROOVY-11935 behavior changes call-site relinking (static calls can now update the MutableCallSite target immediately). There are existing tests for IndyInterface’s deprecated entry points, but none appear to assert the new static-call relinking behavior or that it only happens when caching/guards are enabled. Please add a regression test that exercises a static call through IndyInterface.fromCache/fromCacheHandle and verifies the call site target is (or is not) updated as intended. -- 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]
