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

[Dean Long]

Great, just checking!

dl

On 10/30/2015 4:27 PM, Vladimir Ivanov wrote:
> No, it's not needed. All relocations refer to recorded metadata 
> section in the nmethod:
>
> + Method* virtual_call_Relocation::method_value() {
> +   Metadata* m = code()->metadata_at(_method_index);
> +   assert(m != NULL || _method_index == 0, "should be non-null for 
> non-zero index");
> +   assert(m == NULL || m->is_method(), "not a method");
> +   return (Method*)m;
> + }
>
> The following code in nmethod::metadata_do should enumerate them:
>
>   // Visit the metadata section
>   for (Metadata** p = metadata_begin(); p < metadata_end(); p++) {
>     if (*p == Universe::non_oop_word() || *p == NULL)  continue; // 
> skip non-oops
>     Metadata* md = *p;
>     f(md);
>   }
>
> Special cases are needed for embedded metadata.
>
> And recording happens in resolved_method_index (see *.ad changes):
>
> src/share/vm/opto/machnode.hpp:
> +   int resolved_method_index(CodeBuffer &cbuf) const {
> +     if (_override_symbolic_info) {
> +       // Attach corresponding Method* to the call site, so VM can 
> use it during resolution
> +       // instead of querying symbolic info from bytecode.
> +       assert(_method != NULL, "method should be set");
> +       assert(_method->constant_encoding()->is_method(), "should 
> point to a Method");
> +       return 
> cbuf.oop_recorder()->find_index(_method->constant_encoding());
> +     }
> +     return 0; // Use symbolic info from bytecode (resolved_method == 
> NULL).
> +   }
> Best regards,
> Vladimir Ivanov
>
> On 10/30/15 10:46 PM, Dean Long wrote:
>> Do you need *_call_Relocation::method_value() to be scanned in any new
>> places,
>> like nmethod::metadata_do?
>>
>> dl
>>
>> On 10/29/2015 2:36 PM, Vladimir Ivanov wrote:
>>> http://cr.openjdk.java.net/~vlivanov/8072008/webrev.01/
>>> https://bugs.openjdk.java.net/browse/JDK-8072008
>>>
>>> NB! It touches aarch64 & ppc code, so I'd like to hear from the
>>> maintainers whether the changes are fine.
>>>
>>> To simplify review process, the changes can be split in 2 parts.
>>>
>>> http://cr.openjdk.java.net/~vlivanov/8072008/webrev.01/hotspot.reloc/
>>>
>>> Platform-dependent changes in macro assembler to switch from
>>> relocation types to relocation instances, and attach pre-resolved
>>> method to call sites in generated code.
>>>
>>> http://cr.openjdk.java.net/~vlivanov/8072008/webrev.01/hotspot.attach/
>>> High-level adjustments in C2 and test cases.
>>>
>>> Overview
>>>
>>> There's a performance problem with inlining through
>>> MH.linkTo*/invokeBasic linkers. When MemberName/receiver is constant
>>> and VM sees the target method during compilation, but decides not to
>>> inline (e.g. recursive inlining), a call to linker is generated, but
>>> more optimal code shape is to call target method directly.
>>>
>>> The problem with substituting a linker call by a direct/virtual call
>>> is that call site resolution logic relies on bytecode info (see
>>> SharedRuntime::resolve_{static,opt_virtual,virtual}_call_C and
>>> SharedRuntime::find_callee_info_helper). But on bytecode level
>>> symbolic reference points to the linker method, which doesn't tell
>>> anything about target method.
>>>
>>> The fix is to attach additional metadata (Method*) to the call site,
>>> so VM can use it instead of inspecting bytecode when linking the call
>>> site. The Method* is placed in relocation table (for static_call,
>>> opt_virtual_call, and virtual_call relocation types) and contains a
>>> Method* the call site is resolved to by JIT compiler (_method from a
>>> call IR node).
>>>
>>> It is used only in C2 generated code.
>>>
>>> Example:
>>>
>>>   MemberName mn = ... <<T.f1>> ... ; // compile-time constant
>>>   MethodHandle.linkToStatic(..., mn)
>>>
>>> compiles to:
>>>
>>>   callq  0x0000000109cb1900  ; OopMap{off=28}
>>>                              ; *invokestatic linkToStatic
>>>                              ; - Test::invokeStatic at 3 (line 108)
>>>                              ; {static_call T.f1}
>>>
>>> It's a call to MethodHandle.linkToStatic on bytecode level, but in
>>> generated code it's a direct call to T.f1.
>>>
>>> For testing purposes, 2 new WhiteBox methods are introduced:
>>>   - clearInlineCaches
>>>   - deoptimize
>>>
>>> They are used in unit tests (test/compiler/jsr292/NonInlinedCall/*)
>>> which stresses new code shapes.
>>>
>>> WhiteBox changes require some adjustments in build scripts, since
>>> @HotSpotIntrinsicCandidate isn't part of JDK8. But it will be fixed
>>> separately (the idea is to start building wb.jar with JDK9).
>>>
>>> Testing: JPRT, java/lang/invoke, nashorn, octane
>>>
>>> Thanks!
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>