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

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Thu May 18 12:52:33 UTC 2017

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.iResolvedMethodName) 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 findmethod(Method* vmtarget); 87 static oop addmethod(Handle memnametarget); 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::expandMemberName(Handle mname, int suppress, TRAPS) { assert(javalanginvokeMemberName::isinstance(mname()), ""); - Metadata* vmtarget = javalanginvokeMemberName::vmtarget(mname()); int vmindex = javalanginvokeMemberName::vmindex(mname()); - if (vmtarget == NULL) { + bool havedefc = (javalanginvokeMemberName::clazz(mname()) != NULL); + + assert(havedefc, "defining class should be present"); + + if (!havedefc) { THROWMSG(vmSymbols::javalangIllegalArgumentException(), "nothing to expand"); } - bool havedefc = (javalanginvokeMemberName::clazz(mname()) != NULL); bool havename = (javalanginvokeMemberName::name(mname()) != NULL); bool havetype = (javalanginvokeMemberName::type(mname()) != NULL); int flags = javalanginvokeMemberName::flags(mname()); if (suppress != 0) { - if (suppress & suppressdefc) havedefc = true; if (suppress & suppressname) havename = true; if (suppress & suppresstype) havetype = true; } It seems, the assert and THROWMSG duplicate each other. Also, the suppressdefc 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 + anyclasshasresolvedmethods |= theclass->hasresolvedmethods(); 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

More information about the hotspot-dev mailing list