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

[David Holmes]

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.

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