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

[Doug Simon]

> On 19 Aug 2016, at 20:14, John Rose <john.r.rose at oracle.com> wrote:
> 
> On Aug 17, 2016, at 1:46 PM, Doug Simon <doug.simon at oracle.com> wrote:
>> 
>>> 
>>> On 17 Aug 2016, at 22:13, John Rose <john.r.rose at oracle.com> wrote:
>>> 
>>> On Aug 17, 2016, at 1:00 PM, Paul Sandoz <paul.sandoz at oracle.com> wrote:
>>>> 
>>>> 
>>>>> On 17 Aug 2016, at 06:08, Doug Simon <doug.simon at oracle.com> wrote:
>>>>> 
>>>>> Please review this change that should have been part of 8163962. The problem is that java.lang.invoke.VarHandle needs to be treated the same as java.lang.invoke.MethodHandle when resolving invoke bytecodes, especially when the resolution should be eager (e.g., as may be required for ahead of time compilation).
>>>>> 
>>>>> I removed ResolvedJavaMethod.isSignaturePolymorphic(...) as well since a) it was broken and b) we should rely as much as possible on VM code to do the test for whether a method is signature polymorphic to be more future proof.
>>> 
>>> Ideally, the CI should call MethodHandles::is_signature_polymorphic_name in the VM, as resolveInvokeHandleInPool does, or at least assert an equivalence with that call.  Is there a pre-existing attribute-fetching routine for methods that could deliver the extra bit?
>> 
>> I’m a little confused - what CI code are you referring to? In my mind, resolveInvokeHandleInPool is part of JVMCI. Also, I’m not aware of any attribute bit denoting a signature polymorphic method.
> 
> I'll tell you the way it looks to me, with a disclaimer that you (and Igor/Vladimir/Chris) know far more about how the JVMCI works.
> 
> 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).

> 
> Meanwhile, the checks for sig-poly in the JVM are centralized in MethodHandles::is_method_handle_invoke_name and various related inline methods (in methodHandles.hpp).  The tricky dance is more complicated than just fetching a mode bit.  (But perhaps we should add the mode bit.)
> 
> You don't want to make a JVM call for a large number of ResolvedJavaMethods.  So you are looking to replicate the sig-poly logic somewhere in Java.
> 
> But that perpetuates the code duplication (moves it to a lower level in the webrev, IIUC).  It is perhaps defensible because the CI code is tightly coupled to HotSpot, and/or JVM spec talks about sig-poly methods.  But it gets fragile as soon as we are move the spec (as we are in 9 and will probably do more).  And our private APIs between JVM and JDK might have off-label sig-poly methods and classes, not mandated by the JVM spec.  So code duplication = fragile even in this case.

> I see three ways to kill the code duplication:  0. Put in a query method that punches into the JVM (too slow), 1. Have an into-the-JVM query method, but gate it on an easy-to-determine precondition rarely true (intrinsic_id comes to mind, but that doesn't work for VarHandles), 2. Pull a flag eagerly from the JVM when creating a HSRJM, on the assumption that you are already visiting the JVM when you are creating a HSRJM that might be sig-poly (which may be false?).

As of the webrev, the only Java side duplication of the sig-poly check is for a method’s holder being in the sig-poly holders set (i.e., currently composed of MH and VH). I’m not sure this is any worse than using intrinsic_id. Are sig-poly methods guaranteed to have a non-zero intrinsic_id, now and forever more? The holder check could be made more robust if the set of sig-poly holders could be initialized from the VM (I just couldn’t find the place to query).

> 
> I see there are no eagerly-pulled flags in HSRJMI, which makes me doubt option 2.
> 
>> 
>>> 
>>> 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. 

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

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

> 
> Finally, I'm not against the lower-road approach, of moving the duplicated query logic down-stack, but I want us to consider the best option.

+1

-Doug