RFR: 8213755: Let nmethods be is_unloading() outside of safepoints

[coleen.phillimore at oracle.com]

How about instead of two bool arguments that are hard to read and error 
prone and need comments, like:

76 CompiledMethodIterator iter(true /* only_alive */, false /* 
only_not_unloading */);


enum FilterAlive { all_blobs, only_alive };
enum FilterUnloading { all_blobs, only_not_unloading };

Then it can be something like:

    CompiledMethodIterator iter(only_alive, all_blobs);

Don't know if you can repeate all_blobs like that.

http://cr.openjdk.java.net/~eosterlund/8213755/webrev.00/src/hotspot/share/code/codeCache.cpp.frames.html

I'm not sure if metadata_do() should filter out unloading.  It's looking 
for "old" methods to stop from deallocating memory for them (but it's in 
a safepoint).  If you're unloading, the IC caches are already cleaned out?

http://cr.openjdk.java.net/~eosterlund/8213755/webrev.00/src/hotspot/share/code/nmethod.cpp.frames.html

1108 // The release is only needed for compile-time ordering, as accesses
1109 // into the nmethod after the store is not safe, due to the sweeper
1110 // being allowed to free it then, in the case of concurrent nmethod
1111 // unloading. Therefore, there is no need for acquire on the loader 
side.
1112 OrderAccess::release_store(&_state, (signed char)unloaded);

I tried to make sense out of this first sentence, but couldn't really.  
After the store to unloaded, can the sweeper free the nmethod?  maybe 
remove "then, " and it would make more sense?

Thanks,
Coleen

On 11/12/18 5:46 PM, Erik Österlund wrote:
> ..put in bug number in subject to make mail filters happy.
>
> /Erik
>
> On 2018-11-12 23:44, Erik Österlund wrote:
>> Hi,
>>
>> All current GCs perform code cache unloading in safepoints. 
>> Therefore, nmethods that are alive but is_unloading() are never 
>> observed outside of safepoints. With concurrent class unloading, 
>> nmethods that are alive but is_unloading() will become observable 
>> outside of safepoints. This must be handled appropriately.
>>
>> In this patch I changed the nmethod/compiled method iterators to 
>> accept parameters describing whether they should filter out not 
>> is_alive() or is_unloading() methods. Since there is no obvious 
>> default (all combinations are used depending on call site), you have 
>> to explicitly set what iteration mode you want.
>>
>> Other than that, I make sure that the sweeper stays away from 
>> is_unloading() nmethods that are not yet is_unloaded(). To make the 
>> interactions between the sweeper and concurrent GC threads safe, I 
>> had to move down the store that sets the state to unloaded, and use a 
>> release_store there, to make sure no accesses float below it at 
>> compile-time. Once that store is observed, nmethods may be deleted.
>>
>> In the IC miss handler, I also need to lazily clean stale IC caches 
>> due to calling is_unloading nmethods using nmethod entry barriers.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8213755/webrev.00/
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8213755
>>
>> Thanks,
>> /Erik