Review comments

Author: liach

src/java.base/share/classes/jdk/internal/reflect/MethodHandleAccessorFactory.java

@@ -210,7 +210,7 @@ static FieldAccessorImpl newFieldAccessor(Field field, boolean isReadOnly) {
     }
 
     private static MethodHandle getDirectMethod(Method method, boolean callerSensitive) throws IllegalAccessException {
-        var mtype = methodType(method.getReturnType(), method.getParameterTypes());
+        var mtype = methodType(method.getReturnType(), reflectionFactory.getExecutableSharedParameterTypes(method));
         var isStatic = Modifier.isStatic(method.getModifiers());
         var dmh = isStatic ? JLIA.findStatic(method.getDeclaringClass(), method.getName(), mtype)
                                         : JLIA.findVirtual(method.getDeclaringClass(), method.getName(), mtype);
@@ -232,7 +232,7 @@ private static MethodHandle getDirectMethod(Method method, boolean callerSensiti
     private static MethodHandle findCallerSensitiveAdapter(Method method) throws IllegalAccessException {
         String name = method.getName();
         // append a Class parameter
-        MethodType mtype = methodType(method.getReturnType(), method.getParameterTypes())
+        MethodType mtype = methodType(method.getReturnType(), reflectionFactory.getExecutableSharedParameterTypes(method))
                                 .appendParameterTypes(Class.class);
         boolean isStatic = Modifier.isStatic(method.getModifiers());
 
@@ -370,11 +370,15 @@ private static boolean useNativeAccessor(Executable member) {
         if (member instanceof Method method && isSignaturePolymorphicMethod(method))
             return true;
 
-        // java.lang.invoke fails to create MH for bad ACC_VARARGS methods with no
-        // trailing array,  but core reflection ignores ACC_VARARGS flag like the JVM does.
-        // Fall back to use the native implementation instead.
-        if (isInvalidVarArgs(member))
+        // Lookup always calls MethodHandle::setVarargs on a member with ACC_VARARGS
+        // bit set, which verifies that the last parameter of the member must be
+        // an array type.  Such restriction does not exist in core reflection
+        // and the JVM.  Fall back to use the native implementation instead.
+        int paramCount = member.getParameterCount();
+        if (member.isVarArgs() &&
+                (paramCount == 0 || !(reflectionFactory.getExecutableSharedParameterTypes(member)[paramCount-1].isArray()))) {
             return true;
+        }
 
         // A method handle cannot be created if its type has an arity >= 255
         // as the method handle's invoke method consumes an extra argument
@@ -395,7 +399,7 @@ private static boolean useNativeAccessor(Executable member) {
      */
     private static int slotCount(Executable member) {
         int slots = 0;
-        Class<?>[] ptypes = member.getParameterTypes();
+        Class<?>[] ptypes = reflectionFactory.getExecutableSharedParameterTypes(member);
         for (Class<?> ptype : ptypes) {
             if (ptype == double.class || ptype == long.class) {
                 slots++;
@@ -430,24 +434,6 @@ public static boolean isSignaturePolymorphicMethod(Method method) {
         return parameters.length == 1 && parameters[0] == Object[].class;
     }
 
-    /**
-     * Lookup always calls MethodHandle::setVarargs on a member with varargs modifier
-     * bit set, which verifies that the last parameter of the member must be an array type.
-     * Thus, Lookup cannot create MethodHandle for such methods or constructors.
-     * The JVMS does not require that the last parameter descriptor of the method descriptor
-     * is an array type if the ACC_VARARGS flag is set in the access_flags item.
-     * Core reflection also has no variable arity support and ignores the ACC_VARARGS flag,
-     * treating them as regular arguments.
-     */
-    private static boolean isInvalidVarArgs(Executable member) {
-        if (!member.isVarArgs())
-            return false;
-
-        Class<?>[] parameters = reflectionFactory.getExecutableSharedParameterTypes(member);
-        var count = parameters.length;
-        return count == 0 || !parameters[count - 1].isArray();
-    }
-
     /*
      * Delay initializing these static fields until java.lang.invoke is fully initialized.
      */

test/jdk/java/lang/invoke/MethodHandleInvokeUOE.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2021, 2024, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -21,16 +21,15 @@
  * questions.
  */
 
-/*
- * @test
- * @bug 8343377
+/* @test
  * @summary Test MethodHandle::invokeExact and MethodHandle::invoke throws
  *          UnsupportedOperationException when called via Method::invoke
  * @run testng test.java.lang.invoke.MethodHandleInvokeUOE
  */
 
 package test.java.lang.invoke;
 
+import org.testng.*;
 import org.testng.annotations.*;
 
 import java.lang.invoke.MethodHandle;

test/jdk/java/lang/invoke/VarHandles/VarHandleTestReflection.java

@@ -23,7 +23,6 @@
 
 /*
  * @test
- * @bug 8335638 8343377
  * @run testng VarHandleTestReflection
  */
 

test/micro/org/openjdk/bench/java/lang/reflect/NativeMethodInvoke.java

@@ -0,0 +1,79 @@
+/*
+ * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+package org.openjdk.bench.java.lang.reflect;
+
+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.lang.reflect.Method;
+import java.util.concurrent.TimeUnit;
+
+/**
+ * Benchmark for regression in native method invocation.
+ */
+@BenchmarkMode(Mode.AverageTime)
+@OutputTimeUnit(TimeUnit.NANOSECONDS)
+@State(Scope.Thread)
+@Warmup(iterations = 10, time = 1, timeUnit = TimeUnit.SECONDS)
+@Measurement(iterations = 5, time = 2, timeUnit = TimeUnit.SECONDS)
+@Fork(3)
+public class NativeMethodInvoke {
+
+    private Method objectHashCode;
+    private Method threadCurrentThread;
+
+    private Object[] objects;
+
+    @Setup
+    public void setup() throws ReflectiveOperationException {
+        objects = new Object[]{
+                1, 5L,
+                5.6d, 23.11f,
+                Boolean.TRUE, 'd'
+        };
+
+        objectHashCode = Object.class.getDeclaredMethod("hashCode");
+        threadCurrentThread = Thread.class.getDeclaredMethod("currentThread");
+    }
+
+    @Benchmark
+    public void objectHashCode(Blackhole bh) throws ReflectiveOperationException {
+        for (var obj : objects) {
+            bh.consume(objectHashCode.invoke(obj));
+        }
+    }
+
+    @Benchmark
+    public Object threadCurrentThread() throws ReflectiveOperationException {
+        return threadCurrentThread.invoke(null);
+    }
+}