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

[Mikael Gerdin]

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.


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