RFR 8152271: MemberNameTable doesn't purge stale entries

[Coleen Phillimore]

Hi Vladimir,

On 6/15/16 8:22 AM, Vladimir Ivanov wrote:
>
>
> 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.

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

I was but I didn't realize what the cloning was for.
>
> 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
>     }

Yes, this could be added if there's a problem observed.   This should be 
considered in the context of 
https://bugs.openjdk.java.net/browse/JDK-8013267 for later.  I'll add it 
to the bug.

Thanks,
Coleen

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