RFR: 8209189: Make CompiledMethod::do_unloading more concurrent

[Erik Österlund]

Hi Coleen,

On 2018-10-26 19:31, coleen.phillimore at oracle.com wrote:
> 
> 
> 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; ?

Yes basically. It's equivalent to Atomic::store, which is (currently) 
equivalent to a normal volatile store as you say. Except I managed to 
use way more templates to express the same thing in a semantically more 
satisfying way. And that makes me feel a lot better. I'm not biased...

After Kim's C++14 upgrades, it will map to memory_order_relaxed accesses 
(hence the name MO_RELAXED), as volatile accesses don't really guarantee 
atomicity according to the C++14 standard. In fact, it will be 
explicitly undefined behaviour then.

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

Will do!

Thanks for the review.

/Erik

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