RFR(XS): 8163533: jdk.vm.ci.hotspot.test.MethodHandleAccessProviderTest fails on jdk9/dev

[Doug Simon]

> On 12 Aug 2016, at 17:37, Michael Haupt <michael.haupt at oracle.com> wrote:
> 
> Hi Doug,
> 
> while that obviously works in this very simple case (which is currently the only use case), I don't think it's a very stable solution to represent signatures as strings rather than in terms of types.

In this case, it should be stable since everything is guaranteed to be loaded by the same class loader and the only type involved is void (which is obviously class loader independent). If someone subsequently gets the signature string wrong, the lookup will fail fast.

Remember, we don’t need a general solution here - it’s a single use helper method in a static initializer.

> If the extra work you mention is really unacceptable, I'd rather adopt the first solution that added a separate check for the array length.
> 
> I'll move forward with the solution at http://cr.openjdk.java.net/~mhaupt/8163533/webrev.00/ now.

I hope you’re open to changing your mind on this based on the above ;-)

-Doug

> 
>> Am 12.08.2016 um 00:21 schrieb Doug Simon <doug.simon at Oracle.COM>:
>> 
>> Use of HotSpotSignature adds this extra work:
>> 
>> - the parameter types array will be cloned
>> - a list of parameter type names will be built
>> - the string form of the signature will be constructed even if the exception won’t be thrown
>> 
>> You can avoid all this by simply using the string form of the signature everywhere:
>> 
>> diff -r 5fde6ccb4092 src/jdk.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotMethodHandleAccessProvider.java
>> --- a/src/jdk.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotMethodHandleAccessProvider.java       	Tue Aug 09 16:47:47 2016 +0300
>> +++ b/src/jdk.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotMethodHandleAccessProvider.java       	Fri Aug 12 09:20:06 2016 +0200
>> @@ -25,6 +25,7 @@
>> import static jdk.vm.ci.hotspot.CompilerToVM.compilerToVM;
>> import static jdk.vm.ci.hotspot.HotSpotJVMCIRuntime.runtime;
>> import static jdk.vm.ci.hotspot.HotSpotResolvedObjectTypeImpl.fromObjectClass;
>> +
>> import jdk.vm.ci.common.JVMCIError;
>> import jdk.vm.ci.meta.ConstantReflectionProvider;
>> import jdk.vm.ci.meta.JavaConstant;
>> @@ -33,8 +34,8 @@
>> import jdk.vm.ci.meta.ResolvedJavaField;
>> import jdk.vm.ci.meta.ResolvedJavaMethod;
>> import jdk.vm.ci.meta.ResolvedJavaType;
>> -import jdk.vm.ci.meta.Signature;
>> 
>> +// @formatter:off
>> public class HotSpotMethodHandleAccessProvider implements MethodHandleAccessProvider {
>> 
>>     private final ConstantReflectionProvider constantReflection;
>> @@ -78,53 +79,28 @@
>>             throw new NoSuchFieldError(fieldType.getName() + " " + className + "." + fieldName);
>>         }
>> 
>> -        private static ResolvedJavaMethod findMethodInClass(String className, String methodName,
>> -                ResolvedJavaType resultType, ResolvedJavaType[] parameterTypes) throws ClassNotFoundException {
>> +        private static ResolvedJavaMethod findMethodInClass(String className, String methodName, String signature) throws ClassNotFoundException {
>>             Class<?> clazz = Class.forName(className);
>>             HotSpotResolvedObjectTypeImpl type = fromObjectClass(clazz);
>>             ResolvedJavaMethod result = null;
>>             for (ResolvedJavaMethod method : type.getDeclaredMethods()) {
>> -                if (method.getName().equals(methodName) && signatureMatches(method, resultType, parameterTypes)) {
>> +                if (method.getName().equals(methodName) && method.getSignature().toMethodDescriptor().equals(signature)) {
>>                     result = method;
>>                 }
>>             }
>>             if (result == null) {
>> -                StringBuilder sig = new StringBuilder("(");
>> -                for (ResolvedJavaType t : parameterTypes) {
>> -                    sig.append(t.getName()).append(",");
>> -                }
>> -                if (sig.length() > 1) {
>> -                    sig.replace(sig.length() - 1, sig.length(), ")");
>> -                } else {
>> -                    sig.append(')');
>> -                }
>> -                throw new NoSuchMethodError(resultType.getName() + " " + className + "." + methodName + sig.toString());
>> +                throw new NoSuchMethodError(className + "." + methodName + signature);
>>             }
>>             return result;
>>         }
>> 
>> -        private static boolean signatureMatches(ResolvedJavaMethod m, ResolvedJavaType resultType,
>> -                ResolvedJavaType[] parameterTypes) {
>> -            Signature s = m.getSignature();
>> -            if (!s.getReturnType(CLASS).equals(resultType)) {
>> -                return false;
>> -            }
>> -            for (int i = 0; i < s.getParameterCount(false); ++i) {
>> -                if (!s.getParameterType(i, CLASS).equals(parameterTypes[i])) {
>> -                    return false;
>> -                }
>> -            }
>> -            return true;
>> -        }
>> -
>>         static {
>>             try {
>>                 methodHandleFormField = findFieldInClass("java.lang.invoke.MethodHandle", "form",
>>                     fromObjectClass(Class.forName("java.lang.invoke.LambdaForm")));
>>                 lambdaFormVmentryField = findFieldInClass("java.lang.invoke.LambdaForm", "vmentry",
>>                     fromObjectClass(Class.forName("java.lang.invoke.MemberName")));
>> -                lambdaFormCompileToBytecodeMethod = findMethodInClass("java.lang.invoke.LambdaForm", "compileToBytecode",
>> -                    new HotSpotResolvedPrimitiveType(JavaKind.Void), new ResolvedJavaType[]{});
>> +                lambdaFormCompileToBytecodeMethod = findMethodInClass("java.lang.invoke.LambdaForm", "compileToBytecode", "()V");
>>                 memberNameVmtargetField = (HotSpotResolvedJavaField) findFieldInClass("java.lang.invoke.MemberName", "vmtarget",
>>                     new HotSpotResolvedPrimitiveType(JavaKind.Long));
>>             } catch (Throwable ex) {
>> 
>> -Doug
>> 
>>> On 12 Aug 2016, at 07:07, Aleksey Shipilev <aleksey.shipilev at gmail.com> wrote:
>>> 
>>> On 08/12/2016 02:33 AM, Michael Haupt wrote:
>>>> thank you. That is a worthwhile suggestion that I'd like to take
>>>> up: http://cr.openjdk.java.net/~mhaupt/8163533/webrev.01/
>>>> <http://cr.openjdk.java.net/%7Emhaupt/8163533/webrev.01/> - please
>>>> review. I've started the test job for this change and will push once
>>>> both your review and the results are in.
>>> 
>>> Looks fine to me.
>>> 
>>> I would probably go deeper and used Signature.toMethodDescriptor() for
>>> generating NSME error message too. Then, ultimately, making
>>> findMethodInClass accept Signature would be the last nail into this.
>>> 
>>> Thanks,
>>> -Aleksey
>>> 
>> 
> 
> -- 
> 
> 
> Dr. Michael Haupt | Principal Member of Technical Staff
> Phone: +49 331 200 7277 | Fax: +49 331 200 7561
> Oracle Java Platform Group | LangTools Team | Nashorn
> Oracle Deutschland B.V. & Co. KG | Schiffbauergasse 14 | 14467 Potsdam, Germany
> 
> ORACLE Deutschland B.V. & Co. KG | Hauptverwaltung: Riesstraße 25, D-80992 München
> Registergericht: Amtsgericht München, HRA 95603
> 
> Komplementärin: ORACLE Deutschland Verwaltung B.V. | Hertogswetering 163/167, 3543 AS Utrecht, Niederlande
> Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697
> Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher
> 	Oracle is committed to developing practices and products that help protect the environment
>