8258255: Move PtrQueue active flag to SATBMarkQueue

Reviewed-by: tschatzl, sjohanss

Author: Kim Barrett

src/hotspot/share/gc/g1/g1BarrierSet.cpp

@@ -144,7 +144,6 @@ void G1BarrierSet::on_thread_destroy(Thread* thread) {
 void G1BarrierSet::on_thread_attach(Thread* thread) {
   assert(!G1ThreadLocalData::satb_mark_queue(thread).is_active(), "SATB queue should not be active");
   assert(G1ThreadLocalData::satb_mark_queue(thread).is_empty(), "SATB queue should be empty");
-  assert(G1ThreadLocalData::dirty_card_queue(thread).is_active(), "Dirty card queue should be active");
   // Can't assert that the DCQ is empty.  There is early execution on
   // the main thread, before it gets added to the threads list, which
   // is where this is called.  That execution may enqueue dirty cards.

src/hotspot/share/gc/g1/g1DirtyCardQueue.cpp

@@ -50,9 +50,7 @@
 #include "utilities/ticks.hpp"
 
 G1DirtyCardQueue::G1DirtyCardQueue(G1DirtyCardQueueSet* qset) :
-  // Dirty card queues are always active, so we create them with their
-  // active field set to true.
-  PtrQueue(qset, true /* active */),
+  PtrQueue(qset),
   _refinement_stats(new G1ConcurrentRefineStats())
 { }
 
@@ -86,9 +84,7 @@ G1DirtyCardQueueSet::G1DirtyCardQueueSet(BufferNode::Allocator* allocator) :
   _max_cards(MaxCardsUnlimited),
   _padded_max_cards(MaxCardsUnlimited),
   _detached_refinement_stats()
-{
-  _all_active = true;
-}
+{}
 
 G1DirtyCardQueueSet::~G1DirtyCardQueueSet() {
   abandon_completed_buffers();

src/hotspot/share/gc/g1/g1RedirtyCardsQueue.cpp

@@ -71,7 +71,7 @@ void G1RedirtyCardsLocalQueueSet::flush() {
 // G1RedirtyCardsQueue
 
 G1RedirtyCardsQueue::G1RedirtyCardsQueue(G1RedirtyCardsLocalQueueSet* qset) :
-  PtrQueue(qset, true /* always active */)
+  PtrQueue(qset)
 {}
 
 #ifdef ASSERT

src/hotspot/share/gc/g1/vmStructs_g1.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2011, 2020, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -70,7 +70,7 @@
                                                                               \
   nonstatic_field(HeapRegionSetBase,   _length,         uint)                 \
                                                                               \
-  nonstatic_field(PtrQueue,            _active,         bool)                 \
+  nonstatic_field(SATBMarkQueue,       _active,         bool)                 \
   nonstatic_field(PtrQueue,            _buf,            void**)               \
   nonstatic_field(PtrQueue,            _index,          size_t)
 

src/hotspot/share/gc/shared/ptrQueue.cpp

@@ -35,9 +35,8 @@
 
 #include <new>
 
-PtrQueue::PtrQueue(PtrQueueSet* qset, bool active) :
+PtrQueue::PtrQueue(PtrQueueSet* qset) :
   _qset(qset),
-  _active(active),
   _index(0),
   _capacity_in_bytes(index_to_byte_index(qset->buffer_size())),
   _buf(NULL)
@@ -221,8 +220,7 @@ size_t BufferNode::Allocator::reduce_free_list(size_t remove_goal) {
 }
 
 PtrQueueSet::PtrQueueSet(BufferNode::Allocator* allocator) :
-  _allocator(allocator),
-  _all_active(false)
+  _allocator(allocator)
 {}
 
 PtrQueueSet::~PtrQueueSet() {}

src/hotspot/share/gc/shared/ptrQueue.hpp

@@ -47,9 +47,6 @@ class PtrQueue {
   // The ptr queue set to which this queue belongs.
   PtrQueueSet* const _qset;
 
-  // Whether updates should be logged.
-  bool _active;
-
   // The (byte) index at which an object was last enqueued.  Starts at
   // capacity_in_bytes (indicating an empty buffer) and goes towards zero.
   // Value is always pointer-size aligned.
@@ -92,7 +89,7 @@ class PtrQueue {
 
   // Initialize this queue to contain a null buffer, and be part of the
   // given PtrQueueSet.
-  PtrQueue(PtrQueueSet* qset, bool active = false);
+  PtrQueue(PtrQueueSet* qset);
 
   // Requires queue flushed.
   ~PtrQueue();
@@ -145,21 +142,6 @@ class PtrQueue {
     return _buf == NULL || capacity_in_bytes() == _index;
   }
 
-  // Set the "active" property of the queue to "b".  An enqueue to an
-  // inactive thread is a no-op.  Setting a queue to inactive resets its
-  // log to the empty state.
-  void set_active(bool b) {
-    _active = b;
-    if (!b && _buf != NULL) {
-      reset();
-    } else if (b && _buf != NULL) {
-      assert(index() == capacity(),
-             "invariant: queues are empty when activated.");
-    }
-  }
-
-  bool is_active() const { return _active; }
-
   // To support compiler.
 
 protected:
@@ -176,14 +158,6 @@ class PtrQueue {
   }
 
   static ByteSize byte_width_of_buf() { return in_ByteSize(_element_size); }
-
-  template<typename Derived>
-  static ByteSize byte_offset_of_active() {
-    return byte_offset_of(Derived, _active);
-  }
-
-  static ByteSize byte_width_of_active() { return in_ByteSize(sizeof(bool)); }
-
 };
 
 class BufferNode {
@@ -290,8 +264,6 @@ class PtrQueueSet {
   NONCOPYABLE(PtrQueueSet);
 
 protected:
-  bool _all_active;
-
   // Create an empty ptr queue set.
   PtrQueueSet(BufferNode::Allocator* allocator);
   ~PtrQueueSet();
@@ -329,8 +301,6 @@ class PtrQueueSet {
   // Adds node to the completed buffer list.
   virtual void enqueue_completed_buffer(BufferNode* node) = 0;
 
-  bool is_active() { return _all_active; }
-
   size_t buffer_size() const {
     return _allocator->buffer_size();
   }

src/hotspot/share/gc/shared/satbMarkQueue.cpp

@@ -38,13 +38,13 @@
 #include "utilities/globalCounter.inline.hpp"
 
 SATBMarkQueue::SATBMarkQueue(SATBMarkQueueSet* qset) :
-  // SATB queues are only active during marking cycles. We create
-  // them with their active field set to false. If a thread is
-  // created during a cycle and its SATB queue needs to be activated
-  // before the thread starts running, we'll need to set its active
-  // field to true. This must be done in the collector-specific
+  PtrQueue(qset),
+  // SATB queues are only active during marking cycles. We create them
+  // with their active field set to false. If a thread is created
+  // during a cycle, it's SATB queue needs to be activated before the
+  // thread starts running.  This is handled by the collector-specific
   // BarrierSet thread attachment protocol.
-  PtrQueue(qset, false /* active */)
+  _active(false)
 { }
 
 void SATBMarkQueue::flush() {
@@ -86,7 +86,8 @@ SATBMarkQueueSet::SATBMarkQueueSet(BufferNode::Allocator* allocator) :
   _list(),
   _count_and_process_flag(0),
   _process_completed_buffers_threshold(SIZE_MAX),
-  _buffer_enqueue_threshold(0)
+  _buffer_enqueue_threshold(0),
+  _all_active(false)
 {}
 
 SATBMarkQueueSet::~SATBMarkQueueSet() {
@@ -207,7 +208,13 @@ void SATBMarkQueueSet::set_active_all_threads(bool active, bool expected_active)
     SetThreadActiveClosure(SATBMarkQueueSet* qset, bool active) :
       _qset(qset), _active(active) {}
     virtual void do_thread(Thread* t) {
-      _qset->satb_queue_for_thread(t).set_active(_active);
+      SATBMarkQueue& queue = _qset->satb_queue_for_thread(t);
+      if (queue.buffer() != nullptr) {
+        assert(!_active || queue.index() == _qset->buffer_size(),
+               "queues should be empty when activated");
+        queue.set_index(_qset->buffer_size());
+      }
+      queue.set_active(_active);
     }
   } closure(this, active);
   Threads::threads_do(&closure);

src/hotspot/share/gc/shared/satbMarkQueue.hpp

@@ -46,15 +46,23 @@ class SATBBufferClosure : public StackObj {
 
 // A PtrQueue whose elements are (possibly stale) pointers to object heads.
 class SATBMarkQueue: public PtrQueue {
+  friend class VMStructs;
   friend class SATBMarkQueueSet;
 
 private:
+  // Per-queue (so thread-local) cache of the SATBMarkQueueSet's
+  // active state, to support inline barriers in compiled code.
+  bool _active;
+
   // Filter out unwanted entries from the buffer.
   inline void filter();
 
 public:
   SATBMarkQueue(SATBMarkQueueSet* qset);
 
+  bool is_active() const { return _active; }
+  void set_active(bool value) { _active = value; }
+
   // Process queue entries and free resources.
   void flush();
 
@@ -81,10 +89,10 @@ class SATBMarkQueue: public PtrQueue {
   using PtrQueue::byte_width_of_buf;
 
   static ByteSize byte_offset_of_active() {
-    return PtrQueue::byte_offset_of_active<SATBMarkQueue>();
+    return byte_offset_of(SATBMarkQueue, _active);
   }
-  using PtrQueue::byte_width_of_active;
 
+  static ByteSize byte_width_of_active() { return in_ByteSize(sizeof(bool)); }
 };
 
 class SATBMarkQueueSet: public PtrQueueSet {
@@ -95,7 +103,9 @@ class SATBMarkQueueSet: public PtrQueueSet {
   // These are rarely (if ever) changed, so same cache line as count.
   size_t _process_completed_buffers_threshold;
   size_t _buffer_enqueue_threshold;
-  DEFINE_PAD_MINUS_SIZE(2, DEFAULT_CACHE_LINE_SIZE, 3 * sizeof(size_t));
+  // SATB is only active during marking.  Enqueuing is only done when active.
+  bool _all_active;
+  DEFINE_PAD_MINUS_SIZE(2, DEFAULT_CACHE_LINE_SIZE, 4 * sizeof(size_t));
 
   BufferNode* get_completed_buffer();
   void abandon_completed_buffers();
@@ -121,6 +131,8 @@ class SATBMarkQueueSet: public PtrQueueSet {
 public:
   virtual SATBMarkQueue& satb_queue_for_thread(Thread* const t) const = 0;
 
+  bool is_active() const { return _all_active; }
+
   // Apply "set_active(active)" to all SATB queues in the set. 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
@@ -138,9 +150,11 @@ class SATBMarkQueueSet: public PtrQueueSet {
   // buffer; the leading entries may be excluded due to filtering.
   bool apply_closure_to_completed_buffer(SATBBufferClosure* cl);
 
+  // When active, add obj to queue by calling enqueue_known_active.
   void enqueue(SATBMarkQueue& queue, oop obj) {
     if (queue.is_active()) enqueue_known_active(queue, obj);
   }
+  // Add obj to queue.  This qset and the queue must be active.
   void enqueue_known_active(SATBMarkQueue& queue, oop obj);
   virtual void filter(SATBMarkQueue& queue) = 0;
   virtual void enqueue_completed_buffer(BufferNode* node);