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

Wed Nov 21 13:40:47 UTC 2018

Hi Vladimir,

Thanks for reviewing.

On 2018-11-21 02:04, Vladimir Kozlov wrote:
> 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?

Yes. The answer to is_alive() monotonically goes from true to false and 
never back. Similarly, the answer to is_unloading() monotonically goes 
from false to true and never back. Once is_unloading - always 
is_unloading(). This is intentional to avoid races where reading 
is_unloading() again would race and give a different answer.

Thanks,
/Erik

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