RFR: 8164214: [JVMCI] include VarHandle in signature polymorphic method test

[Christian Thalinger]

> On Aug 22, 2016, at 11:20 AM, Doug Simon <doug.simon at oracle.com> wrote:
> 
> I’ve created https://bugs.openjdk.java.net/browse/JDK-8164586. With that, can I consider this webrev Reviewed?

Yes.

> 
> -Doug
> 
>> On 22 Aug 2016, at 22:46, Doug Simon <doug.simon at oracle.com> wrote:
>> 
>>> 
>>> On 22 Aug 2016, at 21:49, Paul Sandoz <paul.sandoz at oracle.com> wrote:
>>> 
>>>> 
>>>> On 20 Aug 2016, at 00:59, Doug Simon <doug.simon at oracle.com> wrote:
>>>> 
>>>> 
>>>>> On 20 Aug 2016, at 02:26, Paul Sandoz <paul.sandoz at oracle.com> wrote:
>>>>> 
>>>>> It may be better to expose a new enum value in Flags and set the bit based on the _intrinsic_id, rather than JVMCI checking the range.
>>>>> 
>>>>> Thus the Flags enum value for sig-poly remains constant even if the _intrinsic_id values do not (IIUC they will change when new intrinsics are added), thus less needs to be exposed to JVMCI.
>>>> 
>>>> Yes, that’s what I was thinking. The intrinsic_id range seems more subject to change than a flag.
>>>> 
>>>>> e.g. something like below (not tested)?
>>>> 
>>>> In the context of 8164214, I’ve just realised that making it possible to query whether a Method*/HotSpotResolvedJavaMethodImpl is sig-poly is a red herring. In this context, we’re dealing with constant pool resolution so have neither a Method* nor HotSpotResolvedJavaMethodImpl in hand.
>>>> 
>>> 
>>> Drat.
>>> 
>>> 
>>>> Based on this (late - sorry!) realization, I propose to add `String[] CompilerToVM.getSignaturePolymorphicHolderNames()`:
>>>> 
>>>> http://cr.openjdk.java.net/~dnsimon/8164214v2
>>>> 
>>> 
>>> I share Christian’s concerns here, as it’s just pushing the hardcoded dependency around.
>>> 
>>> Looking more at the sig-poly call i see it already checks the klass names:
>>> 
>>> static bool is_signature_polymorphic_name(Klass* klass, Symbol* name) {
>>>  return signature_polymorphic_name_id(klass, name) != vmIntrinsics::_none;
>>> }
>>> 
>>> vmIntrinsics::ID MethodHandles::signature_polymorphic_name_id(Klass* klass, Symbol* name) {
>>>  if (klass != NULL &&
>>>      (klass->name() == vmSymbols::java_lang_invoke_MethodHandle() ||
>>>       klass->name() == vmSymbols::java_lang_invoke_VarHandle())) {
>>>    vmIntrinsics::ID iid = signature_polymorphic_name_id(name);
>>>    if (iid != vmIntrinsics::_none)
>>>      return iid;
>>>    if (is_method_handle_invoke_name(klass, name))
>>>      return vmIntrinsics::_invokeGeneric;
>>>  }
>>>  return vmIntrinsics::_none;
>>> }
>>> 
>>> 
>>> I guess you want to gate things before calling resolveInvokeHandleInPool?
>> 
>> Yes, we want to avoid VM calls where possible, especially on hot paths like constant pool resolution during bytecode parsing.
>>> 
>>> For expediency perhaps it’s best to proceed with the class dependency in JVMCI Java code and then log an issue (noting the JVMCI dependency) to clean up this sig-poly method area properly?
>> 
>> Yes, I was hoping you’d suggest that ;-)
>> 
>>> When i updated this area in HotSpot i took a conservative (and dumb [*]) approach. I found the sig-poly checks quite intertwined and potentially duplicating checks, but i did not wanna wanna mess around with it and start breaking things.
>> 
>> Exactly my reaction when contemplating the more proper solution.
>> 
>>> There are symbol checks for java_lang_invoke_MethodHandle and java_lang_invoke_VarHandle spread a little throughout the code, likewise with system dictionary checks for MethodHandle_klass and VarHandle_klass. I think we could clear all that up as a separate issue.
>> 
>> Sounds good. Thanks for the review.
>> 
>> -Doug
>