RFR: 8191821: Finer granularity for GC verification

[sangheon.kim]

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.

----------------------------------------------------------------------
src/hotspot/share/gc/g1/g1CollectedHeap.cpp
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

----------------------------------------------------------------------
src/hotspot/share/gc/g1/g1ConcurrentMark.cpp
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

----------------------------------------------------------------------
src/hotspot/share/gc/g1/g1FullCollector.cpp
249     //Only do verification if VerifyDuringGC and Verify_Full is set.
- A space between '//' and 'Only'. (Not part of your change)

----------------------------------------------------------------------
src/hotspot/share/gc/g1/g1HeapVerifier.cpp
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'

----------------------------------------------------------------------
test/hotspot/gtest/gc/g1/test_g1HeapVerifier.cpp
73
- Remove additional lines.

----------------------------------------------------------------------
test/hotspot/jtreg/gc/g1/TestVerifyGCType.java
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?

----------------------------------------------------------------------
src/hotspot/share/runtime/globals.hpp
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,
Sangheon


>
> Thanks,
> Stefan