RFR (XS): 8059527: Disallow ParallelGCThreads=0 for G1

[Erik Helin]

On 2014-10-02 09:22, Marcus Larsson wrote:
> 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/

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

> Regards,
> Marcus
>
> On 01/10/14 16:48, Erik Helin wrote:
>> 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
>>
>> On 2014-10-01 15:22, Marcus Larsson wrote:
>>> 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