From 4a9f8efa867f84463f054d6624bcc5a89033e152 Mon Sep 17 00:00:00 2001 From: Ivan Walulya Date: Mon, 24 Apr 2023 08:47:23 +0000 Subject: [PATCH] 8057586: Explicit GC ignored if GCLocker is active Reviewed-by: tschatzl, ayang --- src/hotspot/share/gc/g1/g1CollectedHeap.cpp | 35 ++- src/hotspot/share/gc/g1/g1CollectedHeap.hpp | 3 + .../gc/parallel/parallelScavengeHeap.cpp | 22 +- .../gc/parallel/parallelScavengeHeap.hpp | 2 +- .../parallel/parallelScavengeHeap.inline.hpp | 6 +- .../share/gc/parallel/psParallelCompact.cpp | 6 +- .../share/gc/parallel/psParallelCompact.hpp | 2 +- .../share/gc/parallel/psVMOperations.cpp | 7 +- .../share/gc/parallel/psVMOperations.hpp | 4 +- src/hotspot/share/gc/shared/gcCause.hpp | 6 + .../share/gc/shared/genCollectedHeap.cpp | 25 +- .../jtreg/gc/TestJNICriticalStressTest.java | 232 ++++++++++++++++++ 12 files changed, 328 insertions(+), 22 deletions(-) create mode 100644 test/hotspot/jtreg/gc/TestJNICriticalStressTest.java diff --git a/src/hotspot/share/gc/g1/g1CollectedHeap.cpp b/src/hotspot/share/gc/g1/g1CollectedHeap.cpp index f3c811a124e..76df081b2ba 100644 --- a/src/hotspot/share/gc/g1/g1CollectedHeap.cpp +++ b/src/hotspot/share/gc/g1/g1CollectedHeap.cpp @@ -1928,6 +1928,35 @@ bool G1CollectedHeap::try_collect_concurrently(GCCause::Cause cause, } } +bool G1CollectedHeap::try_collect_fullgc(GCCause::Cause cause, + const G1GCCounters& counters_before) { + assert_heap_not_locked(); + + while(true) { + VM_G1CollectFull op(counters_before.total_collections(), + counters_before.total_full_collections(), + cause); + VMThread::execute(&op); + + // Request is trivially finished. + if (!GCCause::is_explicit_full_gc(cause) || op.gc_succeeded()) { + return op.gc_succeeded(); + } + + { + MutexLocker ml(Heap_lock); + if (counters_before.total_full_collections() != total_full_collections()) { + return true; + } + } + + if (GCLocker::is_active_and_needs_gc()) { + // If GCLocker is active, wait until clear before retrying. + GCLocker::stall_until_clear(); + } + } +} + bool G1CollectedHeap::try_collect(GCCause::Cause cause, const G1GCCounters& counters_before) { if (should_do_concurrent_full_gc(cause)) { @@ -1951,11 +1980,7 @@ bool G1CollectedHeap::try_collect(GCCause::Cause cause, return op.gc_succeeded(); } else { // Schedule a Full GC. - VM_G1CollectFull op(counters_before.total_collections(), - counters_before.total_full_collections(), - cause); - VMThread::execute(&op); - return op.gc_succeeded(); + return try_collect_fullgc(cause, counters_before); } } diff --git a/src/hotspot/share/gc/g1/g1CollectedHeap.hpp b/src/hotspot/share/gc/g1/g1CollectedHeap.hpp index 06df4dd38aa..59d9f69ed2e 100644 --- a/src/hotspot/share/gc/g1/g1CollectedHeap.hpp +++ b/src/hotspot/share/gc/g1/g1CollectedHeap.hpp @@ -283,6 +283,9 @@ private: uint gc_counter, uint old_marking_started_before); + bool try_collect_fullgc(GCCause::Cause cause, + const G1GCCounters& counters_before); + // indicates whether we are in young or mixed GC mode G1CollectorState _collector_state; diff --git a/src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp b/src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp index 8d665d6bc60..a2db91ffc96 100644 --- a/src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp +++ b/src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp @@ -549,8 +549,26 @@ void ParallelScavengeHeap::collect(GCCause::Cause cause) { return; } - VM_ParallelGCSystemGC op(gc_count, full_gc_count, cause); - VMThread::execute(&op); + while (true) { + VM_ParallelGCSystemGC op(gc_count, full_gc_count, cause); + VMThread::execute(&op); + + if (!GCCause::is_explicit_full_gc(cause) || op.full_gc_succeeded()) { + return; + } + + { + MutexLocker ml(Heap_lock); + if (full_gc_count != total_full_collections()) { + return; + } + } + + if (GCLocker::is_active_and_needs_gc()) { + // If GCLocker is active, wait until clear before retrying. + GCLocker::stall_until_clear(); + } + } } void ParallelScavengeHeap::object_iterate(ObjectClosure* cl) { diff --git a/src/hotspot/share/gc/parallel/parallelScavengeHeap.hpp b/src/hotspot/share/gc/parallel/parallelScavengeHeap.hpp index ab981c5851f..abf87b0e019 100644 --- a/src/hotspot/share/gc/parallel/parallelScavengeHeap.hpp +++ b/src/hotspot/share/gc/parallel/parallelScavengeHeap.hpp @@ -211,7 +211,7 @@ class ParallelScavengeHeap : public CollectedHeap { // will then attempt a full gc. The second collects the entire heap; if // maximum_compaction is true, it will compact everything and clear all soft // references. - inline void invoke_scavenge(); + inline bool invoke_scavenge(); // Perform a full collection void do_full_collection(bool clear_all_soft_refs) override; diff --git a/src/hotspot/share/gc/parallel/parallelScavengeHeap.inline.hpp b/src/hotspot/share/gc/parallel/parallelScavengeHeap.inline.hpp index e852c11265f..ef5f6dedd29 100644 --- a/src/hotspot/share/gc/parallel/parallelScavengeHeap.inline.hpp +++ b/src/hotspot/share/gc/parallel/parallelScavengeHeap.inline.hpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2006, 2022, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2006, 2023, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -35,8 +35,8 @@ inline bool ParallelScavengeHeap::should_alloc_in_eden(const size_t size) const return size < eden_size / 2; } -inline void ParallelScavengeHeap::invoke_scavenge() { - PSScavenge::invoke(); +inline bool ParallelScavengeHeap::invoke_scavenge() { + return PSScavenge::invoke(); } inline bool ParallelScavengeHeap::is_in_young(const void* p) const { diff --git a/src/hotspot/share/gc/parallel/psParallelCompact.cpp b/src/hotspot/share/gc/parallel/psParallelCompact.cpp index 7130cb7880a..9c0febe90a4 100644 --- a/src/hotspot/share/gc/parallel/psParallelCompact.cpp +++ b/src/hotspot/share/gc/parallel/psParallelCompact.cpp @@ -1678,7 +1678,7 @@ void PSParallelCompact::summary_phase(bool maximum_compaction) // may be true because this method can be called without intervening // activity. For example when the heap space is tight and full measure // are being taken to free space. -void PSParallelCompact::invoke(bool maximum_heap_compaction) { +bool PSParallelCompact::invoke(bool maximum_heap_compaction) { assert(SafepointSynchronize::is_at_safepoint(), "should be at safepoint"); assert(Thread::current() == (Thread*)VMThread::vm_thread(), "should be in vm thread"); @@ -1695,8 +1695,8 @@ void PSParallelCompact::invoke(bool maximum_heap_compaction) { const bool clear_all_soft_refs = heap->soft_ref_policy()->should_clear_all_soft_refs(); - PSParallelCompact::invoke_no_policy(clear_all_soft_refs || - maximum_heap_compaction); + return PSParallelCompact::invoke_no_policy(clear_all_soft_refs || + maximum_heap_compaction); } // This method contains no policy. You should probably diff --git a/src/hotspot/share/gc/parallel/psParallelCompact.hpp b/src/hotspot/share/gc/parallel/psParallelCompact.hpp index 194dc70ad0a..07c420bc0dd 100644 --- a/src/hotspot/share/gc/parallel/psParallelCompact.hpp +++ b/src/hotspot/share/gc/parallel/psParallelCompact.hpp @@ -1141,7 +1141,7 @@ class PSParallelCompact : AllStatic { PSParallelCompact(); - static void invoke(bool maximum_heap_compaction); + static bool invoke(bool maximum_heap_compaction); static bool invoke_no_policy(bool maximum_heap_compaction); static void post_initialize(); diff --git a/src/hotspot/share/gc/parallel/psVMOperations.cpp b/src/hotspot/share/gc/parallel/psVMOperations.cpp index a9f99a4dbf5..47eeffb34a5 100644 --- a/src/hotspot/share/gc/parallel/psVMOperations.cpp +++ b/src/hotspot/share/gc/parallel/psVMOperations.cpp @@ -58,7 +58,8 @@ static bool is_cause_full(GCCause::Cause cause) { VM_ParallelGCSystemGC::VM_ParallelGCSystemGC(uint gc_count, uint full_gc_count, GCCause::Cause gc_cause) : - VM_GC_Operation(gc_count, gc_cause, full_gc_count, is_cause_full(gc_cause)) + VM_GC_Operation(gc_count, gc_cause, full_gc_count, is_cause_full(gc_cause)), + _full_gc_succeeded(false) { } @@ -70,8 +71,8 @@ void VM_ParallelGCSystemGC::doit() { GCCauseSetter gccs(heap, _gc_cause); if (!_full) { // If (and only if) the scavenge fails, this will invoke a full gc. - heap->invoke_scavenge(); + _full_gc_succeeded = heap->invoke_scavenge(); } else { - heap->do_full_collection(false); + _full_gc_succeeded = PSParallelCompact::invoke(false); } } diff --git a/src/hotspot/share/gc/parallel/psVMOperations.hpp b/src/hotspot/share/gc/parallel/psVMOperations.hpp index 0cddac3d616..cc49eb631c7 100644 --- a/src/hotspot/share/gc/parallel/psVMOperations.hpp +++ b/src/hotspot/share/gc/parallel/psVMOperations.hpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2007, 2019, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2007, 2023, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -40,10 +40,12 @@ class VM_ParallelGCFailedAllocation : public VM_CollectForAllocation { }; class VM_ParallelGCSystemGC: public VM_GC_Operation { + bool _full_gc_succeeded; public: VM_ParallelGCSystemGC(uint gc_count, uint full_gc_count, GCCause::Cause gc_cause); virtual VMOp_Type type() const { return VMOp_ParallelGCSystemGC; } virtual void doit(); + bool full_gc_succeeded() const { return _full_gc_succeeded; } }; #endif // SHARE_GC_PARALLEL_PSVMOPERATIONS_HPP diff --git a/src/hotspot/share/gc/shared/gcCause.hpp b/src/hotspot/share/gc/shared/gcCause.hpp index bf794fad9f5..152ca787fc2 100644 --- a/src/hotspot/share/gc/shared/gcCause.hpp +++ b/src/hotspot/share/gc/shared/gcCause.hpp @@ -95,6 +95,12 @@ class GCCause : public AllStatic { cause == GCCause::_dcmd_gc_run); } + inline static bool is_explicit_full_gc(GCCause::Cause cause) { + return (is_user_requested_gc(cause) || + is_serviceability_requested_gc(cause) || + cause == GCCause::_wb_full_gc); + } + inline static bool is_serviceability_requested_gc(GCCause::Cause cause) { return (cause == GCCause::_jvmti_force_gc || diff --git a/src/hotspot/share/gc/shared/genCollectedHeap.cpp b/src/hotspot/share/gc/shared/genCollectedHeap.cpp index 56cc45e2e80..8a66487e2bd 100644 --- a/src/hotspot/share/gc/shared/genCollectedHeap.cpp +++ b/src/hotspot/share/gc/shared/genCollectedHeap.cpp @@ -796,9 +796,28 @@ void GenCollectedHeap::collect(GCCause::Cause cause) { ? YoungGen : OldGen; - VM_GenCollectFull op(gc_count_before, full_gc_count_before, - cause, max_generation); - VMThread::execute(&op); + while (true) { + VM_GenCollectFull op(gc_count_before, full_gc_count_before, + cause, max_generation); + VMThread::execute(&op); + + if (!GCCause::is_explicit_full_gc(cause)) { + return; + } + + { + MutexLocker ml(Heap_lock); + // Read the GC count while holding the Heap_lock + if (full_gc_count_before != total_full_collections()) { + return; + } + } + + if (GCLocker::is_active_and_needs_gc()) { + // If GCLocker is active, wait until clear before retrying. + GCLocker::stall_until_clear(); + } + } } void GenCollectedHeap::do_full_collection(bool clear_all_soft_refs) { diff --git a/test/hotspot/jtreg/gc/TestJNICriticalStressTest.java b/test/hotspot/jtreg/gc/TestJNICriticalStressTest.java new file mode 100644 index 00000000000..999435a0bbf --- /dev/null +++ b/test/hotspot/jtreg/gc/TestJNICriticalStressTest.java @@ -0,0 +1,232 @@ +/* + * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +/* + * @test G1 Full GC execution when JNICritical is active + * @summary Check that Full GC calls are not ignored if concurrent with an active GCLocker. + * @bug 8057586 + * @requires vm.gc.G1 + * @modules java.base/jdk.internal.misc + * @library /test/lib + * @build jdk.test.whitebox.WhiteBox + * @run driver jdk.test.lib.helpers.ClassFileInstaller jdk.test.whitebox.WhiteBox + * @run main/othervm -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -Xms3g -Xmx3g -Xmn2g -Xlog:gc TestJNICriticalStressTest 30 4 4 G1 + */ + + /* + * @test Parallel Full GC execution when JNICritical is active + * @bug 8057586 + * @requires vm.gc.Parallel + * @modules java.base/jdk.internal.misc + * @library /test/lib + * @build jdk.test.whitebox.WhiteBox + * @run driver jdk.test.lib.helpers.ClassFileInstaller jdk.test.whitebox.WhiteBox + * @run main/othervm -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -XX:+UseParallelGC -Xms3g -Xmx3g -Xmn2g -Xlog:gc TestJNICriticalStressTest 30 4 4 Parallel + */ + +/* + * @test Serial Full GC execution when JNICritical is active + * @bug 8057586 + * @requires vm.gc.Serial + * @modules java.base/jdk.internal.misc + * @library /test/lib + * @build jdk.test.whitebox.WhiteBox + * @run driver jdk.test.lib.helpers.ClassFileInstaller jdk.test.whitebox.WhiteBox + * @run main/othervm -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -XX:+UseSerialGC -Xms3g -Xmx3g -Xmn2g -Xlog:gc TestJNICriticalStressTest 30 4 4 Serial + */ + +import jdk.test.lib.Asserts; +import jdk.test.whitebox.WhiteBox; + +import java.lang.management.GarbageCollectorMXBean; +import java.lang.management.ManagementFactory; +import java.util.List; +import java.util.HashMap; +import java.util.Map; +import java.util.zip.Deflater; + +/** + * Test verifies that Full GC calls are not ignored if concurrent with an active GCLocker. + * + * The test checks that at least a full gc is executed in the duration of a WhiteBox.fullGC() call; + * either by the calling thread or a concurrent thread. + */ +public class TestJNICriticalStressTest { + private static final WhiteBox wb = WhiteBox.getWhiteBox(); + + static private final int LARGE_MAP_SIZE = 64 * 1024; + + static private final int MAP_ARRAY_LENGTH = 4; + static private final int MAP_SIZE = 1024; + + static private final int BYTE_ARRAY_LENGTH = 16 * 1024; + + static private final long SYSTEM_GC_PERIOD_MS = 5 * 1000; + static private long gcCountBefore = 0; + static private GarbageCollectorMXBean collector = null; + + static private void println(String str) { System.out.println(str); } + static private void exit(int code) { System.exit(code); } + + static Map populateMap(int size) { + Map map = new HashMap(); + for (int i = 0; i < size; i += 1) { + Integer keyInt = Integer.valueOf(i); + String valStr = "value is [" + i + "]"; + map.put(keyInt,valStr); + } + return map; + } + + static private class AllocatingWorker implements Runnable { + private final Object[] array = new Object[MAP_ARRAY_LENGTH]; + private int arrayIndex = 0; + + private void doStep() { + Map map = populateMap(MAP_SIZE); + array[arrayIndex] = map; + arrayIndex = (arrayIndex + 1) % MAP_ARRAY_LENGTH; + } + + public void run() { + while (true) { + doStep(); + } + } + } + + static private class JNICriticalWorker implements Runnable { + private int count; + + private void doStep() { + byte[] inputArray = new byte[BYTE_ARRAY_LENGTH]; + for (int i = 0; i < inputArray.length; i += 1) { + inputArray[i] = (byte) (count + i); + } + + Deflater deflater = new Deflater(); + deflater.setInput(inputArray); + deflater.finish(); + + byte[] outputArray = new byte[2 * inputArray.length]; + deflater.deflate(outputArray); + deflater.end(); + + count += 1; + } + + public void run() { + while (true) { + doStep(); + } + } + } + + static private class SystemGCWorker implements Runnable { + public void run() { + long fullGcCounts = 0; + while (true) { + try { + Thread.sleep(SYSTEM_GC_PERIOD_MS); + } catch (InterruptedException e) { + e.printStackTrace(); + return; + } + + long gcCountBefore = collector.getCollectionCount(); + wb.fullGC(); + long gcCountAfter = collector.getCollectionCount(); + + Asserts.assertLessThan(gcCountBefore, gcCountAfter, "Triggered more Full GCs than expected"); + } + } + } + + static public Map largeMap; + + public static void main(String... args) throws Exception { + if (args.length < 4) { + println("usage: JNICriticalStressTest "); + exit(-1); + } + + long durationSec = Long.parseLong(args[0]); + int allocThreadNum = Integer.parseInt(args[1]); + int jniCriticalThreadNum = Integer.parseInt(args[2]); + + StringBuilder OldGCName = new StringBuilder(); + switch (args[3]) { + case "G1": + OldGCName.append("G1 Old Generation"); + break; + case "Parallel": + OldGCName.append("PS MarkSweep"); + break; + case "Serial": + OldGCName.append("MarkSweepCompact"); + break; + default: + throw new RuntimeException("Unsupported GC selected"); + } + + List collectors = ManagementFactory.getGarbageCollectorMXBeans(); + + for (int i = 0; i < collectors.size(); i++) { + if (collectors.get(i).getName().contains(OldGCName.toString())) { + collector = collectors.get(i); + break; + } + } + + if (collector == null) { + throw new RuntimeException(OldGCName.toString() + " not found"); + } + + println("Running for " + durationSec + " secs"); + + largeMap = populateMap(LARGE_MAP_SIZE); + + // Start threads to allocate memory, this will trigger both GCLocker initiated + // garbage collections (GCs) and regular GCs. Thus increasing the likelihood of + // having different types of GCs happening concurrently with the System.gc call. + println("Starting " + allocThreadNum + " allocating threads"); + for (int i = 0; i < allocThreadNum; i += 1) { + new Thread(new AllocatingWorker()).start(); + } + + println("Starting " + jniCriticalThreadNum + " jni critical threads"); + for (int i = 0; i < jniCriticalThreadNum; i += 1) { + new Thread(new JNICriticalWorker()).start(); + } + + new Thread(new SystemGCWorker()).start(); + + long durationMS = (long) (1000 * durationSec); + try { + Thread.sleep(durationMS); + } catch (InterruptedException e) { + e.printStackTrace(); + exit(-1); + } + } +}