Copilot commented on code in PR #2369:
URL: https://github.com/apache/groovy/pull/2369#discussion_r2702398825
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/IndyInterface.java:
##########
@@ -304,8 +325,18 @@ MethodHandleWrapper get() {
/**
* Get the cached methodhandle. if the related methodhandle is not found
in the inline cache, cache and return it.
+ * @deprecated Use the new boothandle-based approach instead.
Review Comment:
The deprecation message "Use the new boothandle-based approach instead"
contains the typo "boothandle" which should be "boot handle" or "bootHandle"
for consistency with Java naming conventions. This same typo appears in both
deprecated methods.
```suggestion
* @deprecated Use the new bootHandle-based approach instead.
```
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/IndyInterface.java:
##########
@@ -254,19 +255,39 @@ private static CallSite
realBootstrap(MethodHandles.Lookup caller, String name,
* Makes a fallback method for an invalidated method selection
*/
protected static MethodHandle makeFallBack(MutableCallSite mc, Class<?>
sender, String name, int callID, MethodType type, boolean safeNavigation,
boolean thisCall, boolean spreadCall) {
- return make(mc, sender, name, callID, type, safeNavigation, thisCall,
spreadCall, SELECT_METHOD);
+ return makeBoothandle(mc, sender, name, callID, type, safeNavigation,
thisCall, spreadCall, SELECT_METHOD_HANDLE_METHOD);
}
/**
* Makes an adapter method for method selection, i.e. get the cached
methodhandle(fast path) or fallback
*/
private static MethodHandle makeAdapter(MutableCallSite mc, Class<?>
sender, String name, int callID, MethodType type, boolean safeNavigation,
boolean thisCall, boolean spreadCall) {
- return make(mc, sender, name, callID, type, safeNavigation, thisCall,
spreadCall, FROM_CACHE_METHOD);
+ return makeBoothandle(mc, sender, name, callID, type, safeNavigation,
thisCall, spreadCall, FROM_CACHE_HANDLE_METHOD);
Review Comment:
The method name 'makeBoothandle' appears to be a typo. It should likely be
'makeBootHandle' with a capital 'H' to follow standard Java camelCase naming
conventions for compound words.
##########
src/test/groovy/org/codehaus/groovy/vmplugin/v8/IndyInterfaceDeprecatedTest.groovy:
##########
@@ -0,0 +1,105 @@
+/*
+ * 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 org.junit.jupiter.api.Test
+
+import java.lang.invoke.MethodHandles
+import java.lang.invoke.MethodType
+
+import static org.junit.jupiter.api.Assertions.assertEquals
+
+final class IndyInterfaceDeprecatedTest {
+
+ private String foo() {
+ return "foo-result"
+ }
+
+ @Test
+ void testSelectMethodDeprecatedInstanceFoo() {
+ // Prepare call site type: (IndyInterfaceDeprecatedTest) -> Object
+ MethodHandles.Lookup lookup = MethodHandles.lookup()
+ MethodType type = MethodType.methodType(Object,
IndyInterfaceDeprecatedTest)
+ CacheableCallSite callSite = new CacheableCallSite(type, lookup)
+
+ // Provide non-null default/fallback targets (needed for guards in
Selector)
+ def dummyTarget = MethodHandles.dropArguments(
+ MethodHandles.constant(Object, null), 0, type.parameterArray()
+ )
+ callSite.defaultTarget = dummyTarget
+ callSite.fallbackTarget = dummyTarget
+
+ // Prepare invocation arguments
+ def receiver = new IndyInterfaceDeprecatedTest()
+ Object[] args = [receiver] as Object[]
+
+ // Call deprecated selectMethod with the requested flags
+ int callID = IndyInterface.CallType.METHOD.getOrderNumber() //
expected to be 0
+ Object result = IndyInterface.selectMethod(
+ callSite,
+ IndyInterfaceDeprecatedTest, // sender
+ 'foo', // methodName
+ callID,
+ Boolean.FALSE, // safeNavigation
+ Boolean.TRUE, // thisCall (instance method)
+ Boolean.FALSE, // spreadCall
+ 1, // dummyReceiver (marker only)
+ args
+ )
+
+ // Verify the local method foo was actually called
+ assertEquals(receiver.foo(), result)
+ }
+
+ @Test
+ void testFromCacheDeprecatedInstanceFoo() {
+ // Prepare call site type: (IndyInterfaceDeprecatedTest) -> Object
+ MethodHandles.Lookup lookup = MethodHandles.lookup()
+ MethodType type = MethodType.methodType(Object,
IndyInterfaceDeprecatedTest)
+ CacheableCallSite callSite = new CacheableCallSite(type, lookup)
+
+ // Provide non-null default/fallback targets (needed for guards in
Selector)
+ def dummyTarget = MethodHandles.dropArguments(
+ MethodHandles.constant(Object, null), 0, type.parameterArray()
+ )
+ callSite.defaultTarget = dummyTarget
+ callSite.fallbackTarget = dummyTarget
+
+ // Prepare invocation arguments
+ def receiver = new IndyInterfaceDeprecatedTest()
+ Object[] args = [receiver] as Object[]
+
+ // Call deprecated fromCache with the requested flags
+ int callID = IndyInterface.CallType.METHOD.getOrderNumber() //
expected to be 0
+ Object result = IndyInterface.fromCache(
+ callSite,
+ IndyInterfaceDeprecatedTest, // sender
+ 'foo', // methodName
+ callID,
+ Boolean.FALSE, // safeNavigation
+ Boolean.TRUE, // thisCall (instance method)
+ Boolean.FALSE, // spreadCall
+ 1, // dummyReceiver (marker only)
+ args
+ )
+
+ // Verify the local method foo was actually called
+ assertEquals(receiver.foo(), result)
+ }
+}
Review Comment:
The PR removes a test that verified IndyInterface appears in stack traces,
which aligns with the goal of avoiding IndyInterface polluting traces. However,
there is no new test added to verify the intended behavior - that IndyInterface
does NOT appear in stack traces when using the new boothandle-based approach.
Consider adding a test that throws an exception and verifies that IndyInterface
is not present in the stack trace, similar to the removed test but with the
opposite assertion.
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/IndyInterface.java:
##########
@@ -153,30 +153,31 @@ public int getOrderNumber() {
public static final MethodHandles.Lookup LOOKUP = MethodHandles.lookup();
/**
- * handle for the fromCache method
+ * handle for the fromCacheHandle method
*/
- private static final MethodHandle FROM_CACHE_METHOD;
+ private static final MethodHandle FROM_CACHE_HANDLE_METHOD;
/**
- * handle for the selectMethod method
+ * handle for the selectMethodHandle method
*/
- private static final MethodHandle SELECT_METHOD;
+ private static final MethodHandle SELECT_METHOD_HANDLE_METHOD;
+
+ /**
+ * shared invoker for cached method handles
+ */
+ private static final MethodHandle CACHED_INVOKER;
static {
try {
- MethodType mt = MethodType.methodType(Object.class,
CacheableCallSite.class, Class.class, String.class, int.class, Boolean.class,
Boolean.class, Boolean.class, Object.class, Object[].class);
- FROM_CACHE_METHOD = LOOKUP.findStatic(IndyInterface.class,
"fromCache", mt);
+ MethodType handleMt = MethodType.methodType(MethodHandle.class,
CacheableCallSite.class, Class.class, String.class, int.class, Boolean.class,
Boolean.class, Boolean.class, Object.class, Object[].class);
+ FROM_CACHE_HANDLE_METHOD = LOOKUP.findStatic(IndyInterface.class,
"fromCacheHandle", handleMt);
+ SELECT_METHOD_HANDLE_METHOD =
LOOKUP.findStatic(IndyInterface.class, "selectMethodHandle", handleMt);
} catch (Exception e) {
throw new GroovyBugError(e);
}
- try {
- MethodType mt = MethodType.methodType(Object.class,
CacheableCallSite.class, Class.class, String.class, int.class, Boolean.class,
Boolean.class, Boolean.class, Object.class, Object[].class);
- SELECT_METHOD = LOOKUP.findStatic(IndyInterface.class,
"selectMethod", mt);
- } catch (Exception e) {
- throw new GroovyBugError(e);
- }
+ CACHED_INVOKER =
MethodHandles.exactInvoker(MethodType.methodType(Object.class, Object[].class));
Review Comment:
The initialization of CACHED_INVOKER at line 180 is outside the try-catch
block, while it performs MethodHandle operations that could potentially throw
exceptions. For consistency with the error handling of FROM_CACHE_HANDLE_METHOD
and SELECT_METHOD_HANDLE_METHOD, consider wrapping the CACHED_INVOKER
initialization in the same try-catch block or a separate one to ensure proper
error handling and consistent GroovyBugError wrapping.
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/IndyInterface.java:
##########
@@ -254,19 +255,39 @@ private static CallSite
realBootstrap(MethodHandles.Lookup caller, String name,
* Makes a fallback method for an invalidated method selection
*/
protected static MethodHandle makeFallBack(MutableCallSite mc, Class<?>
sender, String name, int callID, MethodType type, boolean safeNavigation,
boolean thisCall, boolean spreadCall) {
- return make(mc, sender, name, callID, type, safeNavigation, thisCall,
spreadCall, SELECT_METHOD);
+ return makeBoothandle(mc, sender, name, callID, type, safeNavigation,
thisCall, spreadCall, SELECT_METHOD_HANDLE_METHOD);
}
/**
* Makes an adapter method for method selection, i.e. get the cached
methodhandle(fast path) or fallback
Review Comment:
The comment references "methodhandle" as one word without proper camelCase
capitalization. For consistency with Java naming conventions and other code in
this file, it should be "method handle" (two words) or "MethodHandle" (when
referring to the class).
```suggestion
* Makes an adapter method for method selection, i.e. get the cached
MethodHandle (fast path) or fallback
```
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/IndyInterface.java:
##########
@@ -352,8 +383,18 @@ private static boolean bypassCache(Boolean spreadCall,
Object[] arguments) {
/**
* Core method for indy method selection using runtime types.
+ * @deprecated Use the new boothandle-based approach instead.
Review Comment:
The deprecation message "Use the new boothandle-based approach instead"
contains the typo "boothandle" which should be "boot handle" or "bootHandle"
for consistency with Java naming conventions. This same typo appears in both
deprecated methods.
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/IndyInterface.java:
##########
@@ -254,19 +255,39 @@ private static CallSite
realBootstrap(MethodHandles.Lookup caller, String name,
* Makes a fallback method for an invalidated method selection
*/
protected static MethodHandle makeFallBack(MutableCallSite mc, Class<?>
sender, String name, int callID, MethodType type, boolean safeNavigation,
boolean thisCall, boolean spreadCall) {
- return make(mc, sender, name, callID, type, safeNavigation, thisCall,
spreadCall, SELECT_METHOD);
+ return makeBoothandle(mc, sender, name, callID, type, safeNavigation,
thisCall, spreadCall, SELECT_METHOD_HANDLE_METHOD);
}
/**
* Makes an adapter method for method selection, i.e. get the cached
methodhandle(fast path) or fallback
*/
private static MethodHandle makeAdapter(MutableCallSite mc, Class<?>
sender, String name, int callID, MethodType type, boolean safeNavigation,
boolean thisCall, boolean spreadCall) {
- return make(mc, sender, name, callID, type, safeNavigation, thisCall,
spreadCall, FROM_CACHE_METHOD);
+ return makeBoothandle(mc, sender, name, callID, type, safeNavigation,
thisCall, spreadCall, FROM_CACHE_HANDLE_METHOD);
}
- private static MethodHandle make(MutableCallSite mc, Class<?> sender,
String name, int callID, MethodType type, boolean safeNavigation, boolean
thisCall, boolean spreadCall, MethodHandle originalMH) {
- MethodHandle mh = MethodHandles.insertArguments(originalMH, 0, mc,
sender, name, callID, safeNavigation, thisCall, spreadCall, /*dummy receiver:*/
1);
- return mh.asCollector(Object[].class,
type.parameterCount()).asType(type);
+ private static MethodHandle makeBoothandle(MutableCallSite mc, Class<?>
sender, String name, int callID, MethodType type, boolean safeNavigation,
boolean thisCall, boolean spreadCall, MethodHandle handleReturningMh) {
+ // Step 1: bind site-constant arguments (incl dummy receiver marker)
+ MethodHandle fromCacheBound = MethodHandles.insertArguments(
+ handleReturningMh,
+ 0, mc, sender, name, callID,
+ safeNavigation, thisCall, spreadCall,
+ /*dummy receiver*/ 1
+ );
+ // fromCacheBound: (Object receiver, Object[] args) → MethodHandle
+
+ // Step 2: fold into the shared invoker (MethodHandle, Object[]) →
Object
+ MethodHandle boothandle = MethodHandles.foldArguments(
+ CACHED_INVOKER, // (MethodHandle, Object[]) → Object
+ fromCacheBound // (Object, Object[]) → MethodHandle
+ );
+ // boothandle: (Object receiver, Object[] args) → Object
Review Comment:
The inline comment at line 276 states "fromCacheBound: (Object receiver,
Object[] args) → MethodHandle", but after binding 8 of the 9 parameters via
insertArguments, the resulting method handle should have signature (Object[]
args) → MethodHandle, not (Object receiver, Object[] args) → MethodHandle.
Similarly, the comment at line 283 may be incorrect. Please verify these
comments accurately reflect the method handle signatures at each step.
```suggestion
// fromCacheBound: (Object[] args) → MethodHandle
// Step 2: fold into the shared invoker (MethodHandle, Object[]) →
Object
MethodHandle boothandle = MethodHandles.foldArguments(
CACHED_INVOKER, // (MethodHandle, Object[]) → Object
fromCacheBound // (Object[] args) → MethodHandle
);
// boothandle: (Object[] args) → Object
```
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/IndyInterface.java:
##########
@@ -254,19 +255,39 @@ private static CallSite
realBootstrap(MethodHandles.Lookup caller, String name,
* Makes a fallback method for an invalidated method selection
*/
protected static MethodHandle makeFallBack(MutableCallSite mc, Class<?>
sender, String name, int callID, MethodType type, boolean safeNavigation,
boolean thisCall, boolean spreadCall) {
- return make(mc, sender, name, callID, type, safeNavigation, thisCall,
spreadCall, SELECT_METHOD);
+ return makeBoothandle(mc, sender, name, callID, type, safeNavigation,
thisCall, spreadCall, SELECT_METHOD_HANDLE_METHOD);
Review Comment:
The method name 'makeBoothandle' appears to be a typo. It should likely be
'makeBootHandle' with a capital 'H' to follow standard Java camelCase naming
conventions for compound words.
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/IndyInterface.java:
##########
@@ -254,19 +255,39 @@ private static CallSite
realBootstrap(MethodHandles.Lookup caller, String name,
* Makes a fallback method for an invalidated method selection
*/
protected static MethodHandle makeFallBack(MutableCallSite mc, Class<?>
sender, String name, int callID, MethodType type, boolean safeNavigation,
boolean thisCall, boolean spreadCall) {
- return make(mc, sender, name, callID, type, safeNavigation, thisCall,
spreadCall, SELECT_METHOD);
+ return makeBoothandle(mc, sender, name, callID, type, safeNavigation,
thisCall, spreadCall, SELECT_METHOD_HANDLE_METHOD);
}
/**
* Makes an adapter method for method selection, i.e. get the cached
methodhandle(fast path) or fallback
*/
private static MethodHandle makeAdapter(MutableCallSite mc, Class<?>
sender, String name, int callID, MethodType type, boolean safeNavigation,
boolean thisCall, boolean spreadCall) {
- return make(mc, sender, name, callID, type, safeNavigation, thisCall,
spreadCall, FROM_CACHE_METHOD);
+ return makeBoothandle(mc, sender, name, callID, type, safeNavigation,
thisCall, spreadCall, FROM_CACHE_HANDLE_METHOD);
}
- private static MethodHandle make(MutableCallSite mc, Class<?> sender,
String name, int callID, MethodType type, boolean safeNavigation, boolean
thisCall, boolean spreadCall, MethodHandle originalMH) {
- MethodHandle mh = MethodHandles.insertArguments(originalMH, 0, mc,
sender, name, callID, safeNavigation, thisCall, spreadCall, /*dummy receiver:*/
1);
- return mh.asCollector(Object[].class,
type.parameterCount()).asType(type);
+ private static MethodHandle makeBoothandle(MutableCallSite mc, Class<?>
sender, String name, int callID, MethodType type, boolean safeNavigation,
boolean thisCall, boolean spreadCall, MethodHandle handleReturningMh) {
Review Comment:
The method name 'makeBoothandle' appears to be a typo. It should likely be
'makeBootHandle' with a capital 'H' to follow standard Java camelCase naming
conventions for compound words.
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/IndyInterface.java:
##########
@@ -254,19 +255,39 @@ private static CallSite
realBootstrap(MethodHandles.Lookup caller, String name,
* Makes a fallback method for an invalidated method selection
*/
protected static MethodHandle makeFallBack(MutableCallSite mc, Class<?>
sender, String name, int callID, MethodType type, boolean safeNavigation,
boolean thisCall, boolean spreadCall) {
- return make(mc, sender, name, callID, type, safeNavigation, thisCall,
spreadCall, SELECT_METHOD);
+ return makeBoothandle(mc, sender, name, callID, type, safeNavigation,
thisCall, spreadCall, SELECT_METHOD_HANDLE_METHOD);
}
/**
* Makes an adapter method for method selection, i.e. get the cached
methodhandle(fast path) or fallback
*/
private static MethodHandle makeAdapter(MutableCallSite mc, Class<?>
sender, String name, int callID, MethodType type, boolean safeNavigation,
boolean thisCall, boolean spreadCall) {
- return make(mc, sender, name, callID, type, safeNavigation, thisCall,
spreadCall, FROM_CACHE_METHOD);
+ return makeBoothandle(mc, sender, name, callID, type, safeNavigation,
thisCall, spreadCall, FROM_CACHE_HANDLE_METHOD);
}
- private static MethodHandle make(MutableCallSite mc, Class<?> sender,
String name, int callID, MethodType type, boolean safeNavigation, boolean
thisCall, boolean spreadCall, MethodHandle originalMH) {
- MethodHandle mh = MethodHandles.insertArguments(originalMH, 0, mc,
sender, name, callID, safeNavigation, thisCall, spreadCall, /*dummy receiver:*/
1);
- return mh.asCollector(Object[].class,
type.parameterCount()).asType(type);
+ private static MethodHandle makeBoothandle(MutableCallSite mc, Class<?>
sender, String name, int callID, MethodType type, boolean safeNavigation,
boolean thisCall, boolean spreadCall, MethodHandle handleReturningMh) {
+ // Step 1: bind site-constant arguments (incl dummy receiver marker)
+ MethodHandle fromCacheBound = MethodHandles.insertArguments(
+ handleReturningMh,
+ 0, mc, sender, name, callID,
+ safeNavigation, thisCall, spreadCall,
+ /*dummy receiver*/ 1
+ );
+ // fromCacheBound: (Object receiver, Object[] args) → MethodHandle
+
+ // Step 2: fold into the shared invoker (MethodHandle, Object[]) →
Object
+ MethodHandle boothandle = MethodHandles.foldArguments(
+ CACHED_INVOKER, // (MethodHandle, Object[]) → Object
+ fromCacheBound // (Object, Object[]) → MethodHandle
+ );
+ // boothandle: (Object receiver, Object[] args) → Object
+
+ // Step 3: adapt to callsite type: collect all arguments into Object[]
and then asType
+ boothandle = boothandle
+ .asCollector(Object[].class, type.parameterCount())
+ .asType(type);
+
+ return boothandle;
Review Comment:
The variable name 'boothandle' appears to be a typo. It should likely be
'bootHandle' with a capital 'H' to follow standard Java camelCase naming
conventions for compound words.
--
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]
