RFR (XS): 8200362: G1Mux2Closure should disable implicit oop verification

[Stefan Karlsson]

On 2018-03-29 11:25, Stefan Johansson wrote:
> 
> 
> On 2018-03-29 11:20, Stefan Karlsson wrote:
>>
>>
>> On 2018-03-29 11:02, Thomas Schatzl wrote:
>>> Hi Stefan,
>>>
>>> On Thu, 2018-03-29 at 09:52 +0200, Stefan Johansson wrote:
>>>> Hi Thomas,
>>>>
>>>> On 2018-03-28 17:27, Thomas Schatzl wrote:
>>>>> Hi all,
>>>>>
>>>>>     can I have reviews for this small change that fixes some
>>>>> annoyance
>>>>> when debugging with G1?
>>>>>
>>>>> In particular, G1Mux2Closure is used by verification to verify and
>>>>> in
>>>>> case of error print out g1 specific error messages.
>>>>>
>>>>> However, since G1Mux2Closure is an OopClosure, it is actually
>>>>> wrapped
>>>>> by NoHeaderExtendedOopClosure in oop_iterate* calls.
>>>>> ExtendedOopClosure
>>>>> has its own verification that is very generic, and may trigger in
>>>>> the
>>>>> same situations as G1Mux2Closure.
>>>>>
>>>>> Disable this implicit verification for G1Mux2Closure so that we
>>>>> always
>>>>> get the g1 specific error messages.
>>>>>
>>>>> CR:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8200362
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~tschatzl/8200362/webrev/
>>>>
>>>> Nice change, always good to get detailed information. I think you
>>>> need
>>>> to change the code using the G1Mux2Closure as well. On line 666 in
>>>> heapRegion.cpp:
>>>>     G1Mux2Closure mux(&vl_cl, &vr_cl);
>>>>     obj->oop_iterate_no_header(&mux);
>>>> This should now be a normal oop_iterate(&mux) to make sure the right
>>>> verification is done.
>>>
>>> Fixed. Also looked through other closures used for verification.
>>>
>>> http://cr.openjdk.java.net/~tschatzl/8200362/webrev.0_to_1 (diff)
>>> http://cr.openjdk.java.net/~tschatzl/8200362/webrev.1 (full)
>>
>> Looks good.
>>
>> I like that we get rid of usages of oop_iterate_no_header. Maybe we 
>> should remove the other usages in G1, as a separate RFE.
>>
> I like the sound of that, maybe something for Leo to look at. Any 
> history or reason why we have these?

Some years ago ExtendedOopClosure and OopClosure were only one class, 
the OopClosure. The ExtendedOopClosure was created and all GC specific 
code needed for the oop_iterate functions were pulled out from 
OopClosure and put into ExtendedOopClosure.

ExtendedOopClosure is very GC centric, while OopClosure is a much leaner 
API to apply a closure to an oop* (or an narrowOop*).

The iterate_oop_no_header (and NoHeaderExtendedOopClosure) was intended 
to simplify non-GC JVM code to apply an OopClosure to all oop* of an 
object, without having to understand the ExtendedOopClosure class. The 
no_header name is legacy from the Permgen when we sometimes walked the 
"header", the klass pointer. Today no_header means "don't care about 
metadata pointers, or other GC stuff".

We've recently realized that instanceRefKlass::oop_oop_iterate is broken 
when we run GCs with load barriers and concurrent reference processing, 
and non-GC code applies a normal, strong, load barrier to the referent 
field. That strong load barrier will try to use the stale pointer, and 
most likely cause crashes. Therefore, oop_iterate_no_header is 
problematic, and should probably be completely removed. The GCs can deal 
with this by simply implementing ExtendedOopClosure and use oop_iterate, 
just like we do in this patch.

StefanK

> 
> StefanJ
>> StefanK
>>
>>>
>>> Thomas
>>>
>