[9] RFR (L): 8072008: Emit direct call instead of linkTo* for recursive indy/MH.invoke* calls

[Vladimir Ivanov]

Updated version:
   http://cr.openjdk.java.net/~vlivanov/8072008/webrev.03/

02 vs 03 (replaced previous version; please, refresh):
   http://cr.openjdk.java.net/~vlivanov/8072008/webrev.02_03

I spotted a problem when inline caches are disabled (e.g., on SPARC). In 
that case SharedRuntime::extract_attached_method() can be called on an 
instruction which isn't a call (see SharedRuntime::reresolve_call_site 
[1] for the description).

I could compute call address myself (pc - 
NativeCall::return_address_offset) and look for reloc info entry, but 
decided to use NativeCall factories (nativeCall_before) which do 
verification. So, I have to grab Patching_lock and check there is a 
valid call instruction (NativeCall::is_call_before) before using 
nativeCall_before to compute call address.

>> Do you see a better solution than patching bc before performing method
>> resolution?
>
> The most important thing the bc does is carry the distinction between
> linkToSpecial and linkTo{Virtual,Interface}.  The bc also tells whether
> there is a receiver in the mix, which gates a null check.
>
> Maybe these two aspects of bc need to be clarified, by factoring them
> into a bc and a flag (either "has_receiver" or "null_check_receiver").
Added has_receiver local flag in find_callee_info_helper.

Actually, I'm not sure why the code doesn't compute a receiver for 
invokehandle case. MH.invokeBasic is instance method and NPE should be 
through if the receiver is null. Is it because the adapter is 
responsible for throwing the exception?

I experimented with further steps to separate them, but didn't find 
results satisfactory.

I tried to unify 2 cases (bytecode-based resoluation and attached 
method), but it didn't work out well. I tried to get rid of constant 
pool, but it complicated the logic too much for invokedynamic and 
invokehandle cases.

Doing something special for attached method case didn't feel worth it.

> Would it work to consolidate the logic which derives invoke_bc from bc
> to the place where bc is first computed?  The two derivations of bc
> and later invoke_bc seem awkward to me.
Done. I did it that way initially, but then decided to separate bc & 
invoke_bc to emphasize the bc patching happens.

> In fact, now I wonder how an inlined linkToSpecial will perform a receiver
> null check correctly, in the present logic.
Null check during resolution should happen here:
Handle SharedRuntime::find_callee_info_helper(JavaThread* thread,
   ...
   bool has_receiver = bc != Bytecodes::_invokestatic &&
                       bc != Bytecodes::_invokedynamic &&
                       bc != Bytecodes::_invokehandle;

   // Find receiver for non-static call
   if (has_receiver) {
...
     if (receiver.is_null()) {
       THROW_(vmSymbols::java_lang_NullPointerException(), nullHandle);
     }

Or do you have another scenario on your mind?

>> Moved resolve_attached_call_info to LinkResolver::resolve_invoke overload.
>
> Good.  But this signature seems odd to me, since LinkResolver results
> are leading parameters.
>
> +   static void resolve_invoke(methodHandle info, Bytecodes::Code bc,
> +                              Handle& receiver, CallInfo& callinfo, TRAPS);
> +
>
> Also, we use "const methodHandle&" more these days.
>
> To maximize correspondence with the neighboring function, maybe this?
>
> +   static void resolve_invoke(CallInfo& result, Handle recv,
>                               const methodHandle& attached_method,
>                               Bytecodes::Code byte, TRAPS);
Fixed.

>>>> Maybe a special check in CallGenerator::generate() to catch failed
>>> inlines through MH.linkTo/invokeBasic?
>> ... and it looks pretty decent:
>> http://cr.openjdk.java.net/~vlivanov/8072008/webrev.02_03
>
> Yes, I agree that is clearer.
>
> But I think you need to adjust is_call_consistent_with_jvms some more.
> The call to is_inlined_mh_linker appears to be in a place where it
> can never return true.
It does return true when (!cg->is_inline() && 
!resolved_method->is_method_handle_intrinsic()). Exactly the case when 
the linker was eliminated, but inlining does not happen. But I 
refactored that code anyway to do additional verification.

> Also, please consider walking the parallel argument lists for the MH calls
> to check for oop/32/64 mismatches.  You can do it easily with a pair of
> SignatureStream iterators.
Done. It was not that easy (see check_inlined_mh_linker_info in 
doCall.cpp). The main complication is virtual vs static methods in 
MH.invokeBasic case.

Best regards,
Vladimir Ivanov

[1] 
http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/e29bf04214ed/src/share/vm/runtime/sharedRuntime.cpp#l1606