RFR: 8209189: Make CompiledMethod::do_unloading more concurrent

[coleen.phillimore at oracle.com]

I think this looks really good. I appreciate the cleanup in this 
complexity.  It's nice that nmethod/CompiledMethod as non-gc structure 
with oops, there's simply an oops_do() vs. all these 
do_unloading_scopes() functions.   It reduces the special handling. I 
spent a lot of time trying to figure out how this "postponed" cleaning 
works.  This make a lot of sense.

http://cr.openjdk.java.net/~eosterlund/8209189/webrev.00/src/hotspot/share/code/compiledMethod.cpp.udiff.html

508 bool result = IsUnloadingBehaviour::current()->is_unloading(this);


This line is essentially the special sauce to this change.  What is hard 
to see immediately is that this doesn't just test the flag but will do 
an oops_do in order to set the flag.  Can you add a comment to this 
effect here?

Also, two GC threads could be visiting the same nmethods at the same 
time with this cleaning.  This is ok though as long as they set the 
is_unloading state correctly.

500 state._value = RawAccess<MO_RELAXED>::load(&_is_unloading_state);


Are these required or are you being pedantic?  Shouldn't it be ordered 
for multiple GC threads?

http://cr.openjdk.java.net/~eosterlund/8209189/webrev.00/src/hotspot/share/gc/shared/gcBehaviours.new.hpp.html

Despite the fact that you spelled "behavoirs" wrong, I think I'm warming 
up to this use of the concept here.  I'm having trouble thinking of a 
more specific word for it.

I have to confess to not reviewing the epoch handling very well.

Since there's a big alignment gap in CompiledMethod, why not just have 
two fields and skip this odd union?  One bool for is_unloading and one 
short for unloading_cycle?  Reading the word _inflated is strange here.

Thanks,
Coleen

On 10/19/18 10:22 AM, Erik Österlund wrote:
> Hi,
>
> Today the nmethods are unloaded either serially or in parallel due to 
> GC (and e.g. class unloading). With ZGC concurrent class unloading, a 
> concurrent fashion is required. Rather than inventing yet a third way 
> of unloading nmethods due to class unloading, it would be ideal if 
> there could be one unified way of doing it that makes everyone happy.
>
> To solve this problem in a more general way, a new member function on 
> CompiledMethod is added: is_unloading(). It returns whether a 
> CompiledMethod has broken oops. In order to not have to iterate over 
> all the oops every time this question is asked, it caches the result, 
> so it needs to be computed only once per "epoch". Every time a 
> CodeCache unloading is triggered by the GC, a new epoch is started, 
> which forces re-computation of whether the CompiledMethod 
> is_unloading() the first time it is called.
>
> So by having is_unloading() be lazily evaluated, it is now possible to 
> build a do_unloading() method on nmethod that simply makes the nmethod 
> unloaded if it is_unloading(), and otherwise cleans the various 
> caches. Cleaning e.g. the inline caches of said nmethod, uses the same 
> is_unloading() method on its targets to figure out if the IC cache 
> should be cleaned or not. Again, the epoched caching mechanism makes 
> sure we don't recompute this value.
>
> The new do_unloading() method may be called in both serial, parallel 
> and concurrent contexts. So I removed the parallel variation of this 
> that we had before, that unnecessarily postponed the unloading due to 
> not having computed whether the nmethod should be unloaded or not. 
> Since that is now computed on-demand lazily, there is no longer a need 
> for postponing this, nor to have two phases for parallel nmethod 
> unloading.
>
> While there is more work involved in making concurrent nmethod 
> unloading work, this is a good cleanup towards that goal.
>
> Bug ID:
> https://bugs.openjdk.java.net/browse/JDK-8209189
>
> Webrev:
> cr.openjdk.java.net/~eosterlund/8209189/webrev.00/
>
> Thanks,
> /Erik