RFR: 8150518: G1 GC crashes at G1CollectedHeap::do_collection_pause_at_safepoint(double)
Jon Masamitsu
jon.masamitsu at oracle.com
Mon Mar 14 18:41:13 UTC 2016
On 03/13/2016 11:51 PM, David Holmes wrote:
>
>
> 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!
It's not a well defined convention. It is something that happened in
one of the
GC's and then was imitated or not in others. If there is a code path for
ParallelGCThreads==0 (that's not calculate me ergonomically), then explicit
setting on the command line of ParallelGCThreads to 0 executes that
code path. That exception has been removed in jdk9.
Jon
>
> 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
>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>
More information about the hotspot-runtime-dev
mailing list