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

[serguei.spitsyn at oracle.com]

Hi Coleen,

It looks good to me.
At least, I do not see anything bad in the last update.

Thanks,
Serguei


On 5/26/17 14:48, coleen.phillimore at oracle.com wrote:
>
>
> 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 @@
>>
>