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
--- a/hotspot/src/share/vm/gc_implementation/g1/concurrentMark.cpp Mon Mar 22 02:40:53 2010 -0700
+++ b/hotspot/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,
--- a/hotspot/src/share/vm/gc_implementation/g1/g1SATBCardTableModRefBS.cpp Mon Mar 22 02:40:53 2010 -0700
+++ b/hotspot/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);
--- a/hotspot/src/share/vm/gc_implementation/g1/ptrQueue.cpp Mon Mar 22 02:40:53 2010 -0700
+++ b/hotspot/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)
{}
--- a/hotspot/src/share/vm/gc_implementation/g1/ptrQueue.hpp Mon Mar 22 02:40:53 2010 -0700
+++ b/hotspot/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.
--- a/hotspot/src/share/vm/gc_implementation/g1/satbQueue.cpp Mon Mar 22 02:40:53 2010 -0700
+++ b/hotspot/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);
}
}
--- a/hotspot/src/share/vm/gc_implementation/g1/satbQueue.hpp Mon Mar 22 02:40:53 2010 -0700
+++ b/hotspot/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