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

[John Rose]

On Nov 23, 2015, at 10:31 AM, Vladimir Ivanov <vladimir.x.ivanov at oracle.com> wrote:
> 
> Updated version:
>  http://cr.openjdk.java.net/~vlivanov/8072008/webrev.03/ <http://cr.openjdk.java.net/~vlivanov/8072008/webrev.03/>

Good, ship it!

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

A non-constant MH, if it goes null, will cause a SEGV due to the indirection to MH.form.vmentry.
So we'd get a crash if MH invokeBasic dispatcher were ever called on NULL.
But the interpreter and JITs emit null checks first.
See TemplateTable::invokehandle, DirectCallGenerator::generate, GraphBuilder::invoke.

The special null checks done in SharedRuntime are corner cases which only apply
to the first execution of a given invocation bytecode.  I think there might be a
corner-of-a-corner case where a pathological invocation is never fully resolved,
and always goes through SharedRuntime.

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

If the bc that controls has_receiver is rewritten from invokestatic (for linkToSpecial)
to invokespecial (for the target method), then the null check will happen.
I suppose I was confused which bc was which.  It looks good now.
(I am also permanently confused about why there is null checking during resolution.
I think there's a reason, but it's not one I want to spend gray matter on.)

…

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

Well, there were a number of moving parts in there.  I'm glad you did this; it provides useful documentation of how MH infra. works.

Did that check find any bugs?  If not, you might inject one fault (e.g. linkToStatic instead of linkToSpecial) and verify that the pre-crash dumping code works.

Thank you!

— John
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20151123/9cc02ede/attachment.html>