RFR: 8209189: Make CompiledMethod::do_unloading more concurrent

[Erik Österlund]

Hi Coleen,

Thank you for reviewing this.

On 2018-10-26 17:58, coleen.phillimore at oracle.com wrote:
> 
> 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.

I'm glad you like it! I also think the new model is more intuitive.

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

Done.

New incremental (including Vladimir's suggestion to remove now dead code):
http://cr.openjdk.java.net/~eosterlund/8209189/webrev.00_01/

Full:
http://cr.openjdk.java.net/~eosterlund/8209189/webrev.01/


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

Exactly. The value will monotonically change, possibly by multiple 
threads (but in practice mostly by one thread) to the same value. If two 
threads race, they will race for writing the same value, which is harmless.

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

No, ordering is irrelevant here. What is relevant is that the access is 
atomic; that's why I use MO_RELAXED, which gives me those guarantees.

As mentioned previously, two threads may race, and that is harmless; 
they will race for writing the same cached value. There are no ordering 
constraints.

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

Sorry Coleen, but "behaviours" is the correct English spelling. American 
English is not English. Shots fired...

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

I use the union to make sure that both values are accessed as a pair 
atomically. The union allows me to represent the same data either as a 
single value that can be accessed atomically, and then interpret it as 
an "inflated" view where it comprises a tuple representing both an epoch 
and result value. The single value is used to ensure atomicity, and the 
inflated view is used for interpreting what the state represents.

Thanks,
/Erik

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