RFR: 8164214: [JVMCI] include VarHandle in signature polymorphic method test
Christian Thalinger
cthalinger at twitter.com
Mon Aug 22 17:38:17 UTC 2016
> 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 <http://cr.openjdk.java.net/~dnsimon/8164214v2>
I don’t see why this is any better in terms of “duplicating” logic:
+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
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20160822/58399460/attachment-0001.html>
More information about the hotspot-compiler-dev
mailing list