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

[Doug Simon]

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

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

> This change is an obvious improvement, so I'm not objecting to it!
> 
> BTW, I noticed the deleted unit test isSignaturePolymorphicTest.  Is there an equivalent somewhere else that covers that ground?  Specifically, that tests MH.invoke, MH.type, the linkers, a random method, etc.  Would be good to have immediate test failure if a wire breaks there.

That test was for ResolvedJavaMethod.isSignaturePolymorphic which is also removed by this change. Once agin, this is to push the as much of the sig-poly test back into the VM.

>>> Note that in HotSpotConstantPool.java I also converted sanity check methods of the form `void assert<Condition>(...)` to `boolean check<Condition>(...)` so that the call itself is only made when assertions are enabled. Some of these checks make VM calls which can slow down constant pool resolution and thus bytecode parsing. I can refactor this change into a separate bug if anyone insists.
> 
> This is a valid on-the-fly cleanup, IMO.  Some readers might find a constant "true" return odd from a check method, but in reality that is the best way to compose assertion logic across subroutines.  The HotSpot code does this a lot, too.

Yes, I’d noticed. Thanks for accepting this piggy-backing ;-)

-Doug