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

[Doug Simon]

I’ve created https://bugs.openjdk.java.net/browse/JDK-8164586. With that, can I consider this webrev Reviewed?

-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