[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-gc-dev
Subject:    Re: RFR: 8191821: Finer granularity for GC verification
From:       Per Liden <per.liden () oracle ! com>
Date:       2017-12-05 8:14:16
Message-ID: 18a606d7-29d1-6684-6d5f-d23afc4e9531 () oracle ! com
[Download RAW message or body]

I realize I looked at the wrong version of the patch, so the G1 specific 
stuff was removed. But still, I find it unfortunate that gcArguments is 
involved here and that we now have yet another post_initialize function.

/Per


On 2017-12-05 08:48, Per Liden wrote:
> 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
>>>
>>
[prev in list] [next in list] [prev in thread] [next in thread]