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

[Vladimir Kozlov]

On 11/30/18 3:19 AM, 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.

Thank you for explanation. Can you add short comment about this? Don't need to re-review it.

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

Thanks,
Vladimir

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