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

[John Rose]

On May 17, 2017, at 9:01 AM, 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.)

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.

I don't think that MethodHandles::init_field_MemberName needs TRAPS.
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.

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!

I looked at everything else and didn't find anything out of place.

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