RFR: 8191821: Finer granularity for GC verification

[sangheon.kim]

Hi Stefan,

On 11/29/2017 03:10 AM, Stefan Johansson wrote:
> 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/
webrev.01 looks good to me but as you published webrev.02, I will check 
that as well.

My answers are in-lined.

>
> 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.
You are right.
I was confused with the ccstrlist length limitation. Sorry for the noise.

>>
>> 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.
I'm okay with as is since all other same type command-line options don't 
check its length as well.

One thing that I thought is that if we have enum-string pair of all 
available VerifyGCType, we can use them for printing logs(e.g. when 
printing warning of all available types) and sum of those strings' 
length would be the maximum length of the new option.

Thanks,
Sangheon

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