RFR: 8191821: Finer granularity for GC verification (original) (raw)
Per Liden per.liden at oracle.com
Tue Dec 5 07:48:26 UTC 2017
- Previous message (by thread): [10] RFR (M/L): 8137099: G1 needs to "upgrade" GC within the safepoint if it can't allocate during that safepoint to avoid OoME
- Next message (by thread): RFR: 8191821: Finer granularity for GC verification
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
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
- Previous message (by thread): [10] RFR (M/L): 8137099: G1 needs to "upgrade" GC within the safepoint if it can't allocate during that safepoint to avoid OoME
- Next message (by thread): RFR: 8191821: Finer granularity for GC verification
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]