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

[Vladimir Kozlov]

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