RFR: 8191821: Finer granularity for GC verification

Stefan Johansson stefan.johansson at oracle.com
Wed Nov 29 15:43:35 UTC 2017

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/

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.
>    - please change G1VerifyYoung to G1VerifyYoungOnly. Mixed gcs are
> also young gcs. We should be exact in our internal naming.
>    - in g1HeapVerifier.hpp consider aligning the flag values below each
> other.
>    - 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.
>    - 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,
>    Thomas

More information about the hotspot-gc-dev mailing list