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

[Fairoz Matte]

Hi David,

That was existing code. The patch contains only 3 line change.

    FLAG_SET_DEFAULT(ParallelGCThreads,
                       Abstract_VM_Version::parallel_worker_threads());
    if (ParallelGCThreads == 0) {
      FLAG_SET_DEFAULT(ParallelGCThreads,
                       Abstract_VM_Version::parallel_worker_threads());
+     if (ParallelGCThreads == 0) {
+       vm_exit_during_initialization("The flag -XX:+UseG1GC can not be combined with -XX:ParallelGCThreads=0", NULL);
      }
+   }

Let me cross check and remove the unnecessary existing code.

Thanks,
Fairoz

-----Original Message-----
From: David Holmes 
Sent: Thursday, March 10, 2016 10:24 AM
To: Fairoz Matte; hotspot-runtime-dev at openjdk.java.net
Cc: Jon Masamitsu
Subject: Re: RFR: 8150518: G1 GC crashes at G1CollectedHeap::do_collection_pause_at_safepoint(double)

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

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