RFR 8209844: MemberNameLeak.java fails when ResolvedMethod entry is not removed

[coleen.phillimore at oracle.com]

On 9/5/18 7:45 PM, David Holmes wrote:
> Hi Patricio,
>
> On 6/09/2018 9:05 AM, Patricio Chilano wrote:
>> Hi David,
>>
>> On 9/4/18 9:59 PM, David Holmes wrote:
>>> On 5/09/2018 10:11 AM, Patricio Chilano wrote:
>>>> On 9/3/18 12:27 AM, David Holmes wrote:
>>>>> I'm somewhat surprised this test didn't already fail 
>>>>> intermittently as it assumes a single explicit System.gc() is 
>>>>> enough to trigger the cleanup it is checking for! If that 
>>>>> assumption is not valid then the test will now hang (in a busy 
>>>>> polling loop!) until the test harness times it out and forcibly 
>>>>> terminates it. (Previously it would have failed due to the missing 
>>>>> output immediately after the gc() call returned.)
>>>> I see that System.gc() ends up calling 
>>>> Universe::heap()->collect(GCCause::_java_lang_system_gc) which I 
>>>> understand will always reach SystemDictionary::do_unloading() 
>>>> eventually before the call returns. That would guarantee the table 
>>>> will be cleaned up before the program exits (provided the busy loop 
>>>> is there).
>>> There are tests that verify GC related actions (weak reference 
>>> processing, reference queues etc) that have to call System.gc() more 
>>> than once to actually get the expected actions to happen. It's not 
>>> easy (for me anyway) to determine exactly which actions are 
>>> guaranteed with a single System.gc() and which may require more. At 
>>> a minimum we're relying on current implementation to assume a single 
>>> System.gc() suffices. There are also flags that can affect this - 
>>> though this test is not likely to ever be run with them.
>>
>> By looking at the different GCs it seems for System.gc() to reach 
>> SystemDictionary::do_unloading() they need 
>> GCLocker::check_active_before_gc() to return false, which will be do 
>> if _jni_lock_count=0, but I'm not sure when that can be guaranteed.
>> Some tests are relying on using ClassUnloadCommon.triggerUnloading() 
>> with flag "-Xmn8m" to trigger class unloading. I can replace 
>> System.gc() with that to make the assumption stronger. Do you think 
>> that works better?
>
> Not really, given this definition:
>
>     public static void triggerUnloading() {
>         allocateMemory(16 * 1024); // yg size is 8m with cms, force 
> young collection
>         System.gc();
>     }
>
> Leave it as-is and we'll just have to see if any intermittent failure 
> arise.

Actually, if Patricio is modifying the test anyway, triggerUnloading() 
is the standard way we've been unloading classes in the tests, so I 
think it would be better.  We can always strengthen it to guarantee some 
unloading if we need to.

thanks,
Coleen

>
> Thanks,
> David
>
>> Thanks,
>> Patricio
>>
>>>>> On 29/08/2018 7:52 AM, Patricio Chilano wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> Could you review this change that addresses the intermittent 
>>>>>> failure of test MemberNameLeak.java after 8206423 ?
>>>>>> The test had intermittent failures due to the ServiceThread not 
>>>>>> being able to clean up the resolved method table before the 
>>>>>> process exited. Now a counter is used to guarantee entries in the 
>>>>>> table are removed before exiting. Also variables _oops_removed 
>>>>>> and _oops_counted were declared local in 
>>>>>> ResolvedMethodTable::unlink() since they are not used outside it.
>>>>>> Webrev URL: 
>>>>>> http://cr.openjdk.java.net/~pchilanomate/8209844.01/webrev
>>>>>> Bug URL: https://bugs.openjdk.java.net/browse/JDK-8209844
>>>>>>
>>>>>> Thanks,
>>>>>> Patricio
>>>>
>>