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

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Wed May 31 00:21:18 UTC 2017


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 "havedefc" 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 javalanginvokeResolvedMethodName::findresolvedmethod 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 MHNgetMemberVMInfo, 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 "havedefc" 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::expandMemberName(Handle mname, int suppress, TRAPS) { assert(javalanginvokeMemberName::isinstance(mname()), ""); - Metadata* vmtarget = javalanginvokeMemberName::vmtarget(mname()); - int vmindex = javalanginvokeMemberName::vmindex(mname()); - if (vmtarget == NULL) { - THROWMSG(vmSymbols::javalangIllegalArgumentException(), "nothing to expand"); - } bool havedefc = (javalanginvokeMemberName::clazz(mname()) != NULL); bool havename = (javalanginvokeMemberName::name(mname()) != NULL); @@ -817,10 +812,14 @@ case ISMETHOD: case ISCONSTRUCTOR: { + Method* vmtarget = javalanginvokeMemberName::vmtarget(method()); + if (vmtarget == NULL) { + THROWMSG(vmSymbols::javalangIllegalArgumentException(), "nothing to expand"); + } assert(vmtarget->ismethod(), "method or constructor vmtarget is Method*"); methodHandle m(THREAD, (Method*)vmtarget); DEBUGONLY(vmtarget = NULL); // safety - if (m.isnull()) break; + assert(m.notnull(), "checked above"); if (!havedefc) { InstanceKlass* defc = m->methodholder(); javalanginvokeMemberName::setclazz(mname(), defc->javamirror()); @@ -838,17 +837,16 @@ } case ISFIELD: { - assert(vmtarget->isklass(), "field vmtarget is Klass*"); - if (!((Klass*) vmtarget)->isinstanceklass()) break; - instanceKlassHandle defc(THREAD, (Klass*) vmtarget); - DEBUGONLY(vmtarget = NULL); // safety + oop clazz = javalanginvokeMemberName::clazz(mname()); + if (clazz == NULL) { + THROWMSG(vmSymbols::javalangIllegalArgumentException(), "nothing to expand (as field)"); + } + InstanceKlass* defc = InstanceKlass::cast(javalangClass::asKlass(clazz)); + DEBUGONLY(clazz = NULL); // safety bool isstatic = ((flags & JVMACCSTATIC) != 0); fieldDescriptor fd; // findfield initializes fd if found if (!defc->findfieldfromoffset(vmindex, isstatic, &fd)) break; // cannot expand - if (!havedefc) { - javalanginvokeMemberName::setclazz(mname(), defc->javamirror()); - } if (!havename) { //not javalangString::createfromsymbol; let's intern member names Handle name = StringTable::intern(fd.name(), CHECK); @@ -1389,6 +1387,39 @@



More information about the hotspot-dev mailing list