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

[David Holmes]

On 12/03/2016 1:44 AM, Jon Masamitsu wrote:
>
>
> On 3/10/2016 12:42 PM, Kim Barrett wrote:
>>> 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.
>
> I retract this suggestion to delete 1675-1676.  I'll 99.99% sure that it
> would be
> OK, but argument processing being what it is and this being an update fix,
> leave those lines in.  I've been surprised before.

Okay on being conservative here. But I repeat my earlier statement:

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

David
-----

>> 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.
>
> I agree although I'll also respond to Thomas next.
>
> Jon
>
>>
>>
>>
>