7000559: G1: assertion failure !outer || (full_collections_started ==...

7000559: G1: assertion failure !outer || (full_collections_started == _full_collections_completed + 1)
Summary: The concurrent marking thread can complete its operation and increment the full GC counter during a Full GC. This causes the nesting of increments to the start and end of Full GCs that we are expecting to be wrong. the fix is for the marking thread to join the suspendible thread set before incrementing the counter so that it's blocked until the Full GC (or any other safepoint) is finished. The change also includes some minor code cleanup (I renamed a parameter).
Reviewed-by: brutisso, ysr

src/share/vm/gc_implementation/g1/concurrentMarkThread.cpp

@@ -277,7 +277,9 @@ void ConcurrentMarkThread::run() {
     // completed. This will also notify the FullGCCount_lock in case a
     // Java thread is waiting for a full GC to happen (e.g., it
     // called System.gc() with +ExplicitGCInvokesConcurrent).
-    g1->increment_full_collections_completed(true /* outer */);
+    _sts.join();
+    g1->increment_full_collections_completed(true /* concurrent */);
+    _sts.leave();
   }
   assert(_should_terminate, "just checking");
 

src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp

@@ -1389,7 +1389,7 @@ bool G1CollectedHeap::do_collection(bool explicit_gc,
   }
 
   // Update the number of full collections that have been completed.
-  increment_full_collections_completed(false /* outer */);
+  increment_full_collections_completed(false /* concurrent */);
 
   if (PrintHeapAtGC) {
     Universe::print_heap_after_gc();
@@ -2176,9 +2176,14 @@ bool G1CollectedHeap::should_do_concurrent_full_gc(GCCause::Cause cause) {
      (cause == GCCause::_java_lang_system_gc && ExplicitGCInvokesConcurrent));
 }
 
-void G1CollectedHeap::increment_full_collections_completed(bool outer) {
+void G1CollectedHeap::increment_full_collections_completed(bool concurrent) {
   MonitorLockerEx x(FullGCCount_lock, Mutex::_no_safepoint_check_flag);
 
+  // We assume that if concurrent == true, then the caller is a
+  // concurrent thread that was joined the Suspendible Thread
+  // Set. If there's ever a cheap way to check this, we should add an
+  // assert here.
+
   // We have already incremented _total_full_collections at the start
   // of the GC, so total_full_collections() represents how many full
   // collections have been started.
@@ -2192,17 +2197,18 @@ void G1CollectedHeap::increment_full_collections_completed(bool outer) {
   // behind the number of full collections started.
 
   // This is the case for the inner caller, i.e. a Full GC.
-  assert(outer ||
+  assert(concurrent ||
          (full_collections_started == _full_collections_completed + 1) ||
          (full_collections_started == _full_collections_completed + 2),
-         err_msg("for inner caller: full_collections_started = %u "
+         err_msg("for inner caller (Full GC): full_collections_started = %u "
                  "is inconsistent with _full_collections_completed = %u",
                  full_collections_started, _full_collections_completed));
 
   // This is the case for the outer caller, i.e. the concurrent cycle.
-  assert(!outer ||
+  assert(!concurrent ||
          (full_collections_started == _full_collections_completed + 1),
-         err_msg("for outer caller: full_collections_started = %u "
+         err_msg("for outer caller (concurrent cycle): "
+                 "full_collections_started = %u "
                  "is inconsistent with _full_collections_completed = %u",
                  full_collections_started, _full_collections_completed));
 
@@ -2212,7 +2218,7 @@ void G1CollectedHeap::increment_full_collections_completed(bool outer) {
   // we wake up any waiters (especially when ExplicitInvokesConcurrent
   // is set) so that if a waiter requests another System.gc() it doesn't
   // incorrectly see that a marking cyle is still in progress.
-  if (outer) {
+  if (concurrent) {
     _cmThread->clear_in_progress();
   }
 

src/share/vm/gc_implementation/g1/g1CollectedHeap.hpp

@@ -643,16 +643,16 @@ public:
   // can happen in a nested fashion, i.e., we start a concurrent
   // cycle, a Full GC happens half-way through it which ends first,
   // and then the cycle notices that a Full GC happened and ends
-  // too. The outer parameter is a boolean to help us do a bit tighter
-  // consistency checking in the method. If outer is false, the caller
-  // is the inner caller in the nesting (i.e., the Full GC). If outer
-  // is true, the caller is the outer caller in this nesting (i.e.,
-  // the concurrent cycle). Further nesting is not currently
-  // supported. The end of the this call also notifies the
-  // FullGCCount_lock in case a Java thread is waiting for a full GC
-  // to happen (e.g., it called System.gc() with
+  // too. The concurrent parameter is a boolean to help us do a bit
+  // tighter consistency checking in the method. If concurrent is
+  // false, the caller is the inner caller in the nesting (i.e., the
+  // Full GC). If concurrent is true, the caller is the outer caller
+  // in this nesting (i.e., the concurrent cycle). Further nesting is
+  // not currently supported. The end of the this call also notifies
+  // the FullGCCount_lock in case a Java thread is waiting for a full
+  // GC to happen (e.g., it called System.gc() with
   // +ExplicitGCInvokesConcurrent).
-  void increment_full_collections_completed(bool outer);
+  void increment_full_collections_completed(bool concurrent);
 
   unsigned int full_collections_completed() {
     return _full_collections_completed;