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

[Poonam Bajaj Parhar]

Hello Mikael,

On 1/11/2016 6:14 AM, Mikael Gerdin wrote:
> On 2015-12-22 01:34, Poonam Bajaj Parhar wrote:
>> Hello Mikael,
>>
>> Thanks for your feedback.
>>
>> Here's the updated webrev:
>> http://cr.openjdk.java.net/~poonam/8145442/webrev.01/
>>
>> On 12/21/2015 6:11 AM, Mikael Gerdin wrote:
>>> Hi Poonam,
>>>
>>> On 2015-12-15 22:32, 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/
>>>
>>> I have some general comments on the structure of the code:
>>>
>>> First of all, I would prefer if we could avoid adding yet another
>>> command line flag.
>>> I think it would be preferable to either use VerifyRememberedSets
>>> which already exists or to use the code you've proposed adding under
>>> 8072725 with a "remset" verification subset (and deprecating
>>> VerifyRememberedSets).
>>>
>> I removed the new option and am using VerifyRememberedSets now. After I
>> have integrated the changes for 8072725, we can add 'remset' to the
>> VerifySubSet and can control the rem-set verification of all the
>> collectors under that.
>>
>>> Having RSet verification only post-gc and not before does not appeal
>>> to me, since it's quite possible that we can have missing write
>>> barriers in compiled code leading to missing remembered set entries as
>>> well.
>>>
>> I have made changes to verify rem-sets both before and after GC.
>>
>>> Folding this into the VerifyBefore/After code would allow users to 
>>> select
>>> -XX:+VerifyAfterGC -XX:VerifySubSet=remset
>>> to get the behavior you are suggesting in this webrev.
>>>
>>>
>>> I like Jon's suggestion of moving all the remset verification to a
>>> VerifyRemSetClosure which can be aggregated in the VerifyLiveClosure
>>> if full verification is enabled.
>>>
>> I moved the rem-set verification code to VerifyRSetClosure and derived
>> VerifyLiveClosure from that.
>
> I think it would be better to make an abstract base class,
> class G1VerificationClosure : public OopClosure {}
> (or something similar) and move common data members to that base class 
> instead of making VerifyRemSetClosure be both a base class and a 
> concrete class.
>
> Also, please move the core of the VerifyLiveClosure to a separate 
> method, similar to how you did in VerifyRemSetClosure, perhaps leaving 
> out the oop load and null check. It makes the code easier to read in 
> my opinion.
>
> With this kind of set up you could have the HeapRegion::verify object 
> iteration code do
> G1Mux2Closure<VerifyLiveClosure, VerifyRemSetClosure> mux(&vl_cl, 
> &vr_cl);
> obj->oop_iterate(&mux);
>
> and HeapRegion::verify_remembered_sets() do
> VerifyRemSetClosure vr_cl...
> obj->oop_iterate(&vr_cl);
>
>
> verifyRemSets does not follow the naming conventions of the 
> surrounding code which uses underscores and rarely uses short hands, 
> I'd suggest naming it verify_remembered_set.
>
> the same naming issue is present in HeapRegion::verifyRSet
> also, you've put in a newline after the VerifyOption to verifyRSet for 
> unclear reasons.
>

I have incorporated your suggestions and here's the updated webrev:
http://cr.openjdk.java.net/~poonam/8145442/webrev.01/

Please take a look and let me know your feedback.

Thanks,
Poonam

>
> /Mikael
>
>>
>>> So, given that I've asked for re-use of the VerifyBefore/After code I
>>> would suggest some changes in
>>> HeapRegion::verify(VerifyOption vo, bool* failures)
>>> to make the code select either the VerifyLiveClosure or the
>>> VerifyRemSetClosure depending on the level of verification.
>>>
>>
>> Yes, sure we can add more VerifySubSet levels after I have integrated
>> the changes for 8072725.
>>
>>
>> Thanks,
>> Poonam
>>
>>> The BOT verification near the end of ::verify could be conditionalized
>>> on VerifySubSet=bot or something similar.
>>>
>>> /Mikael
>>>
>>>> Testing: JPRT, GCBasher
>>>>
>>>> Thanks,
>>>> Poonam
>>>>
>>>
>>
>