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

[Erik Österlund]

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
>