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

[Vladimir Kozlov]

I also thought about combining repeative checks is_alive() && !is_unloading() - to match new enum:

bool is_alive_and_not_unloading() { return is_alive() && !is_unloading(); }

But then I see only 5 cases and separate checks seems better for me.

One question is where you clean up 'unloading' state when nmethod state change to 'unloaded'? Is it possible that 
nmethod to have both state at the same time?

Thanks,
Vladimir

On 11/20/18 8:10 AM, Erik Österlund wrote:
> Hi Robbin,
> 
> Thanks for the review. I think I will go without is_dying() now because I couldn't figure out how to put it in, in an 
> intuitive way. Mostly because most of the code is checking for is_alive() && !is_unloading() compiled methods. In that 
> state they are not is_dying(), according to your definition, but checking for !is_dying() doesn't imply that it is 
> alive. So I think I will stick with being more explicit for now.
> 
> Thanks,
> /Erik
> 
> On 2018-11-20 10:57, Robbin Ehn wrote:
>> Looks good!
>>
>> I gave a suggestion IRL about an is_dying method, which covers the is_alive
>> and is_unloading query. If choose to take it or not I'll leave it up to you.
>>
>> Thanks, Robbin
>>
>> On 11/19/18 1:31 PM, Erik Österlund wrote:
>>> Hi Coleen,
>>>
>>> Thanks for having a look at this.
>>>
>>> New full webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8213755/webrev.01/
>>>
>>> Incremental:
>>> http://cr.openjdk.java.net/~eosterlund/8213755/webrev.00_01/
>>>
>>> On 2018-11-17 00:21, coleen.phillimore at oracle.com wrote:
>>>>
>>>> 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.
>>>
>>> You can't really have all_blobs in both. I tried out a variant with a single enum though, hope you like it.
>>> The observation was that we only need 3 different variations: all_blobs, only_alive or only_alive_and_not_unloading. 
>>> So I made an enum out of that.
>>> In fact, these three modes are the only ones I support right now, so they are also the only valid options, which made 
>>> the single enum look even more reasonable.
>>>
>>>> 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?
>>>
>>> IC caches to unloading nmethods are not necessarily cleaned out. The concurrent GC threads will clean the IC caches 
>>> concurrently, but a JavaThread can call the IC cache before the GC has gotten around to cleaning the IC cache. But 
>>> any such call into an is_unloading() nmethod will be trapped by nmethod entry barriers, which will call the IC miss 
>>> handler, which sorts things out lazily. So you can view this as the IC caches to is_unloading() nmethods being 
>>> logically cleared, but not necessarily physically cleared yet. The effect is the same - the is_unloading() nmethods 
>>> may not be entered from anywhere, including IC caches.
>>>
>>> So if you were to not filter is_unloading(), you would get a whole bunch of nmethods that are not really reachable by 
>>> the application, i.e. they are not on any stacks, and may not be entered by the application. Therefore, I think it's 
>>> perfectly fine to filter them out here, unless I missed something. And this makes the behaviour closer to as-if the 
>>> nmethods were indeed unloaded in the safepoint.
>>>
>>>> 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?
>>>
>>> Right, after the store is observed, the sweeper thread may concurrently observe the nmethod to be unloaded, and then 
>>> it may immediately flip it over to zombie. And I don't want any concurrent make_unloaded code in that nmethod to 
>>> still be racing after the sweeper turns it into zombie; a very unnecessary race to deal with.
>>>
>>> I updated the comment - hopefully it reads better now.
>>>
>>> Thanks,
>>> /Erik
>>>
>>>> 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
>>>>
>>>
>