RFR: JDK-8145442: Add an option to verify remembered sets for G1

[Jon Masamitsu]

Poonam,

Thanks for the changes.  Looks good.

Reviewed.

Jon

On 12/21/2015 4:24 PM, Poonam Bajaj Parhar wrote:
> Hello Jon,
>
> Thanks for taking a look at the changes. Here's the updated webrev and 
> please see my comments inline:
> http://cr.openjdk.java.net/~poonam/8145442/webrev.01/
>
> On 12/18/2015 3:20 PM, Jon Masamitsu wrote:
>> Poonam,
>>
>> Generally looks good.
>>
>> http://cr.openjdk.java.net/~poonam/8145442/webrev.00/src/share/vm/gc/g1/g1CollectedHeap.cpp.frames.html 
>>
>>
>> The new GC unified logging implementation has been pushed so you'll
>> need to replace of gclog_or_tty().
>>
>>> 4060 if (G1VerifyRSetsAfterEvacuationPause) {
>>> 4061 gclog_or_tty->print("[Verifying RSets]");
>>> 4062 VerifyRegionRSetClosure v_cl;
>>> 4063 heap_region_iterate(&v_cl);
>>> 4064 }
>>
>> Probably with
>>
>>   log_debug(gc, verify)("[Verifying RSets]");
>>
>>
> Done. Thanks for pointing out.
>
>> http://cr.openjdk.java.net/~poonam/8145442/webrev.00/src/share/vm/gc/g1/heapRegion.hpp.frames.html 
>>
>>
>>> 755 void verifyRSet(VerifyOption vo, bool *failures) const;
>>
>> Any comment worth adding for verifyRSet()?  Are there conditions on 
>> where and
>> when it can be used?
>
> Added a comment there. I don't think there is any specific condition 
> when it should be used.
> It verifies the rem-set entries for that heap region.
>>
>> In verifyRSet() you always use VerifyOption_G1UsePrevMarking. Will 
>> that always
>> be the value for the VerifyOption?  If it is always going to be that, 
>> does it work
>> to remove  the VerifyOption parameter from verifyRSet() and has 
>> verifyRSet()
>> always pass VerifyOption_G1UsePrevMarking to VerifyRSetClosure?
>>
> Currently it is only VerifyOption_G1UsePrevMarking that is being 
> passed to this function. But I would
> prefer to leave it like this to keep it consistent with the verify() 
> funtion:
>
> void verify(VerifyOption vo, bool *failures) const;
>
> The comments for verify() in heapRegion.hpp are true for verifyRSet() 
> too.
>
>> http://cr.openjdk.java.net/~poonam/8145442/webrev.00/src/share/vm/gc/g1/heapRegion.cpp.frames.html 
>>
>>
>> 917 VerifyRSetClosure v_rset_cl(g1, VerifyOption_G1UsePrevMarking);
>>
>> I can see HeapRegionVerifyClosure::verifyRemSets() is used and it 
>> works out
>> well but it seems odd for HeapRegionVerifyClosure (which has a name
>> that I read as generally useful with different Verify closures) to have
>> a method that is specific to remembered-set verification.
>>
>> I think it is like this.
>>
>> HeapRegionVerifyClosure had a verifyRemSets() method that is
>> called from do_oop in VerifyRSetClosure and VerifyLiveClosure.
>>
>> Then when you want to do remembered-set verification you
>> use a VerifyRSetClosure and when you want to verify liveness,
>> you use a VerifyLiveClosure and get the remembered-set
>> verfication included in the VerifyLiveClosure.
>>
>> An alternative would be to put verifyRemSets() into VerifyRSetClosure
>> and have a VerifyRSetClosure as part of the VerifyLiveClosure. Then
>> instead of calling verifyRemSets() (as inherited from 
>> HeapRegionVerifyClosure)
>> in VerifyLiveClosure, you could call verifyRemSets() using the embedded
>> VerifyRSetClosure.
>>
>> Or you could collapse HeapRegionVerifyClosure and VerifyRSetClosure
>> together (give it the name VerifyRSetClosure so that having a 
>> verifyRemSets()
>> seems more natural) and derive VerifyLiveClosure from the new
>> VerifyRSetClosure.
>>
> I like the second alternative better. I have made the changes in the 
> latest webrev to this effect.
>
> Please take a look at the new webrev and let me know your feedback.
>
> Thanks,
> Poonam
>
>> Jon
>>
>> On 12/15/2015 1:32 PM, Poonam Bajaj Parhar wrote:
>>> Hello,
>>>
>>> Please review these changes that add a diagnostic VM option 
>>> 'G1VerifyRSetsAfterEvacuationPause' that when turned on enables the 
>>> verification of Rem Sets of heap regions.
>>>
>>> Bug: JDK-8145442: Add an option to verify remembered sets for G1
>>> Webrev: http://cr.openjdk.java.net/~poonam/8145442/webrev.00/
>>> Testing: JPRT, GCBasher
>>>
>>> Thanks,
>>> Poonam
>>>
>>
>