8238855: Move G1ConcurrentMark flag sanity checks to g1Arguments

Reviewed-by: sjohanss, kbarrett

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

@@ -97,6 +97,23 @@ void G1Arguments::parse_verification_type(const char* type) {
   }
 }
 
+// Returns the maximum number of workers to be used in a concurrent
+// phase based on the number of GC workers being used in a STW
+// phase.
+static uint scale_concurrent_worker_threads(uint num_gc_workers) {
+  return MAX2((num_gc_workers + 2) / 4, 1U);
+}
+
+void G1Arguments::initialize_mark_stack_size() {
+  if (FLAG_IS_DEFAULT(MarkStackSize)) {
+    size_t mark_stack_size = MIN2(MarkStackSizeMax,
+                                  MAX2(MarkStackSize, (size_t)ConcGCThreads * TASKQUEUE_SIZE));
+    FLAG_SET_ERGO(MarkStackSize, mark_stack_size);
+  }
+
+  log_trace(gc)("MarkStackSize: %uk  MarkStackSizeMax: %uk", (uint)(MarkStackSize / K), (uint)(MarkStackSizeMax / K));
+}
+
 void G1Arguments::initialize() {
   GCArguments::initialize();
   assert(UseG1GC, "Error");
@@ -117,12 +134,11 @@ void G1Arguments::initialize() {
     FLAG_SET_ERGO(G1ConcRefinementThreads, ParallelGCThreads);
   }
 
-  // MarkStackSize will be set (if it hasn't been set by the user)
-  // when concurrent marking is initialized.
-  // Its value will be based upon the number of parallel marking threads.
-  // But we do set the maximum mark stack size here.
-  if (FLAG_IS_DEFAULT(MarkStackSizeMax)) {
-    FLAG_SET_DEFAULT(MarkStackSizeMax, 128 * TASKQUEUE_SIZE);
+  if (FLAG_IS_DEFAULT(ConcGCThreads) || ConcGCThreads == 0) {
+    // Calculate the number of concurrent worker threads by scaling
+    // the number of parallel GC threads.
+    uint marking_thread_num = scale_concurrent_worker_threads(ParallelGCThreads);
+    FLAG_SET_ERGO(ConcGCThreads, marking_thread_num);
   }
 
   if (FLAG_IS_DEFAULT(GCTimeRatio) || GCTimeRatio == 0) {
@@ -158,8 +174,6 @@ void G1Arguments::initialize() {
     FLAG_SET_DEFAULT(ParallelRefProcEnabled, true);
   }
 
-  log_trace(gc)("MarkStackSize: %uk  MarkStackSizeMax: %uk", (unsigned int) (MarkStackSize / K), (uint) (MarkStackSizeMax / K));
-
   // By default do not let the target stack size to be more than 1/4 of the entries
   if (FLAG_IS_DEFAULT(GCDrainStackTargetSize)) {
     FLAG_SET_ERGO(GCDrainStackTargetSize, MIN2(GCDrainStackTargetSize, (uintx)TASKQUEUE_SIZE / 4));
@@ -175,6 +189,7 @@ void G1Arguments::initialize() {
   }
 #endif
 
+  initialize_mark_stack_size();
   initialize_verification_types();
 }
 

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

@@ -36,6 +36,7 @@ class G1Arguments : public GCArguments {
 private:
   static size_t MaxMemoryForYoung;
 
+  static void initialize_mark_stack_size();
   static void initialize_verification_types();
   static void parse_verification_type(const char* type);
 

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

@@ -1800,10 +1800,6 @@ jint G1CollectedHeap::initialize() {
   // Create the G1ConcurrentMark data structure and thread.
   // (Must do this late, so that "max_regions" is defined.)
   _cm = new G1ConcurrentMark(this, prev_bitmap_storage, next_bitmap_storage);
-  if (!_cm->completed_initialization()) {
-    vm_shutdown_during_initialization("Could not initialize G1ConcurrentMark");
-    return JNI_ENOMEM;
-  }
   _cm_thread = _cm->cm_thread();
 
   // Now expand into the initial heap size.

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

@@ -353,19 +353,11 @@ bool G1CMRootMemRegions::wait_until_scan_finished() {
   return true;
 }
 
-// Returns the maximum number of workers to be used in a concurrent
-// phase based on the number of GC workers being used in a STW
-// phase.
-static uint scale_concurrent_worker_threads(uint num_gc_workers) {
-  return MAX2((num_gc_workers + 2) / 4, 1U);
-}
-
 G1ConcurrentMark::G1ConcurrentMark(G1CollectedHeap* g1h,
                                    G1RegionToSpaceMapper* prev_bitmap_storage,
                                    G1RegionToSpaceMapper* next_bitmap_storage) :
   // _cm_thread set inside the constructor
   _g1h(g1h),
-  _completed_initialization(false),
 
   _mark_bitmap_1(),
   _mark_bitmap_2(),
@@ -416,6 +408,8 @@ G1ConcurrentMark::G1ConcurrentMark(G1CollectedHeap* g1h,
   _region_mark_stats(NEW_C_HEAP_ARRAY(G1RegionMarkStats, _g1h->max_regions(), mtGC)),
   _top_at_rebuild_starts(NEW_C_HEAP_ARRAY(HeapWord*, _g1h->max_regions(), mtGC))
 {
+  assert(CGC_lock != NULL, "CGC_lock must be initialized");
+
   _mark_bitmap_1.initialize(g1h->reserved_region(), prev_bitmap_storage);
   _mark_bitmap_2.initialize(g1h->reserved_region(), next_bitmap_storage);
 
@@ -425,22 +419,6 @@ G1ConcurrentMark::G1ConcurrentMark(G1CollectedHeap* g1h,
     vm_shutdown_during_initialization("Could not create ConcurrentMarkThread");
   }
 
-  assert(CGC_lock != NULL, "CGC_lock must be initialized");
-
-  if (FLAG_IS_DEFAULT(ConcGCThreads) || ConcGCThreads == 0) {
-    // Calculate the number of concurrent worker threads by scaling
-    // the number of parallel GC threads.
-    uint marking_thread_num = scale_concurrent_worker_threads(ParallelGCThreads);
-    FLAG_SET_ERGO(ConcGCThreads, marking_thread_num);
-  }
-
-  assert(ConcGCThreads > 0, "ConcGCThreads have been set.");
-  if (ConcGCThreads > ParallelGCThreads) {
-    log_warning(gc)("More ConcGCThreads (%u) than ParallelGCThreads (%u).",
-                    ConcGCThreads, ParallelGCThreads);
-    return;
-  }
-
   log_debug(gc)("ConcGCThreads: %u offset %u", ConcGCThreads, _worker_id_offset);
   log_debug(gc)("ParallelGCThreads: %u", ParallelGCThreads);
 
@@ -450,40 +428,6 @@ G1ConcurrentMark::G1ConcurrentMark(G1CollectedHeap* g1h,
   _concurrent_workers = new WorkGang("G1 Conc", _max_concurrent_workers, false, true);
   _concurrent_workers->initialize_workers();
 
-  if (FLAG_IS_DEFAULT(MarkStackSize)) {
-    size_t mark_stack_size =
-      MIN2(MarkStackSizeMax,
-          MAX2(MarkStackSize, (size_t) (_max_concurrent_workers * TASKQUEUE_SIZE)));
-    // Verify that the calculated value for MarkStackSize is in range.
-    // It would be nice to use the private utility routine from Arguments.
-    if (!(mark_stack_size >= 1 && mark_stack_size <= MarkStackSizeMax)) {
-      log_warning(gc)("Invalid value calculated for MarkStackSize (" SIZE_FORMAT "): "
-                      "must be between 1 and " SIZE_FORMAT,
-                      mark_stack_size, MarkStackSizeMax);
-      return;
-    }
-    FLAG_SET_ERGO(MarkStackSize, mark_stack_size);
-  } else {
-    // Verify MarkStackSize is in range.
-    if (FLAG_IS_CMDLINE(MarkStackSize)) {
-      if (FLAG_IS_DEFAULT(MarkStackSizeMax)) {
-        if (!(MarkStackSize >= 1 && MarkStackSize <= MarkStackSizeMax)) {
-          log_warning(gc)("Invalid value specified for MarkStackSize (" SIZE_FORMAT "): "
-                          "must be between 1 and " SIZE_FORMAT,
-                          MarkStackSize, MarkStackSizeMax);
-          return;
-        }
-      } else if (FLAG_IS_CMDLINE(MarkStackSizeMax)) {
-        if (!(MarkStackSize >= 1 && MarkStackSize <= MarkStackSizeMax)) {
-          log_warning(gc)("Invalid value specified for MarkStackSize (" SIZE_FORMAT ")"
-                          " or for MarkStackSizeMax (" SIZE_FORMAT ")",
-                          MarkStackSize, MarkStackSizeMax);
-          return;
-        }
-      }
-    }
-  }
-
   if (!_global_mark_stack.initialize(MarkStackSize, MarkStackSizeMax)) {
     vm_exit_during_initialization("Failed to allocate initial concurrent mark overflow mark stack.");
   }
@@ -505,7 +449,6 @@ G1ConcurrentMark::G1ConcurrentMark(G1CollectedHeap* g1h,
   }
 
   reset_at_marking_complete();
-  _completed_initialization = true;
 }
 
 void G1ConcurrentMark::reset() {

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

@@ -303,7 +303,6 @@ class G1ConcurrentMark : public CHeapObj<mtGC> {
 
   G1ConcurrentMarkThread* _cm_thread;     // The thread doing the work
   G1CollectedHeap*        _g1h;           // The heap
-  bool                    _completed_initialization; // Set to true when initialization is complete
 
   // Concurrent marking support structures
   G1CMBitMap              _mark_bitmap_1;
@@ -604,11 +603,6 @@ class G1ConcurrentMark : public CHeapObj<mtGC> {
 
   inline bool is_marked_in_next_bitmap(oop p) const;
 
-  // Returns true if initialization was successfully completed.
-  bool completed_initialization() const {
-    return _completed_initialization;
-  }
-
   ConcurrentGCTimer* gc_timer_cm() const { return _gc_timer_cm; }
   G1OldTracer* gc_tracer_cm() const { return _gc_tracer_cm; }
 

src/hotspot/share/gc/shared/gc_globals.hpp

@@ -258,6 +258,7 @@
   product(size_t, MarkStackSize, NOT_LP64(32*K) LP64_ONLY(4*M),             \
           "Size of marking stack")                                          \
           constraint(MarkStackSizeConstraintFunc,AfterErgo)                 \
+          range(1, (max_jint - 1))                                          \
                                                                             \
   product(intx, RefDiscoveryPolicy, 0,                                      \
           "Select type of reference discovery policy: "                     \

src/hotspot/share/gc/shared/jvmFlagConstraintsGC.cpp

@@ -172,6 +172,7 @@ JVMFlag::Error SoftRefLRUPolicyMSPerMBConstraintFunc(intx value, bool verbose) {
 }
 
 JVMFlag::Error MarkStackSizeConstraintFunc(size_t value, bool verbose) {
+  // value == 0 is handled by the range constraint.
   if (value > MarkStackSizeMax) {
     JVMFlag::printError(verbose,
                         "MarkStackSize (" SIZE_FORMAT ") must be "

test/hotspot/jtreg/gc/g1/TestMarkStackSizes.java

@@ -0,0 +1,91 @@
+/*
+ * Copyright (c) 2020, 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.g1;
+
+/*
+ * @test TestMarkStackSizes
+ * @key gc regression
+ * @bug 8238855
+ * @summary Consistency checks for marking flag related options.
+ * @requires vm.gc.G1
+ * @library /test/lib
+ * @modules java.base/jdk.internal.misc
+ *          java.management
+ * @run main gc.g1.TestMarkStackSizes
+ */
+
+import java.util.ArrayList;
+import java.util.Collections;
+
+import jdk.test.lib.process.OutputAnalyzer;
+import jdk.test.lib.process.ProcessTools;
+
+public class TestMarkStackSizes {
+    private static void runTest(boolean shouldSucceed, String... extraArgs) throws Exception {
+        ArrayList<String> testArguments = new ArrayList<String>();
+
+        testArguments.add("-XX:+UseG1GC");
+        testArguments.add("-Xmx12m");
+        testArguments.add("-XX:+PrintFlagsFinal");
+        Collections.addAll(testArguments, extraArgs);
+        testArguments.add("-version");
+
+        ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(testArguments.toArray(new String[0]));
+
+        OutputAnalyzer output = new OutputAnalyzer(pb.start());
+
+        System.out.println(output.getStderr());
+
+        if (shouldSucceed) {
+            long markStackSize = Long.parseLong(output.firstMatch("MarkStackSize\\s+=\\s(\\d+)",1));
+            long markStackSizeMax = Long.parseLong(output.firstMatch("MarkStackSizeMax\\s+=\\s(\\d+)",1));
+            long concGCThreads = Long.parseLong(output.firstMatch("ConcGCThreads\\s+=\\s(\\d+)",1));
+            long parallelGCThreads = Long.parseLong(output.firstMatch("ParallelGCThreads\\s+=\\s(\\d+)",1));
+
+            System.out.println("MarkStackSize=" + markStackSize + " MarkStackSizeMax=" + markStackSizeMax +
+                               "ConcGCThreads=" + concGCThreads + " ParallelGCThreads=" + parallelGCThreads);
+
+            output.shouldHaveExitValue(0);
+        } else {
+            output.shouldNotHaveExitValue(0);
+        }
+    }
+
+    public static void main(String[] args) throws Exception {
+        runTest(false, "-XX:MarkStackSize=0");
+        runTest(false, "-XX:MarkStackSizeMax=0");
+
+        runTest(true, "-XX:MarkStackSize=100", "-XX:MarkStackSizeMax=101");
+        runTest(true, "-XX:MarkStackSize=101", "-XX:MarkStackSizeMax=101");
+        runTest(false, "-XX:MarkStackSize=101", "-XX:MarkStackSizeMax=100");
+
+        runTest(true, "-XX:ConcGCThreads=3", "-XX:ParallelGCThreads=3");
+        runTest(true, "-XX:ConcGCThreads=0", "-XX:ParallelGCThreads=3");
+        runTest(false, "-XX:ConcGCThreads=4", "-XX:ParallelGCThreads=3");
+
+        // With that high ParallelGCThreads the default ergonomics would calculate
+        // a mark stack size higher than maximum mark stack size.
+        runTest(true, "-XX:ParallelGCThreads=100", "-XX:MarkStackSizeMax=100");
+    }
+}