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

Wed Nov 21 18:30:54 UTC 2018

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.

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().

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