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

John Rose john.r.rose at oracle.com
Fri May 26 20:48:10 UTC 2017

• Previous message: RFR (L) 8174749: Use hash table/oops for MemberName table
• Next message: RFR (L) 8174749: Use hash table/oops for MemberName table
• Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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 @@

• Previous message: RFR (L) 8174749: Use hash table/oops for MemberName table
• Next message: RFR (L) 8174749: Use hash table/oops for MemberName table
• Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

More information about the hotspot-dev mailing list