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

[Jon Masamitsu]

On 03/14/2016 08:16 AM, Thomas Schatzl wrote:
> Hi Jon,
>
> On Fri, 2016-03-11 at 07:59 -0800, Jon Masamitsu wrote:
>> On 3/10/2016 12:46 PM, Thomas Schatzl wrote:
>>> Hi,
>>>
>>> On Thu, 2016-03-10 at 15:42 -0500, Kim Barrett wrote:
>>>>> On Mar 10, 2016, at 12:21 PM, Jon Masamitsu <
>>>>> jon.masamitsu at oracle.com> wrote:
>>>>>
>>>>>
>>>>>
>>> [...]
>>>> 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,
>>> just some historical note: in 8u, ParallelGCThreads == 0 means "use
>>> the
>>> VM thread, single threaded", and ParallelGCThreads == 1 means use a
>>> single worker thread.
>> By "VM thread, single threaded" I believe you mean that we used the
>> serial version (no threading code) and let the VM execute it.  I
>> recall this being the case with some ParNew  code.
> I am not too well-versed into parnew, but actually we used the same
> code. We could do that because we encapsulated parallel work into these
> AbstractGangTasks.
>
> The code typically looks like the following:
>
> SomeTask cl;
> if (ParallelGCThreads != 0) {
>    workers()->run_task(&cl);
> } else {
>    cl.do_work(0);
> }
>
> (Actually sometimes there is a copy&paste in the else-path...)
>
>>> In jdk9 we removed that option to use the vm thread in a single
>>> threaded way.
>>>
>>> We did not backport that change because of potential performance
>>> impact
>>> (whatever that might be).
>> This can be fixed while leaving the ParallelGCThreads == 0 as an
>> option for G1.
>> In the 8u it is no longer an option for ParNew  nor ParallelGC (both
>> prohibit
>> ParallelGCThreads == 0 in the argument processing).
>>
>> How about we make G1 like the other collectors in 8u and prohibit
>> ParallelGCThreads == 0.   That would be the simplest and most
>> consistent thing to do (in my opinion).
> Fine with me. What's your suggestion on how to achieve this? Backport
> the jdk9 change that removed that, or hack it in for jdk8?

Hack it into jdk8.  The latest change that I've seen that will do that is in
the argument processing for G1.

--- a/src/share/vm/runtime/arguments.cpp        Wed Mar 09 19:51:23 2016 +0300
+++ b/src/share/vm/runtime/arguments.cpp        Mon Mar 14 13:49:33 2016 +0000
@@ -1675,9 +1675,8 @@
    FLAG_SET_DEFAULT(ParallelGCThreads,
                       Abstract_VM_Version::parallel_worker_threads());
    if (ParallelGCThreads == 0) {
-    FLAG_SET_DEFAULT(ParallelGCThreads,
-                     Abstract_VM_Version::parallel_worker_threads());
-  }
+    vm_exit_during_initialization("The flag -XX:+UseG1GC can not be combined with -XX:ParallelGCThreads=0", NULL);
+   }

Jon


>
> Thanks,
>    Thomas