Copilot commented on code in PR #2628: URL: https://github.com/apache/groovy/pull/2628#discussion_r3478830582
########## src/main/java/org/codehaus/groovy/vmplugin/v8/IndyCompoundAssign.java: ########## @@ -0,0 +1,187 @@ +/* + * 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.vmplugin.v8; + +import groovy.lang.MetaClass; +import org.codehaus.groovy.GroovyBugError; +import org.codehaus.groovy.reflection.ClassInfo; +import org.codehaus.groovy.runtime.InvokerHelper; +import org.codehaus.groovy.runtime.ScriptBytecodeAdapter; + +import java.lang.invoke.MethodHandle; +import java.lang.invoke.MethodHandles; +import java.lang.invoke.MethodType; + +/** + * GEP-15: resolver for dynamic compound-assignment operators + * ({@code +=}, {@code -=}, ...). + * + * <p>This class holds only the GEP-15-specific <em>policy</em>; the call-site + * <em>lifecycle</em> is owned by {@link IndyInterface}. A compound-assignment + * {@code op=} is emitted as an {@code invokedynamic} to + * {@link IndyInterface#bootstrap} with call type + * {@link IndyInterface.CallType#COMPOUND_ASSIGN}, so it rides the same boot + * handle, per-receiver-class inline cache, monomorphic-promotion and + * deopt-storm protection as a normal method call. {@link IndyInterface#fallback} + * routes resolution here via {@link #resolve}. + * + * <p>What remains GEP-15-specific: + * <ul> + * <li>the two operator names ({@code assignName}/{@code baseName}) packed into + * the bootstrap {@code name} and unpacked here;</li> + * <li>a {@code respondsTo} probe to pick {@code *Assign} vs base — run only on + * a cache miss;</li> + * <li>the in-place return-receiver composition for the assign branch;</li> + * <li>a {@code (receiver class, arg class)} guard (arg class is part of the + * key so overloads stay correct), under the shared MOP switch point;</li> + * <li>a generic fall back to {@link ScriptBytecodeAdapter#compoundAssign} for + * null/unresolved receivers.</li> + * </ul> + * + * <p>The actual invocation is built by {@link Selector#selectInvokeHandle}, so + * selection/coercion/vargs/category/exception handling match a normal call. + * + * <p>WARNING: internal, indy-only. Not for use outside this package. + * + * @since 6.0.0 + */ +public final class IndyCompoundAssign { + + /** Separator packing {@code assignName} and {@code baseName} into one bootstrap constant (method names cannot contain a space). */ + public static final char NAME_SEPARATOR = ' '; Review Comment: NAME_SEPARATOR uses a literal space and the comment claims method names cannot contain a space. Groovy can define quoted method names containing spaces (e.g. src/test-resources/core/ClassDeclaration_03.groovy), so space is not a safe separator if this packing is ever reused. Use a separator that cannot appear in JVM method names, such as '\0' (NUL), and update the comment accordingly. ########## src/main/java/org/codehaus/groovy/vmplugin/v8/IndyCompoundAssign.java: ########## @@ -0,0 +1,187 @@ +/* + * 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.vmplugin.v8; + +import groovy.lang.MetaClass; +import org.codehaus.groovy.GroovyBugError; +import org.codehaus.groovy.reflection.ClassInfo; +import org.codehaus.groovy.runtime.InvokerHelper; +import org.codehaus.groovy.runtime.ScriptBytecodeAdapter; + +import java.lang.invoke.MethodHandle; +import java.lang.invoke.MethodHandles; +import java.lang.invoke.MethodType; + +/** + * GEP-15: resolver for dynamic compound-assignment operators + * ({@code +=}, {@code -=}, ...). + * + * <p>This class holds only the GEP-15-specific <em>policy</em>; the call-site + * <em>lifecycle</em> is owned by {@link IndyInterface}. A compound-assignment + * {@code op=} is emitted as an {@code invokedynamic} to + * {@link IndyInterface#bootstrap} with call type + * {@link IndyInterface.CallType#COMPOUND_ASSIGN}, so it rides the same boot + * handle, per-receiver-class inline cache, monomorphic-promotion and + * deopt-storm protection as a normal method call. {@link IndyInterface#fallback} + * routes resolution here via {@link #resolve}. + * + * <p>What remains GEP-15-specific: + * <ul> + * <li>the two operator names ({@code assignName}/{@code baseName}) packed into + * the bootstrap {@code name} and unpacked here;</li> + * <li>a {@code respondsTo} probe to pick {@code *Assign} vs base — run only on + * a cache miss;</li> + * <li>the in-place return-receiver composition for the assign branch;</li> + * <li>a {@code (receiver class, arg class)} guard (arg class is part of the + * key so overloads stay correct), under the shared MOP switch point;</li> + * <li>a generic fall back to {@link ScriptBytecodeAdapter#compoundAssign} for + * null/unresolved receivers.</li> + * </ul> + * + * <p>The actual invocation is built by {@link Selector#selectInvokeHandle}, so + * selection/coercion/vargs/category/exception handling match a normal call. + * + * <p>WARNING: internal, indy-only. Not for use outside this package. + * + * @since 6.0.0 + */ +public final class IndyCompoundAssign { + + /** Separator packing {@code assignName} and {@code baseName} into one bootstrap constant (method names cannot contain a space). */ + public static final char NAME_SEPARATOR = ' '; + + private static final MethodHandle GUARD; // (Class,Class,Object,Object) -> boolean + private static final MethodHandle COMPOUND_ASSIGN; // (Object,Object,String,String) -> Object + + /** (Object result, Object receiver, Object arg) -> receiver; folded over the assign invoke. */ + private static final MethodHandle RETURN_RECEIVER; + + static { + try { + MethodHandles.Lookup l = MethodHandles.lookup(); + GUARD = l.findStatic(IndyCompoundAssign.class, "guard", + MethodType.methodType(boolean.class, Class.class, Class.class, Object.class, Object.class)); + COMPOUND_ASSIGN = l.findStatic(ScriptBytecodeAdapter.class, "compoundAssign", + MethodType.methodType(Object.class, Object.class, Object.class, String.class, String.class)); + + MethodHandle pick = MethodHandles.identity(Object.class); // (receiver) -> receiver + pick = MethodHandles.dropArguments(pick, 1, Object.class); // (receiver, arg) -> receiver + RETURN_RECEIVER = MethodHandles.dropArguments(pick, 0, Object.class); // (result, receiver, arg) -> receiver + } catch (ReflectiveOperationException e) { + throw new GroovyBugError(e); + } + } + + private IndyCompoundAssign() { + } + + /** + * Packs the two operator names into a single bootstrap {@code name} constant. + * Called from code generation. + */ + public static String packNames(final String assignName, final String baseName) { + return assignName + NAME_SEPARATOR + baseName; + } + + /** + * Resolves the invocation for one receiver/arg shape and returns a + * {@link MethodHandleWrapper} for {@link IndyInterface}'s inline cache. The + * wrapper's target handle is {@code (Object receiver, Object arg) -> Object}, + * guarded on the receiver/arg classes and the shared MOP switch point with + * the call site's fallback (re-resolve) path as the else-branch. + * + * @param callSite the compound-assignment call site (type {@code (Object,Object)->Object}) + * @param sender the sending class + * @param packedNames {@code assignName} and {@code baseName} joined by {@link #NAME_SEPARATOR} + * @param arguments the runtime arguments: {@code [receiver, arg]} + */ + public static MethodHandleWrapper resolve(final CacheableCallSite callSite, final Class<?> sender, + final String packedNames, final Object[] arguments) { + int sep = packedNames.indexOf(NAME_SEPARATOR); + String assignName = packedNames.substring(0, sep); + String baseName = packedNames.substring(sep + 1); Review Comment: resolve assumes packedNames contains NAME_SEPARATOR; if it does not (e.g. mixed bytecode/runtime versions or unexpected bootstrap usage), indexOf returns -1 and substring will throw StringIndexOutOfBoundsException. Prefer a clear GroovyBugError (or a safe fallback) so failures are actionable and not an opaque parsing crash. ########## src/test/groovy/org/codehaus/groovy/classgen/asm/indy/IndyCompoundAssignTest.groovy: ########## @@ -0,0 +1,219 @@ +/* + * 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.classgen.asm.indy + +import org.junit.Test + +/** + * GEP-15: behaviour parity for the invokedynamic compound-assignment inline + * cache. Exercises the dynamic path (indy is the default target), so each + * {@code op=} routes through the {@code COMPOUND_ASSIGN} call site rather than + * the uncached {@code ScriptBytecodeAdapter.compoundAssign} helper. + */ +final class IndyCompoundAssignTest { + + private static Object ev(String src) { new GroovyShell().evaluate(src) } + Review Comment: This new test uses JUnit 4 (org.junit.Test) while most nearby bytecode-generation tests use JUnit Jupiter, and it doesn't assert/assume that invokedynamic is enabled. If groovy.target.indy is turned off in a build, this test may either fail unexpectedly or (worse) pass without actually exercising the indy COMPOUND_ASSIGN path. Consider switching to Jupiter and gating execution on CompilerConfiguration.DEFAULT.indyEnabled (e.g. via an assumption inside ev). ########## src/main/java/org/codehaus/groovy/vmplugin/v8/IndyCompoundAssign.java: ########## @@ -0,0 +1,187 @@ +/* + * 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.vmplugin.v8; + +import groovy.lang.MetaClass; +import org.codehaus.groovy.GroovyBugError; +import org.codehaus.groovy.reflection.ClassInfo; +import org.codehaus.groovy.runtime.InvokerHelper; +import org.codehaus.groovy.runtime.ScriptBytecodeAdapter; + +import java.lang.invoke.MethodHandle; +import java.lang.invoke.MethodHandles; +import java.lang.invoke.MethodType; + +/** + * GEP-15: resolver for dynamic compound-assignment operators + * ({@code +=}, {@code -=}, ...). + * + * <p>This class holds only the GEP-15-specific <em>policy</em>; the call-site + * <em>lifecycle</em> is owned by {@link IndyInterface}. A compound-assignment + * {@code op=} is emitted as an {@code invokedynamic} to + * {@link IndyInterface#bootstrap} with call type + * {@link IndyInterface.CallType#COMPOUND_ASSIGN}, so it rides the same boot + * handle, per-receiver-class inline cache, monomorphic-promotion and + * deopt-storm protection as a normal method call. {@link IndyInterface#fallback} + * routes resolution here via {@link #resolve}. + * + * <p>What remains GEP-15-specific: + * <ul> + * <li>the two operator names ({@code assignName}/{@code baseName}) packed into + * the bootstrap {@code name} and unpacked here;</li> + * <li>a {@code respondsTo} probe to pick {@code *Assign} vs base — run only on + * a cache miss;</li> + * <li>the in-place return-receiver composition for the assign branch;</li> + * <li>a {@code (receiver class, arg class)} guard (arg class is part of the + * key so overloads stay correct), under the shared MOP switch point;</li> + * <li>a generic fall back to {@link ScriptBytecodeAdapter#compoundAssign} for + * null/unresolved receivers.</li> + * </ul> + * + * <p>The actual invocation is built by {@link Selector#selectInvokeHandle}, so + * selection/coercion/vargs/category/exception handling match a normal call. + * + * <p>WARNING: internal, indy-only. Not for use outside this package. + * + * @since 6.0.0 + */ +public final class IndyCompoundAssign { + + /** Separator packing {@code assignName} and {@code baseName} into one bootstrap constant (method names cannot contain a space). */ + public static final char NAME_SEPARATOR = ' '; + + private static final MethodHandle GUARD; // (Class,Class,Object,Object) -> boolean + private static final MethodHandle COMPOUND_ASSIGN; // (Object,Object,String,String) -> Object + + /** (Object result, Object receiver, Object arg) -> receiver; folded over the assign invoke. */ + private static final MethodHandle RETURN_RECEIVER; + + static { + try { + MethodHandles.Lookup l = MethodHandles.lookup(); + GUARD = l.findStatic(IndyCompoundAssign.class, "guard", + MethodType.methodType(boolean.class, Class.class, Class.class, Object.class, Object.class)); + COMPOUND_ASSIGN = l.findStatic(ScriptBytecodeAdapter.class, "compoundAssign", + MethodType.methodType(Object.class, Object.class, Object.class, String.class, String.class)); + + MethodHandle pick = MethodHandles.identity(Object.class); // (receiver) -> receiver + pick = MethodHandles.dropArguments(pick, 1, Object.class); // (receiver, arg) -> receiver + RETURN_RECEIVER = MethodHandles.dropArguments(pick, 0, Object.class); // (result, receiver, arg) -> receiver + } catch (ReflectiveOperationException e) { + throw new GroovyBugError(e); + } + } + + private IndyCompoundAssign() { + } + + /** + * Packs the two operator names into a single bootstrap {@code name} constant. + * Called from code generation. + */ + public static String packNames(final String assignName, final String baseName) { + return assignName + NAME_SEPARATOR + baseName; + } + + /** + * Resolves the invocation for one receiver/arg shape and returns a + * {@link MethodHandleWrapper} for {@link IndyInterface}'s inline cache. The + * wrapper's target handle is {@code (Object receiver, Object arg) -> Object}, + * guarded on the receiver/arg classes and the shared MOP switch point with + * the call site's fallback (re-resolve) path as the else-branch. + * + * @param callSite the compound-assignment call site (type {@code (Object,Object)->Object}) + * @param sender the sending class + * @param packedNames {@code assignName} and {@code baseName} joined by {@link #NAME_SEPARATOR} + * @param arguments the runtime arguments: {@code [receiver, arg]} + */ + public static MethodHandleWrapper resolve(final CacheableCallSite callSite, final Class<?> sender, + final String packedNames, final Object[] arguments) { + int sep = packedNames.indexOf(NAME_SEPARATOR); + String assignName = packedNames.substring(0, sep); + String baseName = packedNames.substring(sep + 1); + + Object receiver = arguments[0]; + Object arg = arguments[1]; + + // Primitive fast path: a primitive-typed site has no user *Assign method + // and no receiver-class polymorphism, so resolve the base operator directly + // (IndyMath gives a primitive handle via Selector) and guard with the MOP + // switch point only — no respondsTo probe, no class guard. + MethodType st = callSite.type(); + if (st.parameterType(0).isPrimitive() || st.parameterType(1).isPrimitive()) { + MethodHandle invoke = Selector.selectInvokeHandle(callSite, sender, baseName, arguments); + MethodHandle guarded = IndyInterface.switchPoint.guardWithTest(invoke, callSite.getFallbackTarget()); + return wrap(guarded, true); Review Comment: In the primitive-site fast path, the wrapper is always marked cacheable (wrap(..., true)). If a per-instance MetaClass is installed for the boxed receiver class (e.g. Integer), caching by receiver class becomes unsound just like in the non-primitive path. Consider applying the same ClassInfo.hasPerInstanceMetaClasses() cacheability rule here to avoid PIC pollution/staleness. ########## src/main/java/org/codehaus/groovy/vmplugin/v8/Selector.java: ########## @@ -1209,11 +1225,32 @@ public void setCallSiteTarget() { addExceptionHandler(); } - setGuards(args[0]); - doCallSiteTargetSet(); } } + /** + * GEP-15 support: builds the <em>unguarded</em> invocation handle that a normal + * method call site would use for {@code methodName} on the given receiver/args + * ({@code arguments[0]} is the receiver). The result has type + * {@code callSite.type()}. Guard and switch-point wrapping are intentionally + * omitted — the caller (compound-assignment) applies its own per-shape guard + * and shares the global MOP {@link IndyInterface#switchPoint}. The caller must + * have already established that {@code methodName} resolves for this receiver + * (e.g. via {@code respondsTo}); this routes the actual invocation through the + * same selection, coercion and wrapping path as a normal call. + * + * @param callSite a call site supplying the desired {@code (receiver,arg)->Object} type + * @param sender the sending class for visibility/MOP decisions + * @param methodName the resolved method name (the chosen {@code *Assign} or base operator) + * @param arguments the runtime arguments, receiver first + * @return the unguarded invocation handle of type {@code callSite.type()} + */ + public static MethodHandle selectInvokeHandle(CacheableCallSite callSite, Class<?> sender, String methodName, Object[] arguments) { + MethodSelector selector = new MethodSelector(callSite, sender, methodName, CallType.METHOD, Boolean.FALSE, Boolean.FALSE, Boolean.FALSE, arguments); + selector.buildInvokeHandle(); + return selector.handle; + } Review Comment: selectInvokeHandle is introduced for internal reuse by IndyCompoundAssign and is not referenced outside org.codehaus.groovy.vmplugin.v8. Consider reducing its visibility to package-private to avoid exposing an easy-to-misuse unguarded-handle API as public surface. -- 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]
