Review of MH/LF patches in the review queue

Paul Sandoz paul.sandoz at
Fri Apr 4 11:33:42 UTC 2014


Reviews of two patches in the queue.


Looks good, though I don't claim to understand all the nuances around casts and JVM/bytecode correctness. Minor stuff below.


Unused method:

    static boolean match(MemberName member, MethodHandle fn) {
        if (member == null || fn == null)  return false;
        return member.equals(fn.internalMemberName());


If you are gonna remove the weakly typed wrapper methods for array access, you might as well remove USE_WEAKLY_TYPED_ARRAY_ACCESSORS and it's use?

-         // Weakly typed wrappers of Object[] accessors:
-         static Object  getElementL(Object    a, int i)            { return getElementL((Object[])a, i); }
-         static void    setElementL(Object    a, int i, Object  x) {        setElementL((Object[]) a, i, x); }
-         static Object  getElementL(Object   arrayClass, Object a, int i)             { return getElementL((Class<?>) arrayClass, (Object[])a, i); }
-         static void    setElementL(Object   arrayClass, Object a, int i, Object x)   {        setElementL((Class<?>) arrayClass, (Object[])a, i, x); }

Or otherwise for expediency just leave it in until the array improvements patch follows? IMHO better to be consistent either way.


Typo in docs:

     * <p>
     * The primitive type 'void' does not interconvert with any other type,
     * even though it is legal to drop any type from the stack and "return void".
     * The stack effects, though are difference between void and any other type,
     * so it is safer to report a non-trivial conversion.


To re-iterate this is a nice improvement over the previous approach.


For this specialization:

        if (defc == ArrayAccessor.class &&
                match(member, ArrayAccessor.OBJECT_ARRAY_GETTER)) {
        } else if (defc == ArrayAccessor.class &&
                match(member, ArrayAccessor.OBJECT_ARRAY_SETTER)) {
        } else if (member.isMethod()) {

IIUC all stuff on the stack is correctly placed for the substitution of the invokestatic instruction to ArrayAccessor.set/getElementL with an aastore/load instruction. This makes we wonder if there is a more general approach for direct or direct-like MHs to be visited and provide their own snippets of ASM producing code.

Is it worthwhile introducing such direct coupling for a specific case, as that tends to increase the inter-connective complexity of the code. How much performance gain is achieved?

The last two re-assigments are never used and are reassigned again at the top of the loop:

            // Update cached form name's info in case an intrinsic spanning multiple names was encountered.
            name = lambdaForm.names[i];
            member = name.function.member();
            rtype = name.function.methodType().returnType();

More information about the core-libs-dev mailing list