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

[Erik Österlund]

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/

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