RFR: 8150518: G1 GC crashes at G1CollectedHeap::do_collection_pause_at_safepoint(double)
Jon Masamitsu
jon.masamitsu at oracle.com
Tue Mar 15 22:23:41 UTC 2016
On 3/15/2016 6:37 AM, Fairoz Matte wrote:
> Hi Jon, David, Sangheon, Kim and Thomas,
>
> Thanks for your review comments.
> Patch is modified to incorporate the changes
> @@ -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);
> + }
>
> Find below is the details
> Bugid - https://bugs.openjdk.java.net/browse/JDK-8150518
> Webrev - http://cr.openjdk.java.net/~rpatil/8150518/webrev.01/
Looks good.
Jon
>
> Thanks,
> Fairoz
>
>> -----Original Message-----
>> From: Jon Masamitsu
>> Sent: Tuesday, March 15, 2016 12:11 AM
>> To: David Holmes; sangheon; Kim Barrett
>> Cc: hotspot-runtime-dev at openjdk.java.net
>> Subject: Re: RFR: 8150518: G1 GC crashes at
>> G1CollectedHeap::do_collection_pause_at_safepoint(double)
>>
>>
>>
>> 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/6c57a16d023
>>>>>>> 8
>>>>>>> --- 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