8048556: Unnecessary GCLocker-initiated young GCs
authorkbarrett
Wed, 31 Jul 2019 14:28:51 -0400
changeset 57607 8d3886985964
parent 57606 b81062d47d61
child 57608 0d17da18142b
8048556: Unnecessary GCLocker-initiated young GCs Summary: Fixed recognition of unnecessary GCLocker collections. Reviewed-by: pliden, tschatzl
src/hotspot/share/gc/g1/g1CollectedHeap.cpp
src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp
src/hotspot/share/gc/parallel/psVMOperations.cpp
src/hotspot/share/gc/shared/gcLocker.cpp
src/hotspot/share/gc/shared/gcLocker.hpp
src/hotspot/share/gc/shared/gcVMOperations.hpp
src/hotspot/share/gc/shared/genCollectedHeap.cpp
test/hotspot/jtreg/gc/stress/gclocker/TestExcessGCLockerCollections.java
--- 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);
+    }
+}