# HG changeset patch
# User tonyp
# Date 1292361584 18000
# Node ID fb712ff225719ecfba55bf0cf0e221c334510a22
# Parent  f0ef5f5a460f18e664a6f8bca743443bf64de04c
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

diff -r f0ef5f5a460f -r fb712ff22571 src/share/vm/gc_implementation/g1/concurrentMarkThread.cpp
--- a/src/share/vm/gc_implementation/g1/concurrentMarkThread.cpp	Thu Dec 09 21:47:58 2010 -0800
+++ b/src/share/vm/gc_implementation/g1/concurrentMarkThread.cpp	Tue Dec 14 16:19:44 2010 -0500
@@ -277,7 +277,9 @@
     // 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");
 
diff -r f0ef5f5a460f -r fb712ff22571 src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp
--- a/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp	Thu Dec 09 21:47:58 2010 -0800
+++ b/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp	Tue Dec 14 16:19:44 2010 -0500
@@ -1389,7 +1389,7 @@
   }
 
   // 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 @@
      (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 @@
   // 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 @@
   // 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();
   }
 
diff -r f0ef5f5a460f -r fb712ff22571 src/share/vm/gc_implementation/g1/g1CollectedHeap.hpp
--- a/src/share/vm/gc_implementation/g1/g1CollectedHeap.hpp	Thu Dec 09 21:47:58 2010 -0800
+++ b/src/share/vm/gc_implementation/g1/g1CollectedHeap.hpp	Tue Dec 14 16:19:44 2010 -0500
@@ -643,16 +643,16 @@
   // 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;