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

[Mikael Gerdin]

Hi Poonam,

On 2016-01-14 19:48, Poonam Bajaj Parhar wrote:
> 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/

Looks good!
/Mikael

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