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

[coleen.phillimore at oracle.com]

On 5/26/17 4:48 PM, John Rose wrote:
> On May 26, 2017, at 10:47 AM, coleen.phillimore at oracle.com 
> <mailto: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 athttp://cr.openjdk.java.net/~coleenp/8174749.04/webrev 
>> <http://cr.openjdk.java.net/%7Ecoleenp/8174749.04/webrev>
>> open webrev 
>> athttp://cr.openjdk.java.net/~coleenp/8174749.jdk.04/webrev 
>> <http://cr.openjdk.java.net/%7Ecoleenp/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.
Hi John,

I was just thinking of this change below, it makes sense to treat field 
and method MemberName differently as you have below.  The field needs 
the clazz present to be expanded but method MemberName does not.

Yes, this makes sense.
>
> (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.

JavaClasses is in the loop because it knows which fields to assign and 
how to create a ResolvedMethodName.  I think this makes sense to isolate 
it like this an appreciated only changing javaClasses.cpp when I kept 
changing the names of the fields.

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

Yes, it has to be later.  I'm going to file a couple of RFE's after this 
that we discussed so that RMN can be used instead of MN.  And believe it 
or not, large changes make me anxious.  :)
>
> 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:

Done.   Passes java/lang/invoke tests (as sanity).

http://cr.openjdk.java.net/~coleenp/8174749.05/webrev

(wish I could do incremental webrevs because full webrevs take forever).

Thank you for all your help and comments.

Coleen


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