8221361: Eliminate two-phase initialization for PtrQueueSet classes

Kim Barrett · Kim Barrett · commit 8b67b75f5001 · 2019-09-09T16:54:48.000-04:00
Move allocator and CBL monitor init to constructor.

Reviewed-by: tschatzl, shade
diff --git a/src/hotspot/share/gc/g1/g1BarrierSet.cpp b/src/hotspot/share/gc/g1/g1BarrierSet.cpp
@@ -57,8 +57,8 @@ G1BarrierSet::G1BarrierSet(G1CardTable* card_table) :
                       BarrierSet::FakeRtti(BarrierSet::G1BarrierSet)),
   _satb_mark_queue_buffer_allocator("SATB Buffer Allocator", G1SATBBufferSize),
   _dirty_card_queue_buffer_allocator("DC Buffer Allocator", G1UpdateBufferSize),
-  _satb_mark_queue_set(),
-  _dirty_card_queue_set(),
+  _satb_mark_queue_set(&_satb_mark_queue_buffer_allocator),
+  _dirty_card_queue_set(DirtyCardQ_CBL_mon, &_dirty_card_queue_buffer_allocator),
   _shared_dirty_card_queue(&_dirty_card_queue_set)
 {}
 
@@ -159,11 +159,3 @@ void G1BarrierSet::on_thread_detach(Thread* thread) {
   G1ThreadLocalData::satb_mark_queue(thread).flush();
   G1ThreadLocalData::dirty_card_queue(thread).flush();
 }
-
-BufferNode::Allocator& G1BarrierSet::satb_mark_queue_buffer_allocator() {
-  return _satb_mark_queue_buffer_allocator;
-}
-
-BufferNode::Allocator& G1BarrierSet::dirty_card_queue_buffer_allocator() {
-  return _dirty_card_queue_buffer_allocator;
-}
diff --git a/src/hotspot/share/gc/g1/g1BarrierSet.hpp b/src/hotspot/share/gc/g1/g1BarrierSet.hpp
@@ -82,9 +82,6 @@ class G1BarrierSet: public CardTableBarrierSet {
   virtual void on_thread_attach(Thread* thread);
   virtual void on_thread_detach(Thread* thread);
 
-  BufferNode::Allocator& satb_mark_queue_buffer_allocator();
-  BufferNode::Allocator& dirty_card_queue_buffer_allocator();
-
   static G1SATBMarkQueueSet& satb_mark_queue_set() {
     return g1_barrier_set()->_satb_mark_queue_set;
   }
diff --git a/src/hotspot/share/gc/g1/g1CollectedHeap.cpp b/src/hotspot/share/gc/g1/g1CollectedHeap.cpp
@@ -1678,15 +1678,11 @@ jint G1CollectedHeap::initialize() {
   BarrierSet::set_barrier_set(bs);
   _card_table = ct;
 
-  G1BarrierSet::satb_mark_queue_set().initialize(this,
-                                                 &bs->satb_mark_queue_buffer_allocator(),
-                                                 G1SATBProcessCompletedThreshold,
-                                                 G1SATBBufferEnqueueingThresholdPercent);
-
-  // process_cards_threshold and max_cards are updated
-  // later, based on the concurrent refinement object.
-  G1BarrierSet::dirty_card_queue_set().initialize(DirtyCardQ_CBL_mon,
-                                                  &bs->dirty_card_queue_buffer_allocator());
+  {
+    G1SATBMarkQueueSet& satbqs = bs->satb_mark_queue_set();
+    satbqs.set_process_completed_buffers_threshold(G1SATBProcessCompletedThreshold);
+    satbqs.set_buffer_enqueue_threshold_percentage(G1SATBBufferEnqueueingThresholdPercent);
+  }
 
   // Create the hot card cache.
   _hot_card_cache = new G1HotCardCache(this);
diff --git a/src/hotspot/share/gc/g1/g1DirtyCardQueue.cpp b/src/hotspot/share/gc/g1/g1DirtyCardQueue.cpp
@@ -62,9 +62,10 @@ void G1DirtyCardQueue::handle_completed_buffer() {
   }
 }
 
-G1DirtyCardQueueSet::G1DirtyCardQueueSet() :
-  PtrQueueSet(),
-  _cbl_mon(NULL),
+G1DirtyCardQueueSet::G1DirtyCardQueueSet(Monitor* cbl_mon,
+                                         BufferNode::Allocator* allocator) :
+  PtrQueueSet(allocator),
+  _cbl_mon(cbl_mon),
   _completed_buffers_head(NULL),
   _completed_buffers_tail(NULL),
   _num_cards(0),
@@ -88,13 +89,6 @@ uint G1DirtyCardQueueSet::num_par_ids() {
   return (uint)os::initial_active_processor_count();
 }
 
-void G1DirtyCardQueueSet::initialize(Monitor* cbl_mon,
-                                     BufferNode::Allocator* allocator) {
-  PtrQueueSet::initialize(allocator);
-  assert(_cbl_mon == NULL, "Init order issue?");
-  _cbl_mon = cbl_mon;
-}
-
 void G1DirtyCardQueueSet::handle_zero_index_for_thread(Thread* t) {
   G1ThreadLocalData::dirty_card_queue(t).handle_zero_index();
 }
diff --git a/src/hotspot/share/gc/g1/g1DirtyCardQueue.hpp b/src/hotspot/share/gc/g1/g1DirtyCardQueue.hpp
@@ -103,11 +103,9 @@ class G1DirtyCardQueueSet: public PtrQueueSet {
   jint _processed_buffers_rs_thread;
 
 public:
-  G1DirtyCardQueueSet();
+  G1DirtyCardQueueSet(Monitor* cbl_mon, BufferNode::Allocator* allocator);
   ~G1DirtyCardQueueSet();
 
-  void initialize(Monitor* cbl_mon, BufferNode::Allocator* allocator);
-
   // The number of parallel ids that can be claimed to allow collector or
   // mutator threads to do card-processing work.
   static uint num_par_ids();
diff --git a/src/hotspot/share/gc/g1/g1RedirtyCardsQueue.cpp b/src/hotspot/share/gc/g1/g1RedirtyCardsQueue.cpp
@@ -31,12 +31,10 @@
 // G1RedirtyCardsQueueBase::LocalQSet
 
 G1RedirtyCardsQueueBase::LocalQSet::LocalQSet(G1RedirtyCardsQueueSet* shared_qset) :
-  PtrQueueSet(),
+  PtrQueueSet(shared_qset->allocator()),
   _shared_qset(shared_qset),
   _buffers()
-{
-  PtrQueueSet::initialize(_shared_qset->allocator());
-}
+{}
 
 G1RedirtyCardsQueueBase::LocalQSet::~LocalQSet() {
   assert(_buffers._head == NULL, "unflushed qset");
@@ -86,14 +84,12 @@ void G1RedirtyCardsQueue::flush() {
 // G1RedirtyCardsQueueSet
 
 G1RedirtyCardsQueueSet::G1RedirtyCardsQueueSet(BufferNode::Allocator* allocator) :
-  PtrQueueSet(),
+  PtrQueueSet(allocator),
   _list(),
   _entry_count(0),
   _tail(NULL)
   DEBUG_ONLY(COMMA _collecting(true))
-{
-  initialize(allocator);
-}
+{}
 
 G1RedirtyCardsQueueSet::~G1RedirtyCardsQueueSet() {
   verify_empty();
diff --git a/src/hotspot/share/gc/g1/g1SATBMarkQueueSet.cpp b/src/hotspot/share/gc/g1/g1SATBMarkQueueSet.cpp
@@ -32,17 +32,9 @@
 #include "utilities/debug.hpp"
 #include "utilities/globalDefinitions.hpp"
 
-G1SATBMarkQueueSet::G1SATBMarkQueueSet() : _g1h(NULL) {}
-
-void G1SATBMarkQueueSet::initialize(G1CollectedHeap* g1h,
-                                    BufferNode::Allocator* allocator,
-                                    size_t process_completed_buffers_threshold,
-                                    uint buffer_enqueue_threshold_percentage) {
-  SATBMarkQueueSet::initialize(allocator,
-                               process_completed_buffers_threshold,
-                               buffer_enqueue_threshold_percentage);
-  _g1h = g1h;
-}
+G1SATBMarkQueueSet::G1SATBMarkQueueSet(BufferNode::Allocator* allocator) :
+  SATBMarkQueueSet(allocator)
+{}
 
 void G1SATBMarkQueueSet::handle_zero_index_for_thread(Thread* t) {
   G1ThreadLocalData::satb_mark_queue(t).handle_zero_index();
@@ -112,7 +104,7 @@ class G1SATBMarkQueueFilterFn {
   G1CollectedHeap* _g1h;
 
 public:
-  G1SATBMarkQueueFilterFn(G1CollectedHeap* g1h) : _g1h(g1h) {}
+  G1SATBMarkQueueFilterFn() : _g1h(G1CollectedHeap::heap()) {}
 
   // Return true if entry should be filtered out (removed), false if
   // it should be retained.
@@ -122,6 +114,5 @@ class G1SATBMarkQueueFilterFn {
 };
 
 void G1SATBMarkQueueSet::filter(SATBMarkQueue* queue) {
-  assert(_g1h != NULL, "SATB queue set not initialized");
-  apply_filter(G1SATBMarkQueueFilterFn(_g1h), queue);
+  apply_filter(G1SATBMarkQueueFilterFn(), queue);
 }
diff --git a/src/hotspot/share/gc/g1/g1SATBMarkQueueSet.hpp b/src/hotspot/share/gc/g1/g1SATBMarkQueueSet.hpp
@@ -35,12 +35,7 @@ class G1SATBMarkQueueSet : public SATBMarkQueueSet {
   G1CollectedHeap* _g1h;
 
 public:
-  G1SATBMarkQueueSet();
-
-  void initialize(G1CollectedHeap* g1h,
-                  BufferNode::Allocator* allocator,
-                  size_t process_completed_buffers_threshold,
-                  uint buffer_enqueue_threshold_percentage);
+  G1SATBMarkQueueSet(BufferNode::Allocator* allocator);
 
   static void handle_zero_index_for_thread(Thread* t);
   virtual SATBMarkQueue& satb_queue_for_thread(Thread* const t) const;
diff --git a/src/hotspot/share/gc/shared/ptrQueue.cpp b/src/hotspot/share/gc/shared/ptrQueue.cpp
@@ -40,7 +40,7 @@ PtrQueue::PtrQueue(PtrQueueSet* qset, bool active) :
   _qset(qset),
   _active(active),
   _index(0),
-  _capacity_in_bytes(0),
+  _capacity_in_bytes(index_to_byte_index(qset->buffer_size())),
   _buf(NULL)
 {}
 
@@ -80,13 +80,6 @@ void PtrQueue::handle_zero_index() {
   if (_buf != NULL) {
     handle_completed_buffer();
   } else {
-    // Bootstrapping kludge; lazily initialize capacity.  The initial
-    // thread's queues are constructed before the second phase of the
-    // two-phase initialization of the associated qsets.  As a result,
-    // we can't initialize _capacity_in_bytes in the queue constructor.
-    if (_capacity_in_bytes == 0) {
-      _capacity_in_bytes = index_to_byte_index(qset()->buffer_size());
-    }
     allocate_buffer();
   }
 }
@@ -250,18 +243,13 @@ size_t BufferNode::Allocator::reduce_free_list(size_t remove_goal) {
   return removed;
 }
 
-PtrQueueSet::PtrQueueSet() :
-  _allocator(NULL),
+PtrQueueSet::PtrQueueSet(BufferNode::Allocator* allocator) :
+  _allocator(allocator),
   _all_active(false)
 {}
 
 PtrQueueSet::~PtrQueueSet() {}
 
-void PtrQueueSet::initialize(BufferNode::Allocator* allocator) {
-  assert(allocator != NULL, "Init order issue?");
-  _allocator = allocator;
-}
-
 void** PtrQueueSet::allocate_buffer() {
   BufferNode* node = _allocator->allocate();
   return BufferNode::make_buffer_from_node(node);
diff --git a/src/hotspot/share/gc/shared/ptrQueue.hpp b/src/hotspot/share/gc/shared/ptrQueue.hpp
@@ -303,13 +303,9 @@ class PtrQueueSet {
   bool _all_active;
 
   // Create an empty ptr queue set.
-  PtrQueueSet();
+  PtrQueueSet(BufferNode::Allocator* allocator);
   ~PtrQueueSet();
 
-  // Because of init-order concerns, we can't pass these as constructor
-  // arguments.
-  void initialize(BufferNode::Allocator* allocator);
-
 public:
 
   // Return the associated BufferNode allocator.
diff --git a/src/hotspot/share/gc/shared/satbMarkQueue.cpp b/src/hotspot/share/gc/shared/satbMarkQueue.cpp
@@ -108,8 +108,8 @@ void SATBMarkQueue::print(const char* name) {
 
 #endif // PRODUCT
 
-SATBMarkQueueSet::SATBMarkQueueSet() :
-  PtrQueueSet(),
+SATBMarkQueueSet::SATBMarkQueueSet(BufferNode::Allocator* allocator) :
+  PtrQueueSet(allocator),
   _list(),
   _count_and_process_flag(0),
   _process_completed_buffers_threshold(SIZE_MAX),
@@ -153,27 +153,21 @@ static void decrement_count(volatile size_t* cfptr) {
   } while (value != old);
 }
 
-// Scale requested threshold to align with count field.  If scaling
-// overflows, just use max value.  Set process flag field to make
-// comparison in increment_count exact.
-static size_t scale_threshold(size_t value) {
+void SATBMarkQueueSet::set_process_completed_buffers_threshold(size_t value) {
+  // Scale requested threshold to align with count field.  If scaling
+  // overflows, just use max value.  Set process flag field to make
+  // comparison in increment_count exact.
   size_t scaled_value = value << 1;
   if ((scaled_value >> 1) != value) {
     scaled_value = SIZE_MAX;
   }
-  return scaled_value | 1;
+  _process_completed_buffers_threshold = scaled_value | 1;
 }
 
-void SATBMarkQueueSet::initialize(BufferNode::Allocator* allocator,
-                                  size_t process_completed_buffers_threshold,
-                                  uint buffer_enqueue_threshold_percentage) {
-  PtrQueueSet::initialize(allocator);
-  _process_completed_buffers_threshold =
-    scale_threshold(process_completed_buffers_threshold);
-  assert(buffer_size() != 0, "buffer size not initialized");
+void SATBMarkQueueSet::set_buffer_enqueue_threshold_percentage(uint value) {
   // Minimum threshold of 1 ensures enqueuing of completely full buffers.
   size_t size = buffer_size();
-  size_t enqueue_qty = (size * buffer_enqueue_threshold_percentage) / 100;
+  size_t enqueue_qty = (size * value) / 100;
   _buffer_enqueue_threshold = MAX2(size - enqueue_qty, (size_t)1);
 }
 
diff --git a/src/hotspot/share/gc/shared/satbMarkQueue.hpp b/src/hotspot/share/gc/shared/satbMarkQueue.hpp
@@ -112,18 +112,14 @@ class SATBMarkQueueSet: public PtrQueueSet {
 #endif // ASSERT
 
 protected:
-  SATBMarkQueueSet();
+  SATBMarkQueueSet(BufferNode::Allocator* allocator);
   ~SATBMarkQueueSet();
 
   template<typename Filter>
   void apply_filter(Filter filter, SATBMarkQueue* queue) {
     queue->apply_filter(filter);
   }
 
-  void initialize(BufferNode::Allocator* allocator,
-                  size_t process_completed_buffers_threshold,
-                  uint buffer_enqueue_threshold_percentage);
-
 public:
   virtual SATBMarkQueue& satb_queue_for_thread(Thread* const t) const = 0;
 
@@ -133,7 +129,11 @@ class SATBMarkQueueSet: public PtrQueueSet {
   // set itself, has an active value same as expected_active.
   void set_active_all_threads(bool active, bool expected_active);
 
+  void set_process_completed_buffers_threshold(size_t value);
+
   size_t buffer_enqueue_threshold() const { return _buffer_enqueue_threshold; }
+  void set_buffer_enqueue_threshold_percentage(uint value);
+
   virtual void filter(SATBMarkQueue* queue) = 0;
 
   // If there exists some completed buffer, pop and process it, and
diff --git a/src/hotspot/share/gc/shenandoah/shenandoahBarrierSet.cpp b/src/hotspot/share/gc/shenandoah/shenandoahBarrierSet.cpp
@@ -75,7 +75,8 @@ ShenandoahBarrierSet::ShenandoahBarrierSet(ShenandoahHeap* heap) :
              NULL /* barrier_set_nmethod */,
              BarrierSet::FakeRtti(BarrierSet::ShenandoahBarrierSet)),
   _heap(heap),
-  _satb_mark_queue_set()
+  _satb_mark_queue_buffer_allocator("SATB Buffer Allocator", ShenandoahSATBBufferSize),
+  _satb_mark_queue_set(&_satb_mark_queue_buffer_allocator)
 {
 }
 
diff --git a/src/hotspot/share/gc/shenandoah/shenandoahBarrierSet.hpp b/src/hotspot/share/gc/shenandoah/shenandoahBarrierSet.hpp
@@ -41,6 +41,7 @@ class ShenandoahBarrierSet: public BarrierSet {
 private:
 
   ShenandoahHeap* _heap;
+  BufferNode::Allocator _satb_mark_queue_buffer_allocator;
   ShenandoahSATBMarkQueueSet _satb_mark_queue_set;
 
 public:
diff --git a/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp b/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp
@@ -343,11 +343,13 @@ jint ShenandoahHeap::initialize() {
     Copy::fill_to_bytes(_liveness_cache[worker], _num_regions * sizeof(jushort));
   }
 
-  // The call below uses stuff (the SATB* things) that are in G1, but probably
-  // belong into a shared location.
-  ShenandoahBarrierSet::satb_mark_queue_set().initialize(this,
-                                                         20 /* G1SATBProcessCompletedThreshold */,
-                                                         60 /* G1SATBBufferEnqueueingThresholdPercent */);
+  // There should probably be Shenandoah-specific options for these,
+  // just as there are G1-specific options.
+  {
+    ShenandoahSATBMarkQueueSet& satbqs = ShenandoahBarrierSet::satb_mark_queue_set();
+    satbqs.set_process_completed_buffers_threshold(20); // G1SATBProcessCompletedThreshold
+    satbqs.set_buffer_enqueue_threshold_percentage(60); // G1SATBBufferEnqueueingThresholdPercent
+  }
 
   _monitoring_support = new ShenandoahMonitoringSupport(this);
   _phase_timings = new ShenandoahPhaseTimings();
diff --git a/src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.cpp b/src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.cpp
@@ -27,20 +27,10 @@
 #include "gc/shenandoah/shenandoahSATBMarkQueueSet.hpp"
 #include "gc/shenandoah/shenandoahThreadLocalData.hpp"
 
-ShenandoahSATBMarkQueueSet::ShenandoahSATBMarkQueueSet() :
-  _heap(NULL),
-  _satb_mark_queue_buffer_allocator("SATB Buffer Allocator", ShenandoahSATBBufferSize)
+ShenandoahSATBMarkQueueSet::ShenandoahSATBMarkQueueSet(BufferNode::Allocator* allocator) :
+  SATBMarkQueueSet(allocator)
 {}
 
-void ShenandoahSATBMarkQueueSet::initialize(ShenandoahHeap* const heap,
-                                            int process_completed_threshold,
-                                            uint buffer_enqueue_threshold_percentage) {
-  SATBMarkQueueSet::initialize(&_satb_mark_queue_buffer_allocator,
-                               process_completed_threshold,
-                               buffer_enqueue_threshold_percentage);
-  _heap = heap;
-}
-
 SATBMarkQueue& ShenandoahSATBMarkQueueSet::satb_queue_for_thread(Thread* const t) const {
   return ShenandoahThreadLocalData::satb_mark_queue(t);
 }
@@ -60,11 +50,11 @@ class ShenandoahSATBMarkQueueFilterFn {
 };
 
 void ShenandoahSATBMarkQueueSet::filter(SATBMarkQueue* queue) {
-  assert(_heap != NULL, "SATB queue set not initialized");
-  if (_heap->has_forwarded_objects()) {
-    apply_filter(ShenandoahSATBMarkQueueFilterFn<true>(_heap), queue);
+  ShenandoahHeap* heap = ShenandoahHeap::heap();
+  if (heap->has_forwarded_objects()) {
+    apply_filter(ShenandoahSATBMarkQueueFilterFn<true>(heap), queue);
   } else {
-    apply_filter(ShenandoahSATBMarkQueueFilterFn<false>(_heap), queue);
+    apply_filter(ShenandoahSATBMarkQueueFilterFn<false>(heap), queue);
   }
 }
 
diff --git a/src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.hpp b/src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.hpp

Original file line number	Diff line number	Diff line change
`@@ -82,9 +82,6 @@ class G1BarrierSet: public CardTableBarrierSet {`
`82`	`82`	`virtual void on_thread_attach(Thread* thread);`
`83`	`83`	`virtual void on_thread_detach(Thread* thread);`
`84`	`84`
`85`		`- BufferNode::Allocator& satb_mark_queue_buffer_allocator();`
`86`		`- BufferNode::Allocator& dirty_card_queue_buffer_allocator();`
`87`		`-`
`88`	`85`	`static G1SATBMarkQueueSet& satb_mark_queue_set() {`
`89`	`86`	`return g1_barrier_set()->_satb_mark_queue_set;`
`90`	`87`	`}`