Self review

src/hotspot/share/gc/g1/g1ServiceThread.cpp

@@ -210,8 +210,7 @@ void G1ServiceThread::register_task(G1ServiceTask* task, jlong delay) {
 
   // Notify the service thread that there is a new task, thread might
   // be waiting and the newly added task might be first in the list.
-  MonitorLocker ml(&_monitor, Mutex::_no_safepoint_check_flag);
-  ml.notify();
+  notify();
 }
 
 void G1ServiceThread::schedule_task(G1ServiceTask* task, jlong delay_ms) {
@@ -243,6 +242,11 @@ int64_t G1ServiceThread::time_to_next_task_ms() {
   return (int64_t) TimeHelper::counter_to_millis(time_diff);
 }
 
+void G1ServiceThread::notify() {
+  MonitorLocker ml(&_monitor, Mutex::_no_safepoint_check_flag);
+  ml.notify();
+}
+
 void G1ServiceThread::sleep_before_next_cycle() {
   if (should_terminate()) {
     return;
@@ -309,8 +313,7 @@ void G1ServiceThread::run_service() {
 }
 
 void G1ServiceThread::stop_service() {
-  MonitorLocker ml(&_monitor, Mutex::_no_safepoint_check_flag);
-  ml.notify();
+  notify();
 }
 
 G1ServiceTask::G1ServiceTask(const char* name) :

src/hotspot/share/gc/g1/g1ServiceThread.hpp

@@ -133,6 +133,9 @@ class G1ServiceThread: public ConcurrentGCThread {
   // Register a task with the service thread and schedule it. If
   // no delay is specified the task is scheduled to run directly.
   void register_task(G1ServiceTask* task, jlong delay = 0);
+  // Notify a change to the service thread. Used to stop either
+  // stop the service or to force check for new tasks.
+  void notify();
 };
 
 #endif // SHARE_GC_G1_G1SERVICETHREAD_HPP

src/hotspot/share/gc/g1/g1UncommitRegionTask.cpp

@@ -26,6 +26,7 @@
 #include "gc/g1/g1CollectedHeap.inline.hpp"
 #include "gc/g1/g1UncommitRegionTask.hpp"
 #include "gc/shared/suspendibleThreadSet.hpp"
+#include "runtime/globals.hpp"
 #include "utilities/ticks.hpp"
 
 G1UncommitRegionTask* G1UncommitRegionTask::_instance = NULL;
@@ -58,8 +59,11 @@ void G1UncommitRegionTask::activate() {
 
   G1UncommitRegionTask* uncommit_task = instance();
   if (!uncommit_task->is_active()) {
+    // Change state to active and schedule with no delay.
     uncommit_task->set_state(TaskState::active);
     uncommit_task->schedule(0);
+    // Notify service thread to avoid unnecesarry waiting.
+    G1CollectedHeap::heap()->service_thread()->notify();
   }
 }
 
@@ -70,7 +74,6 @@ bool G1UncommitRegionTask::is_active() {
 void G1UncommitRegionTask::set_state(TaskState state) {
   assert(_state != state, "Must do a state change");
   _state = state;
-  log_trace(gc, heap)("%s, new state: %s", name(), is_active() ? "active" : "inactive");
 }
 
 void G1UncommitRegionTask::report_execution(Tickspan time, uint regions) {
@@ -100,12 +103,15 @@ void G1UncommitRegionTask::clear_summary() {
 void G1UncommitRegionTask::execute() {
   assert(_state == TaskState::active, "Must be active");
 
+  // Each execution is limited to uncommit at most 256M worth of regions.
+  static const uint region_limit = (uint) (256 * M / G1HeapRegionSize);
+
   // Prevent from running during a GC pause.
   SuspendibleThreadSetJoiner sts;
   HeapRegionManager* hrm = G1CollectedHeap::heap()->hrm();
 
   Ticks start = Ticks::now();
-  uint uncommit_count = hrm->uncommit_inactive_regions(UncommitRegionLimit);
+  uint uncommit_count = hrm->uncommit_inactive_regions(region_limit);
   Tickspan uncommit_time = (Ticks::now() - start);
 
   if (uncommit_count > 0) {

src/hotspot/share/gc/g1/g1UncommitRegionTask.hpp

@@ -30,10 +30,6 @@
 #include "utilities/ticks.hpp"
 
 class G1UncommitRegionTask : public G1ServiceTask {
-  // Limit a single execution of the task to uncommit at most
-  // this many regions.
-  static const uint UncommitRegionLimit = 32;
-
   static G1UncommitRegionTask* _instance;
   static void initialize();
   static G1UncommitRegionTask* instance();

test/hotspot/jtreg/gc/stress/TestStressG1Uncommit.java

@@ -30,8 +30,6 @@
  * @requires vm.gc.G1
  * @library /test/lib
  * @modules java.base/jdk.internal.misc
- * @build sun.hotspot.WhiteBox
- * @run driver ClassFileInstaller sun.hotspot.WhiteBox
  * @run driver/timeout=1300 gc.stress.TestStressG1Uncommit
  */
 
@@ -53,26 +51,13 @@
 import jdk.test.lib.process.ProcessTools;
 import jdk.test.lib.process.OutputAnalyzer;
 
-import sun.hotspot.WhiteBox;
-
 public class TestStressG1Uncommit {
     public static void main(String[] args) throws Exception {
-        runTest("Full");
-        runTest("Concurrent");
-    }
-
-    // Run the test with explicit arguments to make sure heap is not
-    // fixed and that we get the expected log tags.
-    private static void runTest(String gcType) throws Exception{
         ArrayList<String> options = new ArrayList<>();
         Collections.addAll(options,
-            "-Xbootclasspath/a:.",
-            "-XX:+UnlockDiagnosticVMOptions",
-            "-XX:+WhiteBoxAPI",
             "-Xlog:gc,gc+heap+region=debug",
             "-XX:+UseG1GC",
-            StressUncommit.class.getName(),
-            gcType
+            StressUncommit.class.getName()
         );
         ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(options);
         OutputAnalyzer output = new OutputAnalyzer(pb.start());
@@ -87,18 +72,11 @@ class StressUncommit {
     private static final long G = 1024 * M;
     private static final Instant StartTime = Instant.now();
 
-    private static final WhiteBox wb = WhiteBox.getWhiteBox();
     private static final ThreadMXBean threadBean = (ThreadMXBean) ManagementFactory.getThreadMXBean();
     private static final MemoryMXBean memoryBean = ManagementFactory.getMemoryMXBean();
     private static ConcurrentLinkedQueue<Object> globalKeepAlive;
 
     public static void main(String args[]) throws InterruptedException {
-        // Get GC type for this run.
-        if (args.length != 1) {
-            throw new IllegalArgumentException("Missing argument: <type of GC> Full or Concurrent");
-        }
-        GCType gc = GCType.valueOf(args[0]);
-
         // Leave 20% head room to try to avoid Full GCs.
         long allocationSize = (long) (Runtime.getRuntime().maxMemory() * 0.8);
 
@@ -127,13 +105,13 @@ public static void main(String args[]) throws InterruptedException {
 
                 // Wait for tasks to complete.
                 workersRunning.await();
-                long committedBefore = memoryBean.getHeapMemoryUsage().getCommitted();
 
                 // Clear the reference holding all task allocations alive.
                 globalKeepAlive = null;
 
                 // Do a GC that should shrink the heap.
-                gc.invoke();
+                long committedBefore = memoryBean.getHeapMemoryUsage().getCommitted();
+                System.gc();
                 long committedAfter = memoryBean.getHeapMemoryUsage().getCommitted();
                 Asserts.assertLessThan(committedAfter, committedBefore);
             }
@@ -168,42 +146,4 @@ private static long uptime() {
     private static void log(String text) {
         System.out.println(uptime() + "s: " + text);
     }
-
-    private enum GCType {
-        Full {
-            public void invoke() {
-                log("  Full GC");
-                System.gc();
-            }
-        },
-        Concurrent {
-            public void invoke() {
-                // First wait for any ergonomically started cycles.
-                if (wb.g1InConcurrentMark()) {
-                    waitForConcurrentCycleToFinish();
-                }
-                log("  Concurrent GC");
-                wb.g1StartConcMarkCycle();
-                waitForConcurrentCycleToFinish();
-                // Trigger mixed GCs to compact old a bit.
-                wb.youngGC();
-                wb.youngGC();
-                wb.youngGC();
-                // A second cycle to ensure we free up enough regions.
-                wb.g1StartConcMarkCycle();
-                waitForConcurrentCycleToFinish();
-            }
-        };
-        public abstract void invoke();
-    }
-
-    private static void waitForConcurrentCycleToFinish() {
-        while (wb.g1InConcurrentMark()) {
-            try {
-                Thread.sleep(100);
-            } catch (InterruptedException e) {
-                e.printStackTrace();
-            }
-        }
-    }
 }