[9] RFR(S): 8074553: Crash with assert(!is_unloaded()) failed: should not call follow on unloaded nmethod

[Tobias Hartmann]

Thanks, Vladimir!

Best regards,
Tobias

On 05.04.2016 01:09, Vladimir Kozlov wrote:
> Thank you for explaining. webrev.01 looks good.
> 
> Thanks,
> Vladimir
> 
> On 3/31/16 11:30 PM, Tobias Hartmann wrote:
>> Hi Vladimir,
>>
>> On 31.03.2016 17:49, Vladimir Kozlov wrote:
>>> On 3/31/16 6:22 AM, Tobias Hartmann wrote:
>>>> Hi,
>>>>
>>>> please review the following patch:
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8074553
>>>> http://cr.openjdk.java.net/~thartmann/8074553/webrev.00/
>>>>
>>>> While the code cache sweeper processes a nmethod in NMethodSweeper::process_nmethod(), safepoints may happen and the GC may unload the currently processed nmethod. To prevent this, the sweeper uses a NMethodMarker which saves the nmethod in CodeCacheSweeperThread::_scanned_nmethod. The nmethod is then passed to the GC through a CodeBlobClosure in CodeCacheSweeperThread::oops_do() to keep it alive when the GC iterates over all threads.
>>>>
>>>> The problem is that G1 calls nmethods_do() on all threads in the remark phase (see G1RemarkThreadsClosure::do_thread()) which is not overwritten by the sweeper thread. Since the currently processed nmethod is not passed through nmethods_do() by any thread, it is unloaded and we later hit the assert when encountering the nmethod through oops_do().
>>>>
>>>> Mikael Gerdin and Stefan Karlsson (thanks again!) suggested to overwrite nmethods_do() as well in CodeCacheSweeperThread and pass _scanned_nmethod to the closure. I also modified Threads::nmethods_do() to ignore the sweeper thread because we want to avoid marking the _scanned_nmethod as seen on the stack when scanning stacks from the sweeper (the nmethod may already be zombie and only referenced by the sweeper).
>>>
>>> I did not get this. If you exclude CodeCacheSweeperThread in Threads::nmethods_do() then CodeCacheSweeperThread::nmethods_do() will not be called. What is the point?
>>
>> The point is that the GC code calls JavaThread::nmethods_do() (which I modified to include  _scanned_nmethod) and not Threads::nmethods_do(). The latter one is only used by the CodeCacheSweeperThread to mark nmethods active on the Java stack and should therefore *exclude* _scanned_nmethod. This is because _scanned_nmethod should only be prevented from being unloaded by the GC but the hotness value or stack marking should not be affected (it may very well a zombie already).
>>
>> Please also note that there is Thread*s*::nmethods_do() and Thread::nmethods_do() which is a bit confusing.
>>
>> However, Mikael is right that nmethods_do() should be virtual (like oops_do() is) to allow the GC code to call the CodeCacheSweeperThread::nmethods_do() version.
>>
>> Thanks,
>> Tobias
>>
>>>
>>> Thanks,
>>> Vladimir
>>>
>>>>
>>>> Unfortunately, this bug is extremely hard to reproduce (it showed up 18 times since early 2015). I was able to reproduce it only once after thousands of runs and therefore not often enough to verify the fix. However, I'm very confident that this solves the problem.
>>>>
>>>> Tested with JPRT and RBT (running).
>>>>
>>>> Thanks,
>>>> Tobias
>>>>