RFR: 8191821: Finer granularity for GC verification

[Stefan Johansson]

Thanks for reviewing Sangheon,

A lot of good comments, see my answers inline. Here are new webrevs:
Inc: http://cr.openjdk.java.net/~sjohanss/8191821/00-01/
Full: http://cr.openjdk.java.net/~sjohanss/8191821/01/

On 2017-11-29 08:01, sangheon.kim wrote:
> 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
>
Fixed by adding a function to decide what type it is.
> ----------------------------------------------------------------------
> 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
>
I actually think the current version reads better, leaving as is.
> ----------------------------------------------------------------------
> 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)
>
Fixed, and changed to G1VerifyFull.
> ----------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1HeapVerifier.cpp
> 380   if (strcmp(type, "young") == 0) {
> - strncmp
We can't use strncmp here since we want to ensure a perfect match.
>
> 391     log_warning(gc, verify)("VerifyGCType: '%s' is unknown. 
> Available are: young, mixed, remark, cleanup and full ", type);
> - "Available are" -> "Available types are"?
>
Fixed.
> 396 // First enable will clear _types.
> - '_types' seems old name. Probably something like 'Clear 
> _enabled_verification_types if verifies all types'
>
Correct, fixed.
> ----------------------------------------------------------------------
> test/hotspot/gtest/gc/g1/test_g1HeapVerifier.cpp
> 73
> - Remove additional lines.
>
Fixed.
> ----------------------------------------------------------------------
> 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?
Fixed.
>
> ----------------------------------------------------------------------
> src/hotspot/share/runtime/globals.hpp
> 2276              "Available types are: young, mixed, concurrent, 
> full")         \
> - "concurrent," -> "remark, cleanup and"
Fixed.
>
> * 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.
I don't see any other ccstrlist options do any specific length handling 
and I'm not sure if there is a problem. For each time the option 
VerifyGCType is encountered on the command line a new char array will be 
allocated with NEW_C_HEAP_ARRAY and the old and new value will be added 
to the new array. This can of course fail with a native OOM, but so can 
a lot of other stuff during startup.

Thanks,
Stefan

>
> Thanks,
> Sangheon
>
>
>>
>> Thanks,
>> Stefan
>