RFR (L) 8174749: Use hash table/oops for MemberName table

[coleen.phillimore at oracle.com]

Hi, I made the changes below, which turned out very nice.  It didn't 
take that long to retest.  See:

open webrev at http://cr.openjdk.java.net/~coleenp/8174749.04/webrev
open webrev at http://cr.openjdk.java.net/~coleenp/8174749.jdk.04/webrev

I don't know how to do delta webrevs, so just look at 
linkResolver.cpp/hpp and methodHandles.cpp

Thanks,
Coleen

On 5/26/17 9:37 AM, coleen.phillimore at oracle.com wrote:
>
> Hi John,
> Thank you for these comments and for your help with this bug fix/RFE.
>
> On 5/25/17 8:04 PM, John Rose wrote:
>> On May 17, 2017, at 9:01 AM, coleen.phillimore at oracle.com 
>> <mailto:coleen.phillimore at oracle.com> wrote:
>>>
>>> Summary: Add a Java type called ResolvedMethodName which is 
>>> immutable and can be stored in a hashtable, that is weakly collected 
>>> by gc
>>
>> I'm looking at the 8174749.03/webrev version of your changes.
>>
>> A few comments:
>>
>> In the JVMCI changes, this line appears to be incorrect on 32-bit 
>> machines:
>>
>> +                vmtargetField = (HotSpotResolvedJavaField) 
>> findFieldInClass(methodType, "vmtarget", resolveType(long.class));
>>
>> (It's a pre-existing condition, and I'm not sure if it is a problem.)
>
> I'll add a comment. I don't know if Graal supports 32 bits (I'd guess 
> no).
>>
>> In the new hash table file, the parameter names
>> seem like they could be made more consistent.
>>
>>   85 oop ResolvedMethodTable::basic_add(Method* method, oop vmtarget) {
>>
>>  114 oop ResolvedMethodTable::add_method(Handle mem_name_target) {
>>
>> I think vmtarget and mem_name_target are the same kind of thing.
>> Consider renaming them to "entry_to_add" or something more aligned
>> with the rest of the code.
>
> This code didn't get updated properly from all the renaming.  I've 
> changed to using "method" when the parameter is Method* and using 
> rmethod_name when the parameter is an oop representing 
> ResolvedMethodName.
>
>>
>> I don't think that MethodHandles::init_field_MemberName needs TRAPS.
>
> Yes, removed TRAPS and reverted the use of the default parameter.
>> Also, MethodHandles::init_method_MemberName could omit TRAPS if
>> it were passed the RMN pointer first.  Suggestion:  Remove TRAPS from
>> both *and* add a trapping function which does the info->RMN step.
>>
>>   static oop init_method_MemberName(Handle mname_h, CallInfo& info, 
>> oop resolved_method);
>>   static oop init_method_MemberName(Handle mname_h, CallInfo& info, 
>> TRAPS);
>>
>> Then the trapping overloading can pick up the RMN immediately from 
>> the info,
>> and call the non-trapping overloading.  The reason to do something 
>> indirect
>> like this is that the existing code for init_method_MemberName is (a) 
>> complex
>> and (b) non-trapping.  Promoting it all to trapping makes it harder 
>> to work with.
>>
>> In other words, non-TRAPS code is (IMO) easier to read and reason about,
>> so converting a big method to TRAPS for one line is something I'd 
>> like to
>> avoid.  At least, that's the way I thought about this particular code 
>> when
>> I first wrote it.
>>
>> Better:  Since init_m_MN is joined at the hip with CallInfo, consider 
>> adding the
>> trapping operation to CallInfo.  See patch below.  I think that 
>> preserves CI's
>> claim to be the Source of Truth for call sites, even in 
>> methodHandles.cpp.
>>
>
> This is quite a nice change!   I'll do this and rerun the tests over 
> the weekend and send out a new version next week.
>
>> Thank you very much for this fix.  I know it's been years since we 
>> started
>> talking about it.  I'm glad you let it bother you enough to fix it!
>
> We kept running into this, so it was time.
>
>>
>> I looked at everything else and didn't find anything out of place.
>
> Thank you!
> Coleen
>
>>
>> Reviewed.
>>
>> — John
>>
>> diff --git a/src/share/vm/interpreter/linkResolver.hpp 
>> b/src/share/vm/interpreter/linkResolver.hpp
>> --- a/src/share/vm/interpreter/linkResolver.hpp
>> +++ b/src/share/vm/interpreter/linkResolver.hpp
>> @@ -56,6 +56,7 @@
>>    int          _call_index;             // vtable or itable index of 
>> selected class method (if any)
>>    Handle       _resolved_appendix;      // extra argument in 
>> constant pool (if CPCE::has_appendix)
>>    Handle       _resolved_method_type;   // MethodType (for 
>> invokedynamic and invokehandle call sites)
>> +  Handle       _resolved_method_name;   // optional 
>> ResolvedMethodName object for java.lang.invoke
>>    void set_static(KlassHandle resolved_klass, const methodHandle& 
>> resolved_method, TRAPS);
>>    void set_interface(KlassHandle resolved_klass, KlassHandle 
>> selected_klass,
>> @@ -97,6 +98,7 @@
>>    methodHandle selected_method() const       { return 
>> _selected_method; }
>>    Handle       resolved_appendix() const       { return 
>> _resolved_appendix; }
>>    Handle       resolved_method_type() const      { return 
>> _resolved_method_type; }
>> +  Handle       resolved_method_name() const      { return 
>> _resolved_method_name; }
>>    BasicType    result_type() const       { return 
>> selected_method()->result_type(); }
>>    CallKind     call_kind() const       { return _call_kind; }
>> @@ -117,6 +119,12 @@
>>      return _call_index;
>>    }
>> +  oop find_resolved_method_name(TRAPS) {
>> +    if (_resolved_method_name.is_null())
>> + 
>>  java_lang_invoke_ResolvedMethodName::find_resolved_method(_resolved_method, 
>> CHECK_NULL);
>> +    return _resolved_method_name;
>> +  }
>> +
>>    // debugging
>>  #ifdef ASSERT
>>    bool         has_vtable_index() const      { return _call_index >= 
>> 0 && _call_kind != CallInfo::itable_call; }
>> diff --git a/src/share/vm/interpreter/linkResolver.hpp 
>> b/src/share/vm/interpreter/linkResolver.hpp
>> --- a/src/share/vm/interpreter/linkResolver.hpp
>> +++ b/src/share/vm/interpreter/linkResolver.hpp
>> @@ -56,6 +56,7 @@
>>    int          _call_index;             // vtable or itable index of 
>> selected class method (if any)
>>    Handle       _resolved_appendix;      // extra argument in 
>> constant pool (if CPCE::has_appendix)
>>    Handle       _resolved_method_type;   // MethodType (for 
>> invokedynamic and invokehandle call sites)
>> +  Handle       _resolved_method_name;   // optional 
>> ResolvedMethodName object for java.lang.invoke
>>    void set_static(KlassHandle resolved_klass, const methodHandle& 
>> resolved_method, TRAPS);
>>    void set_interface(KlassHandle resolved_klass, KlassHandle 
>> selected_klass,
>> @@ -97,6 +98,7 @@
>>    methodHandle selected_method() const       { return 
>> _selected_method; }
>>    Handle       resolved_appendix() const       { return 
>> _resolved_appendix; }
>>    Handle       resolved_method_type() const      { return 
>> _resolved_method_type; }
>> +  Handle       resolved_method_name() const      { return 
>> _resolved_method_name; }
>>    BasicType    result_type() const       { return 
>> selected_method()->result_type(); }
>>    CallKind     call_kind() const       { return _call_kind; }
>> @@ -117,6 +119,12 @@
>>      return _call_index;
>>    }
>> +  oop find_resolved_method_name(TRAPS) {
>> +    if (_resolved_method_name.is_null())
>> + 
>>  java_lang_invoke_ResolvedMethodName::find_resolved_method(_resolved_method, 
>> CHECK_NULL);
>> +    return _resolved_method_name;
>> +  }
>> +
>>    // debugging
>>  #ifdef ASSERT
>>    bool         has_vtable_index() const      { return _call_index >= 
>> 0 && _call_kind != CallInfo::itable_call; }
>>
>>
>>
>