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

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Fri May 26 13:37:04 UTC 2017

Hi John, Thank you for these comments and for your help with this bug fix/RFE.

On 5/25/17 8:04 PM, John Rose wrote:

On May 17, 2017, at 9:01 AM, coleen.phillimore at oracle.com <mailto: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 I'm looking at the 8174749.03/webrev version of your changes. A few comments: In the JVMCI changes, this line appears to be incorrect on 32-bit machines: + vmtargetField = (HotSpotResolvedJavaField) findFieldInClass(methodType, "vmtarget", resolveType(long.class)); (It's a pre-existing condition, and I'm not sure if it is a problem.)

I'll add a comment. I don't know if Graal supports 32 bits (I'd guess no).

In the new hash table file, the parameter names seem like they could be made more consistent. 85 oop ResolvedMethodTable::basicadd(Method* method, oop vmtarget) { 114 oop ResolvedMethodTable::addmethod(Handle memnametarget) { I think vmtarget and memnametarget are the same kind of thing. Consider renaming them to "entrytoadd" or something more aligned with the rest of the code.

This code didn't get updated properly from all the renaming. I've changed to using "method" when the parameter is Method* and using rmethod_name when the parameter is an oop representing ResolvedMethodName.

I don't think that MethodHandles::initfieldMemberName needs TRAPS.

Yes, removed TRAPS and reverted the use of the default parameter.

Also, MethodHandles::initmethodMemberName could omit TRAPS if it were passed the RMN pointer first. Suggestion: Remove TRAPS from both and add a trapping function which does the info->RMN step.

static oop initmethodMemberName(Handle mnameh, CallInfo& info, oop resolvedmethod); static oop initmethodMemberName(Handle mnameh, CallInfo& info, TRAPS); Then the trapping overloading can pick up the RMN immediately from the info, and call the non-trapping overloading. The reason to do something indirect like this is that the existing code for initmethodMemberName is (a) complex and (b) non-trapping. Promoting it all to trapping makes it harder to work with. In other words, non-TRAPS code is (IMO) easier to read and reason about, so converting a big method to TRAPS for one line is something I'd like to avoid. At least, that's the way I thought about this particular code when I first wrote it. Better: Since initmMN is joined at the hip with CallInfo, consider adding the trapping operation to CallInfo. See patch below. I think that preserves CI's claim to be the Source of Truth for call sites, even in methodHandles.cpp.

This is quite a nice change! I'll do this and rerun the tests over the weekend and send out a new version next week.

Thank you very much for this fix. I know it's been years since we started talking about it. I'm glad you let it bother you enough to fix it!

We kept running into this, so it was time.

I looked at everything else and didn't find anything out of place.

Thank you! Coleen

Reviewed. — John diff --git a/src/share/vm/interpreter/linkResolver.hpp b/src/share/vm/interpreter/linkResolver.hpp --- a/src/share/vm/interpreter/linkResolver.hpp +++ b/src/share/vm/interpreter/linkResolver.hpp @@ -56,6 +56,7 @@ int callindex; // vtable or itable index of selected class method (if any) Handle resolvedappendix; // extra argument in constant pool (if CPCE::hasappendix) Handle resolvedmethodtype; // MethodType (for invokedynamic and invokehandle call sites) + Handle resolvedmethodname; // optional ResolvedMethodName object for java.lang.invoke void setstatic(KlassHandle resolvedklass, const methodHandle& resolvedmethod, TRAPS); void setinterface(KlassHandle resolvedklass, KlassHandle selectedklass, @@ -97,6 +98,7 @@ methodHandle selectedmethod() const { return selectedmethod; } Handle resolvedappendix() const { return resolvedappendix; } Handle resolvedmethodtype() const { return resolvedmethodtype; } + Handle resolvedmethodname() const { return resolvedmethodname; } BasicType resulttype() const { return selectedmethod()->resulttype(); } CallKind callkind() const { return callkind; } @@ -117,6 +119,12 @@ return callindex; } + oop findresolvedmethodname(TRAPS) { + if (resolvedmethodname.isnull()) + javalanginvokeResolvedMethodName::findresolvedmethod(resolvedmethod, CHECKNULL); + return resolvedmethodname; + } + // debugging #ifdef ASSERT bool hasvtableindex() const { return callindex >= 0 && callkind != CallInfo::itablecall; } diff --git a/src/share/vm/interpreter/linkResolver.hpp b/src/share/vm/interpreter/linkResolver.hpp --- a/src/share/vm/interpreter/linkResolver.hpp +++ b/src/share/vm/interpreter/linkResolver.hpp @@ -56,6 +56,7 @@ int callindex; // vtable or itable index of selected class method (if any) Handle resolvedappendix; // extra argument in constant pool (if CPCE::hasappendix) Handle resolvedmethodtype; // MethodType (for invokedynamic and invokehandle call sites) + Handle resolvedmethodname; // optional ResolvedMethodName object for java.lang.invoke void setstatic(KlassHandle resolvedklass, const methodHandle& resolvedmethod, TRAPS); void setinterface(KlassHandle resolvedklass, KlassHandle selectedklass, @@ -97,6 +98,7 @@ methodHandle selectedmethod() const { return selectedmethod; } Handle resolvedappendix() const { return resolvedappendix; } Handle resolvedmethodtype() const { return resolvedmethodtype; } + Handle resolvedmethodname() const { return resolvedmethodname; } BasicType resulttype() const { return selectedmethod()->resulttype(); } CallKind callkind() const { return callkind; } @@ -117,6 +119,12 @@ return callindex; } + oop findresolvedmethodname(TRAPS) { + if (resolvedmethodname.isnull()) + javalanginvokeResolvedMethodName::findresolvedmethod(resolvedmethod, CHECKNULL); + return resolvedmethodname; + } + // debugging #ifdef ASSERT bool hasvtableindex() const { return callindex >= 0 && callkind != CallInfo::itablecall; }

More information about the hotspot-dev mailing list