Discussion:
RFR (XS): 8059527: Disallow ParallelGCThreads=0 for G1
Marcus Larsson
2014-10-01 13:22:43 UTC
Permalink
Hi,

Can I have reviews for the following small patch disallowing
ParallelGCThreads to be 0 when G1 is used? Also included a small test to
verify that the VM will not start if the count is 0 for parallel
collectors (G1, CMS+ParNew and ParallelGC).

Webrev:
http://cr.openjdk.java.net/~mlarsson/8059527/webrev.00/

Bug:
https://bugs.openjdk.java.net/browse/JDK-8059527

Testing:
jprt+jtreg

Thanks,
Marcus
Bengt Rutisson
2014-10-01 13:33:13 UTC
Permalink
Hi Marcus,
Post by Marcus Larsson
Hi,
Can I have reviews for the following small patch disallowing
ParallelGCThreads to be 0 when G1 is used? Also included a small test
to verify that the VM will not start if the count is 0 for parallel
collectors (G1, CMS+ParNew and ParallelGC).
http://cr.openjdk.java.net/~mlarsson/8059527/webrev.00/
Looks good to me.

Bengt
Post by Marcus Larsson
https://bugs.openjdk.java.net/browse/JDK-8059527
jprt+jtreg
Thanks,
Marcus
Erik Helin
2014-10-01 14:48:54 UTC
Permalink
Hi Marcus,

a couple of comments:

- src/share/vm/runtime/arguments.cpp:
Could you use vm_exit_during_initialization instead of vm_exit?
This way, the error message will be printed in a uniform way and the
VM will also run all shutdown functions (such as os::shutdown,
os::abort etc).

- test/gc/arguments/TestParallelGCThreads.java
+ * @build TestParallelGCThreads FlagsValue
I don't believe this line is needed, jtreg should automically build
your files

+ * @run main/othervm TestParallelGCThreads
Please use `@run driver` instead of `@run main/othervm`. You can use
'driver' because your tests creates its own processes.

Please use assertions from the testlibrary instead of throwing a
RuntimeException.

- Please update hotspot/test/TEST.groups to include your new test for
needs_g1gc, needs_parallelgc and needs_cmsgc

Thanks,
Erik
Post by Marcus Larsson
Hi,
Can I have reviews for the following small patch disallowing
ParallelGCThreads to be 0 when G1 is used? Also included a small test to
verify that the VM will not start if the count is 0 for parallel
collectors (G1, CMS+ParNew and ParallelGC).
http://cr.openjdk.java.net/~mlarsson/8059527/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8059527
jprt+jtreg
Thanks,
Marcus
Marcus Larsson
2014-10-02 07:22:43 UTC
Permalink
Hi Erik,

Thank you for the input.

New webrev:
http://cr.openjdk.java.net/~mlarsson/8059527/webrev.01/

Incremental:
http://cr.openjdk.java.net/~mlarsson/8059527/webrev.00-01/

Regards,
Marcus
Post by Bengt Rutisson
Hi Marcus,
Could you use vm_exit_during_initialization instead of vm_exit?
This way, the error message will be printed in a uniform way and the
VM will also run all shutdown functions (such as os::shutdown,
os::abort etc).
- test/gc/arguments/TestParallelGCThreads.java
I don't believe this line is needed, jtreg should automically build
your files
'driver' because your tests creates its own processes.
Please use assertions from the testlibrary instead of throwing a
RuntimeException.
- Please update hotspot/test/TEST.groups to include your new test for
needs_g1gc, needs_parallelgc and needs_cmsgc
Thanks,
Erik
Post by Marcus Larsson
Hi,
Can I have reviews for the following small patch disallowing
ParallelGCThreads to be 0 when G1 is used? Also included a small test to
verify that the VM will not start if the count is 0 for parallel
collectors (G1, CMS+ParNew and ParallelGC).
http://cr.openjdk.java.net/~mlarsson/8059527/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8059527
jprt+jtreg
Thanks,
Marcus
Bengt Rutisson
2014-10-02 07:32:27 UTC
Permalink
Post by Marcus Larsson
Hi Erik,
Thank you for the input.
http://cr.openjdk.java.net/~mlarsson/8059527/webrev.01/
http://cr.openjdk.java.net/~mlarsson/8059527/webrev.00-01/
Looks even better to me.

Bengt
Post by Marcus Larsson
Regards,
Marcus
Post by Bengt Rutisson
Hi Marcus,
Could you use vm_exit_during_initialization instead of vm_exit?
This way, the error message will be printed in a uniform way and the
VM will also run all shutdown functions (such as os::shutdown,
os::abort etc).
- test/gc/arguments/TestParallelGCThreads.java
I don't believe this line is needed, jtreg should automically build
your files
'driver' because your tests creates its own processes.
Please use assertions from the testlibrary instead of throwing a
RuntimeException.
- Please update hotspot/test/TEST.groups to include your new test for
needs_g1gc, needs_parallelgc and needs_cmsgc
Thanks,
Erik
Post by Marcus Larsson
Hi,
Can I have reviews for the following small patch disallowing
ParallelGCThreads to be 0 when G1 is used? Also included a small test to
verify that the VM will not start if the count is 0 for parallel
collectors (G1, CMS+ParNew and ParallelGC).
http://cr.openjdk.java.net/~mlarsson/8059527/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8059527
jprt+jtreg
Thanks,
Marcus
Erik Helin
2014-10-02 08:53:45 UTC
Permalink
Post by Marcus Larsson
Hi Erik,
Thank you for the input.
http://cr.openjdk.java.net/~mlarsson/8059527/webrev.01/
http://cr.openjdk.java.net/~mlarsson/8059527/webrev.00-01/
Changes look good, just a small comment on the message passed to
vm_exit_during_initialization: instead of writing "The G1 GC", could you
write "The flag -XX:+UseG1GC"? This will make it even easier for the
user to understand what is causing the conflict.

Thanks,
Erik
Post by Marcus Larsson
Regards,
Marcus
Post by Bengt Rutisson
Hi Marcus,
Could you use vm_exit_during_initialization instead of vm_exit?
This way, the error message will be printed in a uniform way and the
VM will also run all shutdown functions (such as os::shutdown,
os::abort etc).
- test/gc/arguments/TestParallelGCThreads.java
I don't believe this line is needed, jtreg should automically build
your files
'driver' because your tests creates its own processes.
Please use assertions from the testlibrary instead of throwing a
RuntimeException.
- Please update hotspot/test/TEST.groups to include your new test for
needs_g1gc, needs_parallelgc and needs_cmsgc
Thanks,
Erik
Post by Marcus Larsson
Hi,
Can I have reviews for the following small patch disallowing
ParallelGCThreads to be 0 when G1 is used? Also included a small test to
verify that the VM will not start if the count is 0 for parallel
collectors (G1, CMS+ParNew and ParallelGC).
http://cr.openjdk.java.net/~mlarsson/8059527/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8059527
jprt+jtreg
Thanks,
Marcus
Marcus Larsson
2014-10-02 09:17:33 UTC
Permalink
Post by Erik Helin
Post by Marcus Larsson
Hi Erik,
Thank you for the input.
http://cr.openjdk.java.net/~mlarsson/8059527/webrev.01/
http://cr.openjdk.java.net/~mlarsson/8059527/webrev.00-01/
Changes look good, just a small comment on the message passed to
vm_exit_during_initialization: instead of writing "The G1 GC", could you
write "The flag -XX:+UseG1GC"? This will make it even easier for the
user to understand what is causing the conflict.
Will fix that before pushing. Thanks for reviewing!

Marcus
Post by Erik Helin
Thanks,
Erik
Post by Marcus Larsson
Regards,
Marcus
Post by Bengt Rutisson
Hi Marcus,
Could you use vm_exit_during_initialization instead of vm_exit?
This way, the error message will be printed in a uniform way and the
VM will also run all shutdown functions (such as os::shutdown,
os::abort etc).
- test/gc/arguments/TestParallelGCThreads.java
I don't believe this line is needed, jtreg should automically build
your files
'driver' because your tests creates its own processes.
Please use assertions from the testlibrary instead of throwing a
RuntimeException.
- Please update hotspot/test/TEST.groups to include your new test for
needs_g1gc, needs_parallelgc and needs_cmsgc
Thanks,
Erik
Post by Marcus Larsson
Hi,
Can I have reviews for the following small patch disallowing
ParallelGCThreads to be 0 when G1 is used? Also included a small test to
verify that the VM will not start if the count is 0 for parallel
collectors (G1, CMS+ParNew and ParallelGC).
http://cr.openjdk.java.net/~mlarsson/8059527/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8059527
jprt+jtreg
Thanks,
Marcus
Loading...