RFR: 8164214: [JVMCI] include VarHandle in signature polymorphic method test
Doug Simon
doug.simon at oracle.com
Mon Aug 22 20:37:00 UTC 2016
> On 22 Aug 2016, at 19:38, Christian Thalinger <cthalinger at twitter.com> wrote:
>
>>
>> On Aug 19, 2016, at 9:59 PM, 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.
>>
>> Based on this (late - sorry!) realization, I propose to add `String[] CompilerToVM.getSignaturePolymorphicHolderNames()`:
>>
>> http://cr.openjdk.java.net/~dnsimon/8164214v2
>
> I don’t see why this is any better in terms of “duplicating” logic:
It’s only slightly better in that it’s now in C++ VM code. I would hope that someone updating the well known sig-poly holder class set would grep the VM sources for VarHandle but I agree that it’s an unrealistic expectation. The real solution as you say is to contain it all in methodHandles.[cpp|hpp]. I’m happy to give that a shot but I’m not sure it should be done as part of this change. Paul, what do you think?
-Doug
> +C2V_VMENTRY(jobject, getSignaturePolymorphicHolders, (JNIEnv*, jobject))
> + objArrayHandle holders = oopFactory::new_objArray(SystemDictionary::String_klass(), 2, CHECK_NULL);
> + Handle mh = java_lang_String::create_from_str("Ljava/lang/invoke/MethodHandle;", CHECK_NULL);
> + Handle vh = java_lang_String::create_from_str("Ljava/lang/invoke/VarHandle;", CHECK_NULL);
> The names are hard-coded again, just in a different place.
>
> What we really want is to refactor all occurrences of this pattern:
>
> static bool has_member_arg(Symbol* klass, Symbol* name) {
> if ((klass == vmSymbols::java_lang_invoke_MethodHandle() ||
> klass == vmSymbols::java_lang_invoke_VarHandle()) &&
>
> 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())) {
>
> bool MethodHandles::is_method_handle_invoke_name(Klass* klass, Symbol* name) {
> if (klass == NULL)
> return false;
> // The following test will fail spuriously during bootstrap of MethodHandle itself:
> // if (klass != SystemDictionary::MethodHandle_klass())
> // Test the name instead:
> if (klass->name() != vmSymbols::java_lang_invoke_MethodHandle() &&
> klass->name() != vmSymbols::java_lang_invoke_VarHandle()) {
>
> (I think it’s only these three) into a method so we have that query in one place. Then add a method that returns the names of these guys right next to it and call it. It would be ideal if one would somehow use the other but I don’t see how. Maybe both should use a table.
>
> Nobody will update a totally unrelated method in JVMCI if the class list changes…
>
>>
>> -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
More information about the hotspot-compiler-dev
mailing list