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

[coleen.phillimore at oracle.com]

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.

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

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?

It looks almost perfect to me :)

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