RFR: 8150518: G1 GC crashes at G1CollectedHeap::do_collection_pause_at_safepoint(double)
Fairoz Matte
fairoz.matte at oracle.com
Tue Mar 15 13:37:40 UTC 2016
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/
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