Review of MH/LF patches in the review queue

[Vladimir Ivanov]

Paul, thanks for the feedback!
See my answers inline.

Updated webrevs:
   http://cr.openjdk.java.net/~vlivanov/8037209/webrev.03/
   http://cr.openjdk.java.net/~vlivanov/8038261/webrev.02/

On 4/4/14 3:33 PM, Paul Sandoz wrote:
> Hi,
>
> Reviews of two patches in the queue.
>
> Paul.
>
>
> http://cr.openjdk.java.net/~vlivanov/8037209/webrev.02/
>
> Looks good, though I don't claim to understand all the nuances around casts and JVM/bytecode correctness. Minor stuff below.
>
> InvokerByteCodeGenerator:
> --
>
> Unused method:
>
>      static boolean match(MemberName member, MethodHandle fn) {
>          if (member == null || fn == null)  return false;
>          return member.equals(fn.internalMemberName());
>      }
Done.

>
> MethodHandleImpl
> --
>
> 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.
Moved to 8038261.

>
> VerifyType
> --
>
> 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.
>
> s/difference/different
>
Done.

>
>
> http://cr.openjdk.java.net/~vlivanov/8038261/webrev.01/
>
> To re-iterate this is a nice improvement over the previous approach.
>
> InvokerByteCodeGenerator
> --
>
> For this specialization:
>
>          if (defc == ArrayAccessor.class &&
>                  match(member, ArrayAccessor.OBJECT_ARRAY_GETTER)) {
>              mv.visitInsn(Opcodes.AALOAD);
>          } else if (defc == ArrayAccessor.class &&
>                  match(member, ArrayAccessor.OBJECT_ARRAY_SETTER)) {
>              mv.visitInsn(Opcodes.AASTORE);
>          } 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.
Correct. setElementL/getElementL are interchangeable with aastore/aaload 
bytecode instructions w.r.t. stack layout.

> 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.
It's an interesting idea. I'm not aware of existing logic to achieve it, 
but I'll definitely experiment to see how it shapes out. There's an 
alternative approach using intrinsics, but it doesn't introduce direct 
coupling between MHs and bytecode shapes.

> 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?
Setters/getters for primitive arrays can be special-cased in the same 
manner, but these special cases don't buy much. Accessors 
(ArrayAccessor.getElement*/setElement*) are very simple anyway. I 
haven't seen any significant performance difference on octane.

I'll experiment with that further.

>
> 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();
I did it intentionally. There were bugs when cached values become stale 
due to processing of multi-name intrinsics and they were erroneously 
used. There's a refactoring of how intrinsics are implemented waiting in 
the queue, so I'd like to leave it as is for now.

Best regards,
Vladimir Ivanov