RFR: 8191821: Finer granularity for GC verification

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8191821: Finer granularity for GC verification

Stefan Johansson
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.

Thanks,
Stefan
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191821: Finer granularity for GC verification

Poonam Parhar
Hello Stefan,

The changes look good! Thanks for implementing this enhancement.

One observation:
gcArguments.cpp: if we use VerifyGCType  with the collectors that currently don't support it, passing it multiple comma separated strings, then parse_verification_type() will print this warning message "VerifyGCType is not supported by this collector." for all the strings. It would be better to break out from the while loop in post_heap_initialize() if the collector does not support this option.

Thanks,
Poonam

On 11/28/2017 8: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.

Thanks,
Stefan

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191821: Finer granularity for GC verification

sangheon.kim@oracle.com
In reply to this post by Stefan Johansson
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

Reply | Threaded
Open this post in threaded view
|

Re: 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
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191821: Finer granularity for GC verification

Thomas Schatzl
In reply to this post by Stefan Johansson
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.

  - please change G1VerifyYoung to G1VerifyYoungOnly. Mixed gcs are
also young gcs. We should be exact in our internal naming.

  - in g1HeapVerifier.hpp consider aligning the flag values below each
other.

  - 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.

  - 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?

  - gcArguments.hpp:52: maybe some documentation what this is supposed
to do, and that the accepted types are GC specific.

  - 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?)

  - 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.

Thanks,
  Thomas
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191821: Finer granularity for GC verification

Stefan Johansson
In reply to this post by Poonam Parhar
Thanks for the review Poonam,

Update webrevs with your and Sangheons comments:
Inc:  http://cr.openjdk.java.net/~sjohanss/8191821/00-01/
Full: http://cr.openjdk.java.net/~sjohanss/8191821/01/

Thanks,
Stefan

On 2017-11-28 23:17, Poonam Parhar wrote:
Hello Stefan,

The changes look good! Thanks for implementing this enhancement.

One observation:
gcArguments.cpp: if we use VerifyGCType  with the collectors that currently don't support it, passing it multiple comma separated strings, then parse_verification_type() will print this warning message "VerifyGCType is not supported by this collector." for all the strings. It would be better to break out from the while loop in post_heap_initialize() if the collector does not support this option.

Thanks,
Poonam

On 11/28/2017 8: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.

Thanks,
Stefan


Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191821: Finer granularity for GC verification

Stefan Johansson
In reply to this post by Thomas Schatzl
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/

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

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191821: Finer granularity for GC verification

sangheon.kim@oracle.com
In reply to this post by Stefan Johansson
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
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191821: Finer granularity for GC verification

Thomas Schatzl
In reply to this post by Stefan Johansson
Hi,

On Wed, 2017-11-29 at 16:43 +0100, 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/
>
> [...]
> >

Thanks for all your changes.

Two minor issues:

- in TestVerifyGCType.java, at the end there is a typo:
s/differnt/different

- please document the return type of
GCArguments::parse_verification_type. It's non-obvious imho.

Looks good otherwise. I do not need a re-review for these two nits
above.

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191821: Finer granularity for GC verification

sangheon.kim@oracle.com
In reply to this post by Stefan Johansson
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
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191821: Finer granularity for GC verification

Per Liden
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
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191821: Finer granularity for GC verification

Per Liden
I realize I looked at the wrong version of the patch, so the G1 specific
stuff was removed. But still, I find it unfortunate that gcArguments is
involved here and that we now have yet another post_initialize function.

/Per


On 2017-12-05 08:48, Per Liden wrote:

> 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
>>>
>>