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

[coleen.phillimore at oracle.com]

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; }
>
>
>