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

Tue Nov 20 16:22:08 UTC 2018

On 11/20/18 11:04 AM, Erik Österlund wrote:
> Hi Coleen,
>
> Thanks for the review.
>
> On 2018-11-19 23:42, coleen.phillimore at oracle.com wrote:
>>
>>
>> On 11/19/18 7:31 AM, 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.
>>
>> I do like it!
>>>
>>>> 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.
>>
>> Ok.  The thing I'm worried about is if we deallocate memory for a 
>> Method* in the is_unloading methods, and some other cleaning or 
>> sweeping thread tries to access that memory.  This is used by 
>> MetadataOnStackMark during safepoints to see what's safe to 
>> deallocate from redefinition.
>
> I understand. I do think this is safe though, and it only walked 
> is_alive() compiled methods looking for metadata before, e.g. not 
> unloaded or zombie methods. Similarly, nobody should be poking around 
> at the metadata of unloading methods. Should being the key word there 
> I guess...
>
>>
>>>
>>>> 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.
>>>
>>
>> How about removing a comma changing it to accesses are not safe, or 
>> access is not safe.
>> + // The release is only needed for compile-time ordering, as accesses
>> + // into the nmethod after the store are not safe, due to the sweeper
>> + // being allowed to free it when the store is observed during
>> + // concurrent nmethod unloading. Therefore, there is no need for
>> + // acquire on the loader side.
>
> Comma will be removed before pushing.
>
>> http://cr.openjdk.java.net/~eosterlund/8213755/webrev.01/src/hotspot/share/code/codeCache.hpp.udiff.html
>>
>> Why is this copy constructor defined?  Isn't this what the default 
>> copy constructor would do?
>
> Yeah it is. I like explicitly typing out what constructors should do. 
> But I can remove it if you prefer that.

Can you remove it?  It's confusing there since we don't generally have 
default constructors typed out, one has to study it to see that it's a 
default copy constructor.  Then ask myself why is it there.

I don't need to see another webrev.

Thanks,
Coleen
>
>> It looks almost perfect to me :)
>
> Awesome. Thanks for reviewing this.
>
> Thanks,
> /Erik
>
>> Coleen
>>
>>> 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
>>>>
>>>
>>
>