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

[Poonam Bajaj Parhar]

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