RFR: 8214302: Allow safely calling is_unloading() on zombie nmethods

[Per Liden]

On 11/30/18 12:19 PM, Erik Österlund wrote:
> Hi Vladimir,
> 
> On 2018-11-27 01:40, Vladimir Kozlov wrote:
>> Hi Erik,
>>
>> Can you tell if there is any concurrency window where you check 
>> is_zombie and store new unloading state - can other thread change 
>> nmethod to zombie?
> 
> Good question. Short version is: no.
> Here is the longer version to explain why.
> 
> There are 2 modes of importance. Let's start with the first one - STW 
> unloading. With STW unloading, all is_alive && is_unloading() nmethods 
> are unlinked and unloaded in the safepoint. That makes races where an 
> nmethod is first observed as is_alive() && is_unloading() and 
> subsequently observed as is_zombie() impossible.
> 
> The more tricky case is with concurrent unloading. The way I have 
> structured this code is to unlink all references to is_unloading() 
> nmethods (IC caches and dependency contexts) when the mark end safepoint 
> is released, and then perform a global handshake operation with all 
> JavaThreads before unloading them. The sweeper never converts is_alive 
> && is_unloading() nmethods to zombies; it waits for them to become 
> is_unloaded(). So before the global handshake, it is impossible for 
> is_unloading() nmethods to racingly become is_zombie(). And 
> is_unloading() is calculated for all is_alive() nmethods before taking 
> that global handshake, meaning that it will never be recalculated after 
> the handshake.
> 
> After that global handshake, is_unloading() nmethods are only observable 
> to the iterators, and they will never trigger recomputation of the 
> cached is_unloading_state, and hence may not suffer from such races.
> 
>> I also noticed that CodeCache::unloading_cycle() is called twice in 
>> this code. Can we cache it in local?
> 
> Yes, sure.
> 
> Incremental:
> http://cr.openjdk.java.net/~eosterlund/8214302/webrev.00_01/
> 
> Full:
> http://cr.openjdk.java.net/~eosterlund/8214302/webrev.01/

Looks good to me.

/Per

> 
> Thanks,
> /Erik
> 
>> Thanks,
>> Vladimir
>>
>> On 11/26/18 7:30 AM, Erik Österlund wrote:
>>> Hi,
>>>
>>> It is currently not safe to call is_unloading on zombie nmethods, 
>>> unless it has been observed to be alive. It should be supported to 
>>> make the code less fragile. When encountering a !is_alive() nmethod 
>>> that has not had its unloading epoch updated, and ask if it 
>>> is_unloading(), the answer is always false. So by adding that, 
>>> is_unloading() can always be safely called.
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8214302/webrev.00/
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8214302
>>>
>>> Thanks,
>>> /Erik