RFR: 8191821: Finer granularity for GC verification

[Thomas Schatzl]

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.

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

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

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

Thanks,
  Thomas