RFR 8152271: MemberNameTable doesn't purge stale entries

[Vladimir Ivanov]

On 6/15/16 3:03 PM, Coleen Phillimore wrote:
>
>
> On 6/15/16 7:06 AM, Vladimir Ivanov wrote:
>> Coleen,
>>
>> Latest version looks good. Thanks for adding clarifying comment in the
>> code.
>>
>>>>> Summary: Intern MemberNames in table instead of allocating new entries
>>>>>
>>>>> For degenerate case, we were leaking native code in the member name
>>>>> table.  Going with the suggested workaround, it was only a few more
>>>>> lines of code to intern MemberName and return the MemberName that was
>>>>> already in the table.
>>>>
>>>> I guess I'm very surprised that these were not already being "cached"
>>>> at some level (ie the Java level where all other reflection-like
>>>> objects get cached)! I'm not familiar with the mechanics or j.l.invoke
>>>> so was wondering where MemberName instances actually get created -
>>>> because I'd like to understand how you get two distinct MemberName
>>>> instances that compare equal in the first place?
>>>
>>> Hi David,
>>>
>>> The MemberNames were not being cached. There was a change to attempt to
>>> cache them in the jdk https://bugs.openjdk.java.net/browse/JDK-8013267
>>> but it was a much bigger change and we still need access to MemberName
>>> in the jvm because we have to replace the Method* in them with
>>> redefinition.
>>>
>>> MemberNames get created mostly when you create a MethodHandle. The
>>> MemberName is the jvm representation of the member of the class. It's
>>> not simply a Method* because it contains flags based on how it's
>>> resolved at linktime.
>>>
>>> In the test case provided,
>>>       MethodHandle mh = lookup.findSpecial(Leak.class, "callMe", mt,
>>> Leak.class);
>>>
>>> this looks up the same method and creates the same MemberName over and
>>> over.
>>
>> j.l.i.MemberName implements Cloneable and MN cloning is pervasively
>> used during DirectMethodHandle construction (see MN.resolve() and
>> other MN instance methods).
>
> So there'd still be a leak but just not a simple one.   Are there plans
> to not use cloning?  That should be a separate issue, though. The VM
> would now reuse MemberName with this change.

No, I'm not aware of any plans to get rid of MN cloning.

The following code takes care of cloned MN instance:

--- old/src/share/vm/prims/jvm.cpp	2016-06-09 17:19:13.896240434 -0400
+++ new/src/share/vm/prims/jvm.cpp	2016-06-09 17:19:13.454960882 -0400
@@ -695,7 +695,7 @@
        // This can safepoint and redefine method, so need both new_obj 
and method
        // in a handle, for two different reasons.  new_obj can move, 
method can be
        // deleted if nothing is using it on the stack.
-      m->method_holder()->add_member_name(new_obj());
+      m->method_holder()->add_member_name(new_obj(), false);
      }
    }

The cloned instance is usually adjusted (changes in MN.flags) right 
away, so it becomes non-equal to the original one. It allows to bypass 
repeated resolution.

When you talk about a leak, do you mean the situation when there's 
already a duplicate of adjusted MN registered ?

If it causes a leak, we can do a call into the JVM to intern adjusted MN 
instance, e.g:

jdk/src/java.base/share/classes/java/lang/invoke/MemberName.java:

     private MemberName changeReferenceKind(byte refKind, byte oldKind) {
         assert(getReferenceKind() == oldKind);
         assert(MethodHandleNatives.refKindIsValid(refKind));
         flags += (((int)refKind - oldKind) << MN_REFERENCE_KIND_SHIFT);
	return MethodHandleNatives.intern(this); // returns interned MN instance
     }

Best regards,
Vladimir Ivanov

>
> Thanks,
> Coleen
>
>>
>> Best regards,
>> Vladimir Ivanov
>>
>>>>
>>>> Otherwise changes seem fine - though perhaps
>>>> MemberNameTable::add_member_name should assert the member_name is not
>>>> already present, just to confirm those intern==false cases are working
>>>> as intended.
>>>
>>> I want to keep add_member_name simple and not go through the list even
>>> under ASSERT.  It's okay if some member_name are already present, and
>>> for the case with JVM_Clone, we want an additional MemberName in the
>>> table (and that uses add_member_name()).
>>>
>>> Thanks for looking at the code.
>>> Coleen
>>>
>>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>>> This has been performance tested to show no regression and works
>>>>> really
>>>>> well for the degenerate test case, even though the real percentage of
>>>>> reused MemberName seems quite small.
>>>>>
>>>>> Tested with all runtime nightly tests, tests in
>>>>> jdk/test/java/lang/invoke.
>>>>>
>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8152271.01/webrev
>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8152271
>>>>>
>>>>> Thanks,
>>>>> Coleen
>>>
>