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

[Doug Simon]

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

Based on this (late - sorry!) realization, I propose to add `String[] CompilerToVM.getSignaturePolymorphicHolderNames()`:

http://cr.openjdk.java.net/~dnsimon/8164214v2

-Doug

> diff -r 881b3acaed84 src/share/vm/oops/method.cpp
> --- a/src/share/vm/oops/method.cpp	Thu Aug 18 21:33:36 2016 +0000
> +++ b/src/share/vm/oops/method.cpp	Fri Aug 19 17:25:22 2016 -0700
> @@ -85,6 +85,7 @@
>   set_constMethod(xconst);
>   set_access_flags(access_flags);
>   set_intrinsic_id(vmIntrinsics::_none);
> +  set_signature_polymorphic(false);
>   set_jfr_towrite(false);
>   set_force_inline(false);
>   set_hidden(false);
> @@ -1451,6 +1452,8 @@
>     id = MethodHandles::signature_polymorphic_name_id(method_holder(), name());
>     if (is_static() != MethodHandles::is_signature_polymorphic_static(id))
>       id = vmIntrinsics::_none;
> +    else
> +      set_signature_polymorphic(true);
>     break;
>   }
> 
> diff -r 881b3acaed84 src/share/vm/oops/method.hpp
> --- a/src/share/vm/oops/method.hpp	Thu Aug 18 21:33:36 2016 +0000
> +++ b/src/share/vm/oops/method.hpp	Fri Aug 19 17:25:22 2016 -0700
> @@ -82,7 +82,8 @@
>     _has_injected_profile  = 1 << 5,
>     _running_emcp          = 1 << 6,
>     _intrinsic_candidate   = 1 << 7,
> -    _reserved_stack_access = 1 << 8
> +    _reserved_stack_access = 1 << 8,
> +    _signature_polymorphic = 1 << 9
>   };
>   mutable u2 _flags;
> 
> @@ -867,6 +868,13 @@
>     _flags = x ? (_flags | _reserved_stack_access) : (_flags & ~_reserved_stack_access);
>   }
> 
> +  bool signature_polymorphic() {
> +    return (_flags & _signature_polymorphic) != 0;
> +  }
> +  void set_signature_polymorphic(bool x) {
> +    _flags = x ? (_flags | _signature_polymorphic) : (_flags & ~_signature_polymorphic);
> +  }
> +
>   ConstMethod::MethodType method_type() const {
>       return _constMethod->method_type();
>   }
> 
>> On 19 Aug 2016, at 17:05, John Rose <john.r.rose at oracle.com> wrote:
>> 
>> On Aug 19, 2016, at 2:32 PM, Doug Simon <doug.simon at oracle.com> wrote:
>>> 
>>>> 
>>>> On 19 Aug 2016, at 20:14, John Rose <john.r.rose at oracle.com> wrote:
>>>> 
>>>> The main attribute-fetching hook for methods is HotSpotResolvedJavaMethodImpl / CompilerToVM::get_jvmci_method, which does as much as possible lazily; the lazy logic tries to use Unsafe peek/poke methods on the metaspace method instead of expensive transitions into the JVM.
>>> 
>>> Most of the laziness is to make HSRJMI objects as light as possible. They have a reference to a Method* and query it with Unsafe (as you observe).
>> 
>> Yes.  It's good pattern:  lazy, minimal state and setup, no extra copies of stuff.
>> 
>> (It's not absolute, though, since the HotSpotResolvedJavaMethodImpl has a few key eager initializations.  The HotSpot's own internal CI makes similar choices.)
>> 
>>>>>> The move of the "magic names" MethodHandle and VarHandle down into the hotspot-specific CI code is good, but it would be best to pull the sig-poly bit straight from the VM.
>>>>> 
>>>>> It’s a little tricky since we cannot deal with Symbol* values directly in the Java part of JVMCI and all VM based sig-poly tests are based on such values. One thing we could do is ask the VM for the set of sig-poly holders. However, I don’t see this centralized anywhere in the VM currently.
>>>> 
>>>> I think a reasonable high road to take would be to model the sig-poly query on the caller-sensitive query.  That means putting a new flag in Method::_flags.  I support this, if you wish to make that cut.  The class file parser would have to (a) detect when a sig-poly-bearing class is being loaded (this is a cheap tax), and then (b) more carefully sift the methods and mark the sig-poly ones.
>>> 
>>> That would definitely be the best option from my perspective. Fast, cheap and shifts all the sig-poly logic to the VM. Assuming I can exercise this option, how best to proceed? The changes to Method and the class file parser should obviously be done in a seperate RFE. Is that something you or someone in your team could undertake? I’d like to integrate 8164214 without waiting for the Method::_signature_polymorphic bit to be available since it’s blocking a few other issues and efforts.
>> 
>> In principle, yes,  But let's see if we can use the other option, the intrinsic_id.
>> 
>>> 
>>>> 
>>>> Another reasonable high road would be to choose a special value to store in the Method::_intrinsic_id field for sig-poly methods that don't already have their own special intrinsic_id.  Again, the value would be set up (cheaply) at class load time, and the JVMCI could probe for that.
>>> 
>>> I’m not sure I fully understand this proposal. Are you saying it might be possible to fully encode the sig-poly bit(s) into intrinsic_id (as opposed to just using intrinsic_id as a guard for a VM call)? And would this encoding be guaranteed never to change? If not, then it could require a change on the Java side at which point I think we’re back to being no better off than the sig-poly holder test as the guard for a VM call.
>> 
>> There is a range of intrinsic ID's which apply only to s-p methods.  And every s-p method has such an IID.
>> Thus, I think you can load the IID (in Java) and range-check it.  You might need a couple of new config parameters,
>> for the bounds of the range.  That's something you can do on the side without a new RFE, right?
>> Actually, good-enough constants are already there:  Config.vmIntrinsicInvokeBasic, etc.
>> See the comment here:
>>  http://hg.openjdk.java.net/jdk9/jdk9/hotspot/file/a4faaf753e03/src/share/vm/classfile/vmSymbols.hpp#l1396
>> 
>> I don't know why that wouldn't work…
>> 
>>>> Paul, does any of this sound reasonable?
>>>> 
>>>> The sig-poly condition, like the caller-sensitive condition, is very rare, but probably needs to be queried on many methods.  Therefore there is a tradeoff between compact representation (ideally a fraction of a bit, as in intrinsic_id) and fast access.
>>> 
>>> Yes, a cheap test of a bit (not sure I understand what a fraction of a bit looks like!) would be ideal. Seems like a justifiable use of one of the 7 spare bits in Method::_flags.
>> 
>> Since s-p methods are disjoint from other intrinsics, it follows that both kinds of methods can share the IID coding space.  The relatively small fraction of IID codes used by s-p methods means that the other methods get about 7.9 bits, and the s-p methods use the remaining 0.1 bits of the 8-bit IID field.  :-)
>> 
>> — John
>> 
>