8048556: Unnecessary GCLocker-initiated young GCs
Summary: Fixed recognition of unnecessary GCLocker collections.
Reviewed-by: pliden, tschatzl
--- a/src/hotspot/share/gc/g1/g1CollectedHeap.cpp Wed Jul 31 13:40:59 2019 -0400
+++ b/src/hotspot/share/gc/g1/g1CollectedHeap.cpp Wed Jul 31 14:28:51 2019 -0400
@@ -2154,6 +2154,12 @@
GCLocker::stall_until_clear();
}
}
+ } else if (GCLocker::should_discard(cause, gc_count_before)) {
+ // Return false to be consistent with VMOp failure due to
+ // another collection slipping in after our gc_count but before
+ // our request is processed. _gc_locker collections upgraded by
+ // GCLockerInvokesConcurrent are handled above and never discarded.
+ return false;
} else {
if (cause == GCCause::_gc_locker || cause == GCCause::_wb_young_gc
DEBUG_ONLY(|| cause == GCCause::_scavenge_alot)) {
--- a/src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp Wed Jul 31 13:40:59 2019 -0400
+++ b/src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp Wed Jul 31 14:28:51 2019 -0400
@@ -520,6 +520,10 @@
full_gc_count = total_full_collections();
}
+ if (GCLocker::should_discard(cause, gc_count)) {
+ return;
+ }
+
VM_ParallelGCSystemGC op(gc_count, full_gc_count, cause);
VMThread::execute(&op);
}
--- a/src/hotspot/share/gc/parallel/psVMOperations.cpp Wed Jul 31 13:40:59 2019 -0400
+++ b/src/hotspot/share/gc/parallel/psVMOperations.cpp Wed Jul 31 14:28:51 2019 -0400
@@ -49,11 +49,16 @@
}
}
+static bool is_cause_full(GCCause::Cause cause) {
+ return (cause != GCCause::_gc_locker) && (cause != GCCause::_wb_young_gc)
+ DEBUG_ONLY(&& (cause != GCCause::_scavenge_alot));
+}
+
// Only used for System.gc() calls
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, true /* full */)
+ VM_GC_Operation(gc_count, gc_cause, full_gc_count, is_cause_full(gc_cause))
{
}
@@ -63,8 +68,7 @@
ParallelScavengeHeap* heap = ParallelScavengeHeap::heap();
GCCauseSetter gccs(heap, _gc_cause);
- if (_gc_cause == GCCause::_gc_locker || _gc_cause == GCCause::_wb_young_gc
- DEBUG_ONLY(|| _gc_cause == GCCause::_scavenge_alot)) {
+ if (!_full) {
// If (and only if) the scavenge fails, this will invoke a full gc.
heap->invoke_scavenge();
} else {
--- a/src/hotspot/share/gc/shared/gcLocker.cpp Wed Jul 31 13:40:59 2019 -0400
+++ b/src/hotspot/share/gc/shared/gcLocker.cpp Wed Jul 31 14:28:51 2019 -0400
@@ -36,6 +36,7 @@
volatile jint GCLocker::_jni_lock_count = 0;
volatile bool GCLocker::_needs_gc = false;
volatile bool GCLocker::_doing_gc = false;
+unsigned int GCLocker::_total_collections = 0;
#ifdef ASSERT
volatile jint GCLocker::_debug_jni_lock_count = 0;
@@ -115,6 +116,11 @@
}
}
+bool GCLocker::should_discard(GCCause::Cause cause, uint total_collections) {
+ return (cause == GCCause::_gc_locker) &&
+ (_total_collections != total_collections);
+}
+
void GCLocker::jni_lock(JavaThread* thread) {
assert(!thread->in_critical(), "shouldn't currently be in a critical region");
MonitorLocker ml(JNICritical_lock);
@@ -138,7 +144,13 @@
decrement_debug_jni_lock_count();
thread->exit_critical();
if (needs_gc() && !is_active_internal()) {
- // We're the last thread out. Cause a GC to occur.
+ // We're the last thread out. Request a GC.
+ // Capture the current total collections, to allow detection of
+ // other collections that make this one unnecessary. The value of
+ // total_collections() is only changed at a safepoint, so there
+ // must not be a safepoint between the lock becoming inactive and
+ // getting the count, else there may be unnecessary GCLocker GCs.
+ _total_collections = Universe::heap()->total_collections();
_doing_gc = true;
{
// Must give up the lock while at a safepoint
--- a/src/hotspot/share/gc/shared/gcLocker.hpp Wed Jul 31 13:40:59 2019 -0400
+++ b/src/hotspot/share/gc/shared/gcLocker.hpp Wed Jul 31 14:28:51 2019 -0400
@@ -25,6 +25,7 @@
#ifndef SHARE_GC_SHARED_GCLOCKER_HPP
#define SHARE_GC_SHARED_GCLOCKER_HPP
+#include "gc/shared/gcCause.hpp"
#include "memory/allocation.hpp"
#include "utilities/globalDefinitions.hpp"
#include "utilities/macros.hpp"
@@ -45,6 +46,7 @@
static volatile bool _needs_gc; // heap is filling, we need a GC
// note: bool is typedef'd as jint
static volatile bool _doing_gc; // unlock_critical() is doing a GC
+ static uint _total_collections; // value for _gc_locker collection
#ifdef ASSERT
// This lock count is updated for all operations and is used to
@@ -98,6 +100,12 @@
// Sets _needs_gc if is_active() is true. Returns is_active().
static bool check_active_before_gc();
+ // Return true if the designated collection is a GCLocker request
+ // that should be discarded. Returns true if cause == GCCause::_gc_locker
+ // and the given total collection value indicates a collection has been
+ // done since the GCLocker request was made.
+ static bool should_discard(GCCause::Cause cause, uint total_collections);
+
// Stalls the caller (who should not be in a jni critical section)
// until needs_gc() clears. Note however that needs_gc() may be
// set at a subsequent safepoint and/or cleared under the
--- a/src/hotspot/share/gc/shared/gcVMOperations.hpp Wed Jul 31 13:40:59 2019 -0400
+++ b/src/hotspot/share/gc/shared/gcVMOperations.hpp Wed Jul 31 14:28:51 2019 -0400
@@ -194,7 +194,8 @@
uint full_gc_count_before,
GCCause::Cause gc_cause,
GenCollectedHeap::GenerationType max_generation)
- : VM_GC_Operation(gc_count_before, gc_cause, full_gc_count_before, true /* full */),
+ : VM_GC_Operation(gc_count_before, gc_cause, full_gc_count_before,
+ max_generation != GenCollectedHeap::YoungGen /* full */),
_max_generation(max_generation) { }
~VM_GenCollectFull() {}
virtual VMOp_Type type() const { return VMOp_GenCollectFull; }
--- a/src/hotspot/share/gc/shared/genCollectedHeap.cpp Wed Jul 31 13:40:59 2019 -0400
+++ b/src/hotspot/share/gc/shared/genCollectedHeap.cpp Wed Jul 31 14:28:51 2019 -0400
@@ -947,8 +947,9 @@
// public collection interfaces
void GenCollectedHeap::collect(GCCause::Cause cause) {
- if (cause == GCCause::_wb_young_gc) {
- // Young collection for the WhiteBox API.
+ if ((cause == GCCause::_wb_young_gc) ||
+ (cause == GCCause::_gc_locker)) {
+ // Young collection for WhiteBox or GCLocker.
collect(cause, YoungGen);
} else {
#ifdef ASSERT
@@ -986,6 +987,11 @@
// Read the GC count while holding the Heap_lock
unsigned int gc_count_before = total_collections();
unsigned int full_gc_count_before = total_full_collections();
+
+ if (GCLocker::should_discard(cause, gc_count_before)) {
+ return;
+ }
+
{
MutexUnlocker mu(Heap_lock); // give up heap lock, execute gets it back
VM_GenCollectFull op(gc_count_before, full_gc_count_before,
@@ -1000,24 +1006,15 @@
void GenCollectedHeap::do_full_collection(bool clear_all_soft_refs,
GenerationType last_generation) {
- GenerationType local_last_generation;
- if (!incremental_collection_will_fail(false /* don't consult_young */) &&
- gc_cause() == GCCause::_gc_locker) {
- local_last_generation = YoungGen;
- } else {
- local_last_generation = last_generation;
- }
-
do_collection(true, // full
clear_all_soft_refs, // clear_all_soft_refs
0, // size
false, // is_tlab
- local_last_generation); // last_generation
+ last_generation); // last_generation
// Hack XXX FIX ME !!!
// A scavenge may not have been attempted, or may have
// been attempted and failed, because the old gen was too full
- if (local_last_generation == YoungGen && gc_cause() == GCCause::_gc_locker &&
- incremental_collection_will_fail(false /* don't consult_young */)) {
+ if (gc_cause() == GCCause::_gc_locker && incremental_collection_failed()) {
log_debug(gc, jni)("GC locker: Trying a full collection because scavenge failed");
// This time allow the old gen to be collected as well
do_collection(true, // full
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/test/hotspot/jtreg/gc/stress/gclocker/TestExcessGCLockerCollections.java Wed Jul 31 14:28:51 2019 -0400
@@ -0,0 +1,188 @@
+/*
+ * Copyright (c) 2019, 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.
+ */
+
+package gc.stress.gclocker;
+
+/*
+ * @test TestExcessGCLockerCollections
+ * @key gc
+ * @bug 8048556
+ * @summary Check for GC Locker initiated GCs that immediately follow another
+ * GC and so have very little needing to be collected.
+ * @requires vm.gc != "Z"
+ * @requires vm.gc != "Epsilon"
+ * @requires vm.gc != "Shenandoah"
+ * @library /test/lib
+ * @modules java.base/jdk.internal.misc
+ * @run driver/timeout=1000 gc.stress.gclocker.TestExcessGCLockerCollections 300 4 2
+ */
+
+import java.util.HashMap;
+import java.util.Map;
+
+import java.util.zip.Deflater;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+
+import jdk.test.lib.Asserts;
+import jdk.test.lib.process.ProcessTools;
+import jdk.test.lib.process.OutputAnalyzer;
+
+class TestExcessGCLockerCollectionsAux {
+ 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 = 128 * 1024;
+
+ static private void println(String str) { System.out.println(str); }
+
+ static private volatile boolean keepRunning = true;
+
+ static Map<Integer,String> populateMap(int size) {
+ Map<Integer,String> map = new HashMap<Integer,String>();
+ 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<Integer,String> map = populateMap(MAP_SIZE);
+ array[arrayIndex] = map;
+ arrayIndex = (arrayIndex + 1) % MAP_ARRAY_LENGTH;
+ }
+
+ public void run() {
+ while (keepRunning) {
+ 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);
+
+ count += 1;
+ }
+
+ public void run() {
+ while (keepRunning) {
+ doStep();
+ }
+ }
+ }
+
+ static public Map<Integer,String> largeMap;
+
+ static public void main(String args[]) {
+ long durationSec = Long.parseLong(args[0]);
+ int allocThreadNum = Integer.parseInt(args[1]);
+ int jniCriticalThreadNum = Integer.parseInt(args[2]);
+
+ println("Running for " + durationSec + " secs");
+
+ largeMap = populateMap(LARGE_MAP_SIZE);
+
+ 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();
+ }
+
+ long durationMS = (long) (1000 * durationSec);
+ long start = System.currentTimeMillis();
+ long now = start;
+ long soFar = now - start;
+ while (soFar < durationMS) {
+ try {
+ Thread.sleep(durationMS - soFar);
+ } catch (Exception e) {
+ }
+ now = System.currentTimeMillis();
+ soFar = now - start;
+ }
+ println("Done.");
+ keepRunning = false;
+ }
+}
+
+public class TestExcessGCLockerCollections {
+ private static final String locker =
+ "\\[gc\\s*\\] .* \\(GCLocker Initiated GC\\)";
+ private static final String ANY_LOCKER = locker + " [1-9][0-9]*M";
+ private static final String BAD_LOCKER = locker + " [1-9][0-9]?M";
+
+ private static final String[] COMMON_OPTIONS = new String[] {
+ "-Xmx1G", "-Xms1G", "-Xmn256M", "-Xlog:gc" };
+
+ public static void main(String args[]) throws Exception {
+ if (args.length < 3) {
+ System.out.println("usage: TestExcessGCLockerCollectionsAux" +
+ " <duration sec> <alloc threads>" +
+ " <jni critical threads>");
+ throw new RuntimeException("Invalid arguments");
+ }
+
+ ArrayList<String> finalArgs = new ArrayList<String>();
+ finalArgs.addAll(Arrays.asList(COMMON_OPTIONS));
+ finalArgs.add(TestExcessGCLockerCollectionsAux.class.getName());
+ finalArgs.addAll(Arrays.asList(args));
+
+ // GC and other options obtained from test framework.
+ ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(
+ true, finalArgs.toArray(new String[0]));
+ OutputAnalyzer output = new OutputAnalyzer(pb.start());
+ output.shouldHaveExitValue(0);
+ //System.out.println("------------- begin stdout ----------------");
+ //System.out.println(output.getStdout());
+ //System.out.println("------------- end stdout ----------------");
+ output.stdoutShouldMatch(ANY_LOCKER);
+ output.stdoutShouldNotMatch(BAD_LOCKER);
+ }
+}