RFR 8152271: MemberNameTable doesn't purge stale entries

[Coleen Phillimore]

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.

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
>>