# HG changeset patch
# User tonyp
# Date 1268928899 14400
# Node ID 6a778ef9ee4f1fb384c5779e0243d6193de1ff89
# Parent  1e887eabb2b9eb1c3adee9ace0b3ac27d78a90c5
6935821: G1: threads created during marking do not active their SATB queues
Summary: Newly-created threads always had the active field of their SATB queue initialized to false, even if they were created during marking. As a result, updates from threads created during a marking cycle were never enqueued and never processed. The fix includes remaining a method from active() to is_active() for readability and naming consistency.
Reviewed-by: ysr, johnc

diff -r 1e887eabb2b9 -r 6a778ef9ee4f src/share/vm/gc_implementation/g1/concurrentMark.cpp
--- a/src/share/vm/gc_implementation/g1/concurrentMark.cpp	Wed Mar 24 08:15:32 2010 -0700
+++ b/src/share/vm/gc_implementation/g1/concurrentMark.cpp	Thu Mar 18 12:14:59 2010 -0400
@@ -760,7 +760,10 @@
   rp->setup_policy(false); // snapshot the soft ref policy to be used in this cycle
 
   SATBMarkQueueSet& satb_mq_set = JavaThread::satb_mark_queue_set();
-  satb_mq_set.set_active_all_threads(true);
+  // This is the start of  the marking cycle, we're expected all
+  // threads to have SATB queues with active set to false.
+  satb_mq_set.set_active_all_threads(true, /* new active value */
+                                     false /* expected_active */);
 
   // update_g1_committed() will be called at the end of an evac pause
   // when marking is on. So, it's also called at the end of the
@@ -1079,7 +1082,11 @@
       gclog_or_tty->print_cr("\nRemark led to restart for overflow.");
   } else {
     // We're done with marking.
-    JavaThread::satb_mark_queue_set().set_active_all_threads(false);
+    // This is the end of  the marking cycle, we're expected all
+    // threads to have SATB queues with active set to true.
+    JavaThread::satb_mark_queue_set().set_active_all_threads(
+                                                  false, /* new active value */
+                                                  true /* expected_active */);
 
     if (VerifyDuringGC) {
       HandleMark hm;  // handle scope
@@ -2586,7 +2593,11 @@
 
   SATBMarkQueueSet& satb_mq_set = JavaThread::satb_mark_queue_set();
   satb_mq_set.abandon_partial_marking();
-  satb_mq_set.set_active_all_threads(false);
+  // This can be called either during or outside marking, we'll read
+  // the expected_active value from the SATB queue set.
+  satb_mq_set.set_active_all_threads(
+                                 false, /* new active value */
+                                 satb_mq_set.is_active() /* expected_active */);
 }
 
 static void print_ms_time_info(const char* prefix, const char* name,
diff -r 1e887eabb2b9 -r 6a778ef9ee4f src/share/vm/gc_implementation/g1/g1SATBCardTableModRefBS.cpp
--- a/src/share/vm/gc_implementation/g1/g1SATBCardTableModRefBS.cpp	Wed Mar 24 08:15:32 2010 -0700
+++ b/src/share/vm/gc_implementation/g1/g1SATBCardTableModRefBS.cpp	Thu Mar 18 12:14:59 2010 -0400
@@ -35,7 +35,7 @@
 
 void G1SATBCardTableModRefBS::enqueue(oop pre_val) {
   assert(pre_val->is_oop_or_null(true), "Error");
-  if (!JavaThread::satb_mark_queue_set().active()) return;
+  if (!JavaThread::satb_mark_queue_set().is_active()) return;
   Thread* thr = Thread::current();
   if (thr->is_Java_thread()) {
     JavaThread* jt = (JavaThread*)thr;
@@ -51,7 +51,7 @@
 G1SATBCardTableModRefBS::write_ref_field_pre_static(T* field,
                                                     oop new_val,
                                                     JavaThread* jt) {
-  if (!JavaThread::satb_mark_queue_set().active()) return;
+  if (!JavaThread::satb_mark_queue_set().is_active()) return;
   T heap_oop = oopDesc::load_heap_oop(field);
   if (!oopDesc::is_null(heap_oop)) {
     oop pre_val = oopDesc::decode_heap_oop_not_null(heap_oop);
@@ -62,7 +62,7 @@
 
 template <class T> void
 G1SATBCardTableModRefBS::write_ref_array_pre_work(T* dst, int count) {
-  if (!JavaThread::satb_mark_queue_set().active()) return;
+  if (!JavaThread::satb_mark_queue_set().is_active()) return;
   T* elem_ptr = dst;
   for (int i = 0; i < count; i++, elem_ptr++) {
     T heap_oop = oopDesc::load_heap_oop(elem_ptr);
diff -r 1e887eabb2b9 -r 6a778ef9ee4f src/share/vm/gc_implementation/g1/ptrQueue.cpp
--- a/src/share/vm/gc_implementation/g1/ptrQueue.cpp	Wed Mar 24 08:15:32 2010 -0700
+++ b/src/share/vm/gc_implementation/g1/ptrQueue.cpp	Thu Mar 18 12:14:59 2010 -0400
@@ -25,8 +25,8 @@
 # include "incls/_precompiled.incl"
 # include "incls/_ptrQueue.cpp.incl"
 
-PtrQueue::PtrQueue(PtrQueueSet* qset_, bool perm) :
-  _qset(qset_), _buf(NULL), _index(0), _active(false),
+PtrQueue::PtrQueue(PtrQueueSet* qset_, bool perm, bool active) :
+  _qset(qset_), _buf(NULL), _index(0), _active(active),
   _perm(perm), _lock(NULL)
 {}
 
diff -r 1e887eabb2b9 -r 6a778ef9ee4f src/share/vm/gc_implementation/g1/ptrQueue.hpp
--- a/src/share/vm/gc_implementation/g1/ptrQueue.hpp	Wed Mar 24 08:15:32 2010 -0700
+++ b/src/share/vm/gc_implementation/g1/ptrQueue.hpp	Thu Mar 18 12:14:59 2010 -0400
@@ -62,7 +62,7 @@
 public:
   // Initialize this queue to contain a null buffer, and be part of the
   // given PtrQueueSet.
-  PtrQueue(PtrQueueSet*, bool perm = false);
+  PtrQueue(PtrQueueSet*, bool perm = false, bool active = false);
   // Release any contained resources.
   void flush();
   // Calls flush() when destroyed.
@@ -101,6 +101,8 @@
     }
   }
 
+  bool is_active() { return _active; }
+
   static int byte_index_to_index(int ind) {
     assert((ind % oopSize) == 0, "Invariant.");
     return ind / oopSize;
@@ -257,7 +259,7 @@
   bool process_completed_buffers() { return _process_completed; }
   void set_process_completed(bool x) { _process_completed = x; }
 
-  bool active() { return _all_active; }
+  bool is_active() { return _all_active; }
 
   // Set the buffer size.  Should be called before any "enqueue" operation
   // can be called.  And should only be called once.
diff -r 1e887eabb2b9 -r 6a778ef9ee4f src/share/vm/gc_implementation/g1/satbQueue.cpp
--- a/src/share/vm/gc_implementation/g1/satbQueue.cpp	Wed Mar 24 08:15:32 2010 -0700
+++ b/src/share/vm/gc_implementation/g1/satbQueue.cpp	Thu Mar 18 12:14:59 2010 -0400
@@ -82,9 +82,57 @@
   t->satb_mark_queue().handle_zero_index();
 }
 
-void SATBMarkQueueSet::set_active_all_threads(bool b) {
+#ifdef ASSERT
+void SATBMarkQueueSet::dump_active_values(JavaThread* first,
+                                          bool expected_active) {
+  gclog_or_tty->print_cr("SATB queue active values for Java Threads");
+  gclog_or_tty->print_cr(" SATB queue set: active is %s",
+                         (is_active()) ? "TRUE" : "FALSE");
+  gclog_or_tty->print_cr(" expected_active is %s",
+                         (expected_active) ? "TRUE" : "FALSE");
+  for (JavaThread* t = first; t; t = t->next()) {
+    bool active = t->satb_mark_queue().is_active();
+    gclog_or_tty->print_cr("  thread %s, active is %s",
+                           t->name(), (active) ? "TRUE" : "FALSE");
+  }
+}
+#endif // ASSERT
+
+void SATBMarkQueueSet::set_active_all_threads(bool b,
+                                              bool expected_active) {
+  assert(SafepointSynchronize::is_at_safepoint(), "Must be at safepoint.");
+  JavaThread* first = Threads::first();
+
+#ifdef ASSERT
+  if (_all_active != expected_active) {
+    dump_active_values(first, expected_active);
+
+    // I leave this here as a guarantee, instead of an assert, so
+    // that it will still be compiled in if we choose to uncomment
+    // the #ifdef ASSERT in a product build. The whole block is
+    // within an #ifdef ASSERT so the guarantee will not be compiled
+    // in a product build anyway.
+    guarantee(false,
+              "SATB queue set has an unexpected active value");
+  }
+#endif // ASSERT
   _all_active = b;
-  for(JavaThread* t = Threads::first(); t; t = t->next()) {
+
+  for (JavaThread* t = first; t; t = t->next()) {
+#ifdef ASSERT
+    bool active = t->satb_mark_queue().is_active();
+    if (active != expected_active) {
+      dump_active_values(first, expected_active);
+
+      // I leave this here as a guarantee, instead of an assert, so
+      // that it will still be compiled in if we choose to uncomment
+      // the #ifdef ASSERT in a product build. The whole block is
+      // within an #ifdef ASSERT so the guarantee will not be compiled
+      // in a product build anyway.
+      guarantee(false,
+                "thread has an unexpected active value in its SATB queue");
+    }
+#endif // ASSERT
     t->satb_mark_queue().set_active(b);
   }
 }
diff -r 1e887eabb2b9 -r 6a778ef9ee4f src/share/vm/gc_implementation/g1/satbQueue.hpp
--- a/src/share/vm/gc_implementation/g1/satbQueue.hpp	Wed Mar 24 08:15:32 2010 -0700
+++ b/src/share/vm/gc_implementation/g1/satbQueue.hpp	Thu Mar 18 12:14:59 2010 -0400
@@ -29,8 +29,7 @@
 class ObjPtrQueue: public PtrQueue {
 public:
   ObjPtrQueue(PtrQueueSet* qset_, bool perm = false) :
-    PtrQueue(qset_, perm)
-  {}
+    PtrQueue(qset_, perm, qset_->is_active()) { }
   // Apply the closure to all elements, and reset the index to make the
   // buffer empty.
   void apply_closure(ObjectClosure* cl);
@@ -55,6 +54,9 @@
   // is ignored.
   bool apply_closure_to_completed_buffer_work(bool par, int worker);
 
+#ifdef ASSERT
+  void dump_active_values(JavaThread* first, bool expected_active);
+#endif // ASSERT
 
 public:
   SATBMarkQueueSet();
@@ -65,9 +67,11 @@
 
   static void handle_zero_index_for_thread(JavaThread* t);
 
-  // Apply "set_active(b)" to all thread tloq's.  Should be called only
-  // with the world stopped.
-  void set_active_all_threads(bool b);
+  // Apply "set_active(b)" to all Java threads' SATB queues. It should be
+  // called only with the world stopped. The method will assert that the
+  // SATB queues of all threads it visits, as well as the SATB queue
+  // set itself, has an active value same as expected_active.
+  void set_active_all_threads(bool b, bool expected_active);
 
   // Register "blk" as "the closure" for all queues.  Only one such closure
   // is allowed.  The "apply_closure_to_completed_buffer" method will apply