RFR: 8150518: G1 GC crashes at G1CollectedHeap::do_collection_pause_at_safepoint(double)
sangheon
sangheon.kim at oracle.com
Mon Mar 14 07:10:18 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?
It is hard to say. :)
I was explaining how the related codes are working.
I think if we need to pick some value to mean that it will be decided
ergonomically, using '0' would be consistent for all flags. But it would
be better to add that explanation on its description.
Thanks,
Sangheon
> 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
>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>
More information about the hotspot-runtime-dev
mailing list