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

[John Rose]

On May 26, 2017, at 10:47 AM, coleen.phillimore at oracle.com wrote:
> 
> 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 <http://cr.openjdk.java.net/~coleenp/8174749.04/webrev>
> open webrev at http://cr.openjdk.java.net/~coleenp/8174749.jdk.04/webrev <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

Re-reviewed.

See previous message for a late-breaking comment on expand.
See below for a sketch of what I mean by keeping "have_defc" as is.

(Another reviewer commented about a dead mode bit.  The purpose of that
stuff is to allow us to tweak the JDK API.  I don't care much either way about
GC-ing unused mode bits but I do want to keep the expander capability so we
can prototype stuff in the JDK without having to edit the JVM.  So on balance,
I'd give the mode bits the benefit of the doubt.  They can be used from the JDK,
even if they aren't at the moment.)

I also like how this CallInfo change turned out.  Notice how now the function
java_lang_invoke_ResolvedMethodName::find_resolved_method has only
one usage, from the inside of CallInfo.  This feels right.  It also means you
can take javaClasses.cpp out of the loop here, and just have CallInfo call
directly into SystemDictionary and ResolvedMethodTable.  It seems just
as reasonable to me that linkResolver.cpp would do that job, than that it
would be to delegate via javaClasses.cpp.  I also think the patch will get
a little smaller if you cut javaClasses.cpp out of that loop.

Thanks,
— John

P.S. As a step after this fix, if we loosen the coupling of the JVM with MemberName,
I think we will want to get rid of MN::vmtarget and just have MN::method.
In the code of MHN_getMemberVMInfo, the unchanged line "x = mname()"
really wants to be "x = method" where method is the RMN.  The JDK code
expects a MN at that point, but it should really be the RMN now.  The only
JDK change would be in MemberName.java:

-            assert(vmtarget instanceof MemberName) : vmtarget + " in " + this;
+            assert(vmtarget instanceof ResolvedMethodName) : vmtarget + " in " + this;

I wouldn't object if you anticipated this in the present change set, but it's OK
to do it later.

P.P.S.  Here's a sketch of what I mean by walking back some of the "have_defc"
changes.  Maybe I'm missing something, but I think this version makes more
sense than the current version:

git a/src/share/vm/prims/methodHandles.cpp b/src/share/vm/prims/methodHandles.cpp
--- a/src/share/vm/prims/methodHandles.cpp
+++ b/src/share/vm/prims/methodHandles.cpp
@@ -794,11 +794,6 @@
 // which refers directly to JVM internals.
 void MethodHandles::expand_MemberName(Handle mname, int suppress, TRAPS) {
   assert(java_lang_invoke_MemberName::is_instance(mname()), "");
-  Metadata* vmtarget = java_lang_invoke_MemberName::vmtarget(mname());
-  int vmindex  = java_lang_invoke_MemberName::vmindex(mname());
-  if (vmtarget == NULL) {
-    THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(), "nothing to expand");
-  }
 
   bool have_defc = (java_lang_invoke_MemberName::clazz(mname()) != NULL);
   bool have_name = (java_lang_invoke_MemberName::name(mname()) != NULL);
@@ -817,10 +812,14 @@
   case IS_METHOD:
   case IS_CONSTRUCTOR:
     {
+      Method* vmtarget = java_lang_invoke_MemberName::vmtarget(method());
+      if (vmtarget == NULL) {
+        THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(), "nothing to expand");
+      }
       assert(vmtarget->is_method(), "method or constructor vmtarget is Method*");
       methodHandle m(THREAD, (Method*)vmtarget);
       DEBUG_ONLY(vmtarget = NULL);  // safety
-      if (m.is_null())  break;
+      assert(m.not_null(), "checked above");
       if (!have_defc) {
         InstanceKlass* defc = m->method_holder();
         java_lang_invoke_MemberName::set_clazz(mname(), defc->java_mirror());
@@ -838,17 +837,16 @@
     }
   case IS_FIELD:
     {
-      assert(vmtarget->is_klass(), "field vmtarget is Klass*");
-      if (!((Klass*) vmtarget)->is_instance_klass())  break;
-      instanceKlassHandle defc(THREAD, (Klass*) vmtarget);
-      DEBUG_ONLY(vmtarget = NULL);  // safety
+      oop clazz = java_lang_invoke_MemberName::clazz(mname());
+      if (clazz == NULL) {
+        THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(), "nothing to expand (as field)");
+      }
+      InstanceKlass* defc = InstanceKlass::cast(java_lang_Class::as_Klass(clazz));
+      DEBUG_ONLY(clazz = NULL);  // safety
       bool is_static = ((flags & JVM_ACC_STATIC) != 0);
       fieldDescriptor fd; // find_field initializes fd if found
       if (!defc->find_field_from_offset(vmindex, is_static, &fd))
         break;                  // cannot expand
-      if (!have_defc) {
-        java_lang_invoke_MemberName::set_clazz(mname(), defc->java_mirror());
-      }
       if (!have_name) {
         //not java_lang_String::create_from_symbol; let's intern member names
         Handle name = StringTable::intern(fd.name(), CHECK);
@@ -1389,6 +1387,39 @@