RFR: 8191821: Finer granularity for GC verification

sangheon.kim sangheon.kim at oracle.com
Wed Nov 29 07:01:41 UTC 2017

Hi Stefan,

On 11/28/2017 08:25 AM, 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.
Thank you for improving this area!

Looks good, I have minor nits.

2987 _verifier->verify_before_gc(collector_state()->yc_type() == Mixed ? 
G1HeapVerifier::G1VerifyMixed : G1HeapVerifier::G1VerifyYoung);
3147 _verifier->verify_after_gc(collector_state()->yc_type() == Mixed ? 
G1HeapVerifier::G1VerifyMixed : G1HeapVerifier::G1VerifyYoung);
- (weak suggestion) Change for better readability? e.g. split into 3 lines

1018 g1h->verifier()->verify(G1HeapVerifier::G1VerifyRemark, 
VerifyOption_G1UsePrevMarking, "During GC (before)");
1039 g1h->verifier()->verify(G1HeapVerifier::G1VerifyRemark, 
VerifyOption_G1UsePrevMarking, "During GC (overflow)");
1054 g1h->verifier()->verify(G1HeapVerifier::G1VerifyRemark, 
VerifyOption_G1UseNextMarking, "During GC (after)");
1186 g1h->verifier()->verify(G1HeapVerifier::G1VerifyCleanup, 
VerifyOption_G1UsePrevMarking, "During GC (before)");
1258 g1h->verifier()->verify(G1HeapVerifier::G1VerifyCleanup, 
VerifyOption_G1UsePrevMarking, "During GC (after)");
- (weak suggestion) Change for better readability? e.g. split into 3 lines

249     //Only do verification if VerifyDuringGC and Verify_Full is set.
- A space between '//' and 'Only'. (Not part of your change)

380   if (strcmp(type, "young") == 0) {
- strncmp

391     log_warning(gc, verify)("VerifyGCType: '%s' is unknown. 
Available are: young, mixed, remark, cleanup and full ", type);
- "Available are" -> "Available types are"?

396   // First enable will clear _types.
- '_types' seems old name. Probably something like 'Clear 
_enabled_verification_types if verifies all types'

- Remove additional lines.

42     public static final String  VERIFY_TAG    = "[gc,verify]";
- There are additional space between String and VERIFY_TAG. line 43, 44, 
45 as well.

48         // Test with all verification enabled
49         OutputAnalyzer output = testWithVerificationType(new String[0]);
- Can we split into sub-tests functions for better readability?

2276              "Available types are: young, mixed, concurrent, 
full")         \
- "concurrent," -> "remark, cleanup and"

* I don't see any length limitation logic for VerifyGCType. Not sure how 
other ccstrlist type command-line options are handled, but it would be 
better to have one.


> Thanks,
> Stefan

More information about the hotspot-gc-dev mailing list