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

[David Holmes]

On 14/03/2016 4:34 PM, sangheon wrote:
> Hi David,
>
> On 03/13/2016 05:49 PM, David Holmes wrote:
>> 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 ??
> A flag's value set by default(FLAG_IS_DEFAULT) is different from set by
> command-line(FLAG_SET_CMDLINE) when we process that flag.
> Usually a flag that has a default value of '0' means 'will be decided
> ergonomically'.

Is this a well-defined convention? I thought that when zero means "use 
the default" (ie ergonomics or whatever) that it always means that 
regardless of whether it is set explicitly or not!

Thanks,
David


>
>  From above example,
> 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());
>
> line1676, parallel_worker_threads() is checking whether the flag is a
> default value or not.
> And use the value if it is not 'set-by-default'.
>
> unsigned int Abstract_VM_Version::parallel_worker_threads() {
>    if (!_parallel_worker_threads_initialized) {
>      if (FLAG_IS_DEFAULT(ParallelGCThreads)) {
>        _parallel_worker_threads =
> VM_Version::calc_parallel_worker_threads();
>      } else {
>        _parallel_worker_threads = ParallelGCThreads;
>      }
>      _parallel_worker_threads_initialized = true;
>    }
>    return _parallel_worker_threads;
> }
>
> Thanks,
> Sangheon
>
>
>>
>> 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
>>>
>>>>
>>>>
>>>>
>>>
>