RFR: 8191821: Finer granularity for GC verification

[Per Liden]

Sorry for being late to this review.

Can we please name this new option G1VerifyGCType (or something 
similar). This seems very G1 specific to me and as a result I find it 
unfortunate that gcArguments has is involved here and has knowledge 
about this option.

cheers,
Per

On 2017-11-30 18:52, sangheon.kim wrote:
> Hi Stefan,
>
> On 11/29/2017 07:43 AM, Stefan Johansson wrote:
>> Thanks for reviewing Thomas,
>>
>> Updated webrevs:
>> Inc:  http://cr.openjdk.java.net/~sjohanss/8191821/01-02/
>> Full: http://cr.openjdk.java.net/~sjohanss/8191821/02/
> 02 version looks good.
>
> Thanks,
> Sangheon
>
>
>>
>> Comments inline.
>>
>> On 2017-11-29 12:16, Thomas Schatzl wrote:
>>> Hi,
>>>
>>> On Tue, 2017-11-28 at 17:25 +0100, Stefan Johansson wrote:
>>>> Hi,
>>>>
>>>> Please review this change for enhancement:
>>>> https://bugs.openjdk.java.net/browse/JDK-8191821
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~sjohanss/8191821/00/
>>>>
>>>> Summary:
>>>> Heap verification is a very good way to track down GC bugs, but it
>>>> comes
>>>> with a lot of overhead. Using VerifyBeforeGC, VerifyDuringGC and
>>>> VerifyAfterGC often slows down the execution more than is needed
>>>> since
>>>> we sometimes only want to verify certain types of GCs. This change
>>>> adds
>>>> this feature for G1 by adding a new diagnostic flag VerifyGCType.
>>>> The
>>>> new flag currently only works with G1 but can easily be added for
>>>> more
>>>> GCs if needed. The type of the flag is ccstrlist which means the
>>>> option
>>>> can be used multiple times to allow more than one type to be
>>>> verified.
>>>> The types available for G1 is, young, mixed, remark, cleanup and
>>>> full.
>>>> If the flag is not specified all GCs are verified.
>>>>
>>>> Note that Verify/Before/After/During/GC is still needed to decide
>>>> what
>>>> to verify, VerifyGCType only describes when.
>>>>
>>>> Testing:
>>>> * Added new Gtest for G1HeapVerifier functionality.
>>>> * Added new Jtreg test for basic command line functionality.
>>>> * Executed Jtreg tests through mach5 to make sure it passes on all
>>>> platforms.
>>>>
>>>    - I would like to see a special verification type for initial mark
>>> gcs, since these are part of the concurrent cycle. I mean, the change
>>> does not allow to verify the whole concurrent cycle alone without
>>> enabling verification for *all* young-only gcs.
>> Fixed.
>>>
>>>    - please change G1VerifyYoung to G1VerifyYoungOnly. Mixed gcs are
>>> also young gcs. We should be exact in our internal naming.
>> Fixed.
>>>
>>>    - in g1HeapVerifier.hpp consider aligning the flag values below each
>>> other.
>> Fixed.
>>>
>>>    - gcArguments.cpp:89: the warning message should print the given
>>> type. Not in "VerifyGCType %s", as that would be confusing, potentially
>>> only indicating that "type" is not supported.
>> Updated per Poonams request to only be printed once and with this I
>> think I can be left as is.
>>>
>>>    - gcArguments.cpp:109: are these really the only delimiters for
>>> arguments. Not sure if e.g. \t is also a delimiter. Isn't there some
>>> existing argument processing method available (or at least this
>>> constant) elsewhere?
>> So I basically used the same that were available for VerifySubSet but
>> also added \n since this is the delimiter added if the flag is used
>> multiple times. I think the expected way to use ccstrlist is to have
>> one value per usage like:
>> -XX:VerifyGCType=full -XX:VerifyGCType=young
>>
>> But since VerifySubSet allows comma separated values I decided to do
>> so as well. I people start using tabs I'm not sure we want to support
>> that. I would then rather just support "\n" and have one value per flag.
>>>
>>>    - gcArguments.hpp:52: maybe some documentation what this is supposed
>>> to do, and that the accepted types are GC specific.
>> Fixed.
>>>
>>>    - globals.hpp:2276: the list of available types is not complete
>>> (remark, cleanup?). Also there is no indication that the supported
>>> types are collector specific. Or just leave them out here, and document
>>> the available strings in the collector's parsing code header file (in
>>> the enum?)
>> Good point, will remove this listing and update the text.
>>>
>>>    - TestVerifyGCType.java: instead of using a GarbageProducer you can
>>> directly trigger specific GCs using whitebox. This would make the tests
>>> a lot more specific imho.
>> Agreed and fixed. Also added test for mixed and initial-mark now.
>>
>> Thanks,
>> Stefan
>>>
>>> Thanks,
>>>    Thomas
>>
>