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

Erik Österlund erik.osterlund at oracle.com
Wed Nov 21 20:16:55 UTC 2018 

Hi Vladimir,

On 2018-11-21 19:30, Vladimir Kozlov wrote:
> On 11/21/18 5:40 AM, Erik Österlund wrote:
>> 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.
> 
> Okay, so nmethod will be is_unloading() regardless _state value until it 
> is removed from CodeCache.

Exactly.

> I am confused by code in  bool nmethod::can_convert_to_zombie() which 
> from one side check only is_unloading() in assert but both 
> (!is_unloading() || is_unloaded()) in final condition. On other hand 
> can_convert_to_zombie() is only called when cm->is_not_entrant() is true 
> in NMethodSweeper::process_compiled_method().

What this code is dealing with is that the sweeper finds a not_entrant 
nmethod. But it could race with the GC unloading nmethods concurrently. 
And then by the time we call can_convert_to_zombie(), it might no longer 
be not_entrant(). If the nmethod is_unloading(), it can flip to unloaded 
at any time. So what this code is saying is that it is still okay to in 
this race convert the nmethod to zombie if it becomes unloaded. Then 
since it was not_entrant, we know it was a non-OSR nmethod, and that it 
should indeed convert over to zombie.

Thanks,
/Erik

> Thanks,
> Vladimir
> 
>>
>> 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
>>>>>>>
>>>>>>
>>>>
>>

More information about the hotspot-dev mailing list