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

[David Holmes]

Hi Patricio,

On 5/09/2018 10:11 AM, Patricio Chilano wrote:
> Hi David,
> 
> 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.

>> Exposing the actual change to the resolved method table via the WB API 
>> seems quite reasonable, but in general busy polling loops should be 
>> avoided by inserting short sleeps between polls.
> I can add a delay since I have to fix the test to add the 
> UnlockDiagnosticVMOptions flag as you correctly found (I was running all 
> tests in debug mode). I see some tests have delays of 100ms, should that 
> delay be okay?

Yes 100ms should be fine.

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
>