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

[Kim Barrett]

> On Mar 10, 2016, at 12:21 PM, Jon Masamitsu <jon.masamitsu at oracle.com> wrote:
> 
> 
> 
> 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.

The retry setting to parallel_worker_threads() again code dates back to the initial G1 checkin.  Hard to know what was intended.

The current jdk9 code does not have that: it looks like

  FLAG_SET_DEFAULT(ParallelGCThreads, Abstract_VM_Version::parallel_worker_threads());
  if (ParallelGCThreads == 0) {
    assert(!FLAG_IS_DEFAULT(ParallelGCThreads), "The default value for ParallelGCThreads should not be 0.");
    vm_exit_during_initialization("The flag -XX:+UseG1GC can not be combined with -XX:ParallelGCThreads=0", NULL);
  }

This proposed change is for jdk8u-something, correct?

In jdk9 G1 does not support ParallelGCThreads == 0 at all, as can be seen above.  Making use of that decision, a cleanup pass was made that eliminated a whole bunch of “if (ParallelGCThreads == 0) then pretend it’s 1 or do some other special thing” code.  The backport of 8017462 to 8u72 (i.e. 8149347) looks like it might not have taken that into account.  For example,

http://hg.openjdk.java.net/jdk8u/jdk8u-dev/hotspot/rev/6c57a16d0238
--- a/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp	Wed Feb 17 13:42:03 2016 +0000
+++ b/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp	Thu Feb 18 20:30:45 2016 +0000
@@ -3991,8 +3991,15 @@
 
     TraceCPUTime tcpu(G1Log::finer(), true, gclog_or_tty);
 
-    uint active_workers = (G1CollectedHeap::use_parallel_gc_threads() ?
-                                workers()->active_workers() : 1);
+    uint active_workers = AdaptiveSizePolicy::calc_active_workers(workers()->total_workers(),
+                                                                  workers()->active_workers(),
+                                                                  Threads::number_of_non_daemon_threads());

Note the conditionalization on use_parallel_gc_threads().

It might be that the simplest thing to do for 8u backporting is indeed to backport prevention of ParallelGCThreads == 0, as suggested in the proposed patch.