RFR: 8209189: Make CompiledMethod::do_unloading more concurrent

Fri Oct 26 17:31:17 UTC 2018

On 10/26/18 12:25 PM, Erik Österlund wrote:
> 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.

So is RawAccess<MO_RELAXED> the same as:

state._value = _is_unloading_state; ?
>
>> 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.

Can you put some sort of comment above the IsUnloading{Union,Struct} to 
that effect?  I don't need to see it.  I'm sure your English will be good :)

Coleen

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