RFR: 8150518: G1 GC crashes at G1CollectedHeap::do_collection_pause_at_safepoint(double)

[Jon Masamitsu]

On 03/09/2016 08:53 PM, David Holmes wrote:
> On 9/03/2016 9:33 PM, Fairoz Matte wrote:
>> Background:
>>
>> After the backport of 
>> https://bugs.openjdk.java.net/browse/JDK-8017462, The flag 
>> -XX:+UseG1GC combined with -XX:ParallelGCThreads=0 makes the _workers 
>> to null in 8u.
>>
>> As there is no condition to handle such scenario in 
>> share/vm/memory/sharedHeap.cpp, which causes the crash.
>>
>> The similar condition is already implemented for following scenarios
>>
>> 1.       -XX:+UseParallelGC -XX:ParallelGCThreads=0
>>
>> 2.       -XX:+UseParNewGC -XX:ParallelGCThreads=0
>>
>> 3.       -XX:+UseConcMarkSweepGC -XX:ParallelGCThreads=0
>>
>>
>>
>> Fix:
>>
>> Condition check is added in src/share/vm/runtime/arguments.cpp file 
>> to verify "-XX:+UseG1GC -XX:ParallelGCThreads=0"
>>
>> Thanks for the base patch from Jon.
>>
>> Due to this patch it makes some of the test cases absolute. They have 
>> been removed.
>>
>>
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8150518
>>
>> Webrev:  http://cr.openjdk.java.net/~rpatil/8150518/webrev.00/
>
> This existing code looks wrong:
>
> 1675   FLAG_SET_DEFAULT(ParallelGCThreads,
> 1676 Abstract_VM_Version::parallel_worker_threads());
> 1677   if (ParallelGCThreads == 0) {
> 1678     FLAG_SET_DEFAULT(ParallelGCThreads,
> 1679 Abstract_VM_Version::parallel_worker_threads());
>
> Line 1678 seems to do the same as 1675 - is 
> Abstract_VM_Version::parallel_worker_threads() somehow expected to 
> return a different value on the second call ??

No, Abstract_VM_Version::parallel_worker_threads() won't return a different
value for a second call.  It's harmless but would be cleaner deleting 
1675,1676.

Jon
>
> That aside I'm confused by the fix as we have:
>
> ./share/vm/runtime/globals.hpp:  product(uintx, ParallelGCThreads, 0,
>
> so it seems odd to report an error for setting a value that is already 
> the default ??
>
> Thanks,
> David