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

[coleen.phillimore at oracle.com]

Hi Serguei,   Thank you for reviewing this.  I should have called you 
out to do it :)

On 5/18/17 4:10 AM, serguei.spitsyn at oracle.com wrote:
> Hi Coleen,
>
>
> Nice refactoring!

Thank you!
>
> Some quick comments.
>
>
> http://oklahoma.us.oracle.com/~cphillim/webrev/8174749.01/webrev/src/share/vm/classfile/javaClasses.hpp.udiff.html
>
>   The function name ResolvedMethodName (and the 
> j.l.i_ResolvedMethodName) sounds confusing.
>   Would it better name it ResolvedMethod (by its meaning it is 
> MemberNameResolvedMethod)?

John and I liked that name.  When I called it ResolvedMethod, in the vm 
code it didn't look like an oop.   Also, we're envisioning it to replace 
MemberName in some places like LambdaForms and StackWalk frames, so I 
wanted to keep the Name in the name.
>
>
> http://oklahoma.us.oracle.com/~cphillim/webrev/8174749.01/webrev/src/share/vm/prims/resolvedMethodTable.hpp.html
>    85   // Called from MethodHandles
>    86   static oop find_method(Method* vmtarget);
>    87   static oop add_method(Handle mem_name_target);
>
>   Do you mean, called from the MethodHandles.cpp ?
>   I do not see these functions are ever called from the MethodHandles.cpp.
>   But they are called from the javaClasses.cpp.

Thanks for noticing that.  I'd moved the code.   Updated the comment like:

   // Called from java_lang_invoke_ResolvedMethodName
   static oop find_method(Method* vmtarget);
   static oop add_method(Handle mem_name_target);

>
>
> http://oklahoma.us.oracle.com/~cphillim/webrev/8174749.01/webrev/src/share/vm/prims/methodHandles.cpp.udiff.html
>   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) {
> + bool have_defc = (java_lang_invoke_MemberName::clazz(mname()) != NULL);
> +
> + assert(have_defc, "defining class should be present");
> +
> + if (!have_defc) {
>       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);
>     bool have_type = (java_lang_invoke_MemberName::type(mname()) != NULL);
>     int flags      = java_lang_invoke_MemberName::flags(mname());
>   
>     if (suppress != 0) {
> - if (suppress & _suppress_defc) have_defc = true;
>       if (suppress & _suppress_name)  have_name = true;
>       if (suppress & _suppress_type)  have_type = true;
>     }
>
>    It seems, the assert and THROW_MSG duplicate each other.
>    Also, the _suppress_defc is not used anymore.
>    Should it be removed from the enum in the methodHandles.hpp,
>    or there is a plan to use it later?

There is no plan, I missed that flag.   I had the assert to verify that 
the clazz is always initialized properly when the MemberName needs 
expansion.
>
>
> http://oklahoma.us.oracle.com/~cphillim/webrev/8174749.01/webrev/src/share/vm/prims/jvmtiRedefineClasses.cpp.udiff.html
>
> + _any_class_has_resolved_methods |= the_class->has_resolved_methods();
>   Should we use '||=' instead of '|=' ?


Yes, fixed.

Thanks,
Coleen
>
>
> Thanks,
> Serguei
>
>
>
>
>
> On 5/17/17 09:01, 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
>>
>> Thanks to John for his help with MemberName, and to-be-filed RFEs for 
>> further improvements.  Thanks to Stefan for GC help.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8174749.01/webrev
>> open webrev at http://cr.openjdk.java.net/~coleenp/8174749.jdk.01/webrev
>> bug link https://bugs.openjdk.java.net/browse/JDK-8174749
>>
>> Tested with RBT nightly, compiler/jsr292 tests (included in rbt 
>> nightly), JPRT, jdk/test/java/lang/invoke, 
>> jdk/test/java/lang/instrument tests.
>>
>> There are platform dependent changes in this change. They are very 
>> straightforward, ie. add an indirection to MemberName invocations, 
>> but could people with access to these platforms test this out for me?
>>
>> Performance testing showed no regression, and large 1000% improvement 
>> for the cases that caused us to backout previous attempts at this 
>> change.
>>
>> Thanks,
>> Coleen
>>
>