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

[Michael Haupt]

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. 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.

Best,

Michael

> 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
>> 
> 

-- 

 <http://www.oracle.com/>
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
 <http://www.oracle.com/commitment>	Oracle is committed to developing practices and products that help protect the environment

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20160812/1181201c/attachment-0001.html>