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

Wed Nov 21 21:33:01 UTC 2018

Good. Please, add/update comments to explain all cases in can_convert_to_zombie().

Thanks,
Vladimir

On 11/21/18 12:16 PM, Erik Österlund wrote:
> 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
>>>>>>>>
>>>>>>>
>>>>>
>>>