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

[Erik Osterlund]

Hi Vladimir,

> On 30 Nov 2018, at 18:53, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
> 
>> 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.

Yes, will do.

> 
>>> 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 for the review!

/Erik

> 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