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

[Tobias Hartmann]

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