8221360: Eliminate Shared_DirtyCardQ_lock

Reviewed-by: tschatzl, iwalulya

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

@@ -58,8 +58,7 @@ G1BarrierSet::G1BarrierSet(G1CardTable* card_table) :
   _satb_mark_queue_buffer_allocator("SATB Buffer Allocator", G1SATBBufferSize),
   _dirty_card_queue_buffer_allocator("DC Buffer Allocator", G1UpdateBufferSize),
   _satb_mark_queue_set(&_satb_mark_queue_buffer_allocator),
-  _dirty_card_queue_set(&_dirty_card_queue_buffer_allocator),
+  _dirty_card_queue_set(&_dirty_card_queue_buffer_allocator)
-  _shared_dirty_card_queue(&_dirty_card_queue_set)
 {}
 
 void G1BarrierSet::enqueue(oop pre_val) {

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

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2001, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2001, 2021, 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
@@ -27,7 +27,6 @@
 
 #include "gc/g1/g1DirtyCardQueue.hpp"
 #include "gc/g1/g1SATBMarkQueueSet.hpp"
-#include "gc/g1/g1SharedDirtyCardQueue.hpp"
 #include "gc/shared/cardTable.hpp"
 #include "gc/shared/cardTableBarrierSet.hpp"
 
@@ -43,7 +42,6 @@ class G1BarrierSet: public CardTableBarrierSet {
   BufferNode::Allocator _dirty_card_queue_buffer_allocator;
   G1SATBMarkQueueSet _satb_mark_queue_set;
   G1DirtyCardQueueSet _dirty_card_queue_set;
-  G1SharedDirtyCardQueue _shared_dirty_card_queue;
 
   static G1BarrierSet* g1_barrier_set() {
     return barrier_set_cast<G1BarrierSet>(BarrierSet::barrier_set());
@@ -94,10 +92,6 @@ class G1BarrierSet: public CardTableBarrierSet {
     return g1_barrier_set()->_dirty_card_queue_set;
   }
 
-  static G1SharedDirtyCardQueue& shared_dirty_card_queue() {
-    return g1_barrier_set()->_shared_dirty_card_queue;
-  }
-
   // Callbacks for runtime accesses.
   template <DecoratorSet decorators, typename BarrierSetT = G1BarrierSet>
   class AccessBarrier: public ModRefBarrierSet::AccessBarrier<decorators, BarrierSetT> {

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

@@ -546,8 +546,6 @@ void G1DirtyCardQueueSet::abandon_logs() {
     }
   } closure(*this);
   Threads::threads_do(&closure);
-
-  G1BarrierSet::shared_dirty_card_queue().reset();
 }
 
 void G1DirtyCardQueueSet::concatenate_logs() {
@@ -571,7 +569,6 @@ void G1DirtyCardQueueSet::concatenate_logs() {
   } closure(*this);
   Threads::threads_do(&closure);
 
-  G1BarrierSet::shared_dirty_card_queue().flush();
   enqueue_all_paused_buffers();
   verify_num_cards();
   set_max_cards(old_limit);

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

@@ -41,7 +41,6 @@
 #include "gc/g1/g1RootClosures.hpp"
 #include "gc/g1/g1RemSet.hpp"
 #include "gc/g1/g1ServiceThread.hpp"
-#include "gc/g1/g1SharedDirtyCardQueue.hpp"
 #include "gc/g1/g1_globals.hpp"
 #include "gc/g1/heapRegion.inline.hpp"
 #include "gc/g1/heapRegionManager.inline.hpp"
@@ -1688,12 +1687,27 @@ void G1RemSet::refine_card_concurrently(CardValue* const card_ptr,
     return;
   }
 
-  // Re-dirty the card and enqueue in the *shared* queue.  Can't use
+  enqueue_for_reprocessing(card_ptr);
-  // the thread-local queue, because that might be the queue that is
+}
-  // being processed by us; we could be a Java thread conscripted to
+
-  // perform refinement on our queue's current buffer.
+// Re-dirty and re-enqueue the card to retry refinement later.
+// This is used to deal with a rare race condition in concurrent refinement.
+void G1RemSet::enqueue_for_reprocessing(CardValue* card_ptr) {
+  // We can't use the thread-local queue, because that might be the queue
+  // that is being processed by us; we could be a Java thread conscripted to
+  // perform refinement on our queue's current buffer.  This situation only
+  // arises from rare race condition, so it's not worth any significant
+  // development effort or clever lock-free queue implementation.  Instead
+  // we use brute force, allocating and enqueuing an entire buffer for just
+  // this card.  Since buffers are processed in FIFO order and we try to
+  // keep some in the queue, it is likely that the racing state will have
+  // resolved by the time this card comes up for reprocessing.
   *card_ptr = G1CardTable::dirty_card_val();
-  G1BarrierSet::shared_dirty_card_queue().enqueue(card_ptr);
+  G1DirtyCardQueueSet& dcqs = G1BarrierSet::dirty_card_queue_set();
+  void** buffer = dcqs.allocate_buffer();
+  size_t index = dcqs.buffer_size() - 1;
+  buffer[index] = card_ptr;
+  dcqs.enqueue_completed_buffer(BufferNode::make_node_from_buffer(buffer, index));
 }
 
 void G1RemSet::print_periodic_summary_info(const char* header, uint period_count) {

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

@@ -57,6 +57,9 @@ class HeapRegionClaimer;
 // external heap references into it.  Uses a mod ref bs to track updates,
 // so that they can be used to update the individual region remsets.
 class G1RemSet: public CHeapObj<mtGC> {
+public:
+  typedef CardTable::CardValue CardValue;
+
 private:
   G1RemSetScanState* _scan_state;
 
@@ -72,10 +75,10 @@ private:
   void print_merge_heap_roots_stats();
 
   void assert_scan_top_is_null(uint hrm_index) NOT_DEBUG_RETURN;
+
+  void enqueue_for_reprocessing(CardValue* card_ptr);
+
 public:
-
-  typedef CardTable::CardValue CardValue;
-
   // Initialize data that depends on the heap size being known.
   void initialize(uint max_reserved_regions);
 

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

@@ -1,73 +0,0 @@
-/*
- * Copyright (c) 2019, 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
- * under the terms of the GNU General Public License version 2 only, as
- * published by the Free Software Foundation.
- *
- * This code is distributed in the hope that it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
- * version 2 for more details (a copy is included in the LICENSE file that
- * accompanied this code).
- *
- * You should have received a copy of the GNU General Public License version
- * 2 along with this work; if not, write to the Free Software Foundation,
- * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
- *
- * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
- * or visit www.oracle.com if you need additional information or have any
- * questions.
- *
- */
-
-#include "precompiled.hpp"
-#include "gc/g1/g1DirtyCardQueue.hpp"
-#include "gc/g1/g1SharedDirtyCardQueue.hpp"
-#include "gc/shared/ptrQueue.hpp"
-#include "runtime/mutex.hpp"
-#include "runtime/mutexLocker.hpp"
-
-G1SharedDirtyCardQueue::G1SharedDirtyCardQueue(G1DirtyCardQueueSet* qset) :
-  _qset(qset),
-  _buffer(NULL),
-  _index(0)
-{}
-
-G1SharedDirtyCardQueue::~G1SharedDirtyCardQueue() {
-  flush();
-}
-
-void G1SharedDirtyCardQueue::enqueue(void* card_ptr) {
-  MutexLocker ml(Shared_DirtyCardQ_lock, Mutex::_no_safepoint_check_flag);
-  if (_index == 0) {
-    flush();
-    _buffer = _qset->allocate_buffer();
-    _index = _qset->buffer_size();
-    assert(_index != 0, "invariant");
-  }
-  _buffer[--_index] = card_ptr;
-}
-
-void G1SharedDirtyCardQueue::flush() {
-  if (_buffer != NULL) {
-    BufferNode* node = BufferNode::make_node_from_buffer(_buffer, _index);
-    _buffer = NULL;
-    _index = 0;
-    if (node->index() == _qset->buffer_size()) {
-      _qset->deallocate_buffer(node);
-    } else {
-      _qset->enqueue_completed_buffer(node);
-    }
-  }
-  assert(_index == 0, "invariant");
-}
-
-void G1SharedDirtyCardQueue::reset() {
-  if (_buffer == NULL) {
-    _index = 0;
-  } else {
-    _index = _qset->buffer_size();
-  }
-}

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

@@ -1,58 +0,0 @@
-/*
- * Copyright (c) 2019, 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
- * under the terms of the GNU General Public License version 2 only, as
- * published by the Free Software Foundation.
- *
- * This code is distributed in the hope that it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
- * version 2 for more details (a copy is included in the LICENSE file that
- * accompanied this code).
- *
- * You should have received a copy of the GNU General Public License version
- * 2 along with this work; if not, write to the Free Software Foundation,
- * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
- *
- * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
- * or visit www.oracle.com if you need additional information or have any
- * questions.
- *
- */
-
-#ifndef SHARE_GC_G1_G1SHAREDDIRTYCARDQUEUE_HPP
-#define SHARE_GC_G1_G1SHAREDDIRTYCARDQUEUE_HPP
-
-#include "utilities/globalDefinitions.hpp"
-
-class G1DirtyCardQueueSet;
-
-// A dirty card queue providing thread-safe enqueue.  A shared global
-// instance can be used for cases where a thread-local dirty card can't
-// be used.
-class G1SharedDirtyCardQueue {
-  G1DirtyCardQueueSet* const _qset;
-  void** _buffer;
-  size_t _index;
-
-  NONCOPYABLE(G1SharedDirtyCardQueue);
-
-public:
-  G1SharedDirtyCardQueue(G1DirtyCardQueueSet* qset);
-  ~G1SharedDirtyCardQueue();    // flushes the queue.
-
-  // Thread-safe addition to shared logging buffer.
-  void enqueue(void* card_ptr);
-
-  // Flush any pending entries to the qset and remove the buffer.
-  // Not thread-safe.
-  void flush();
-
-  // Discard any pending entries.
-  // Not thread-safe.
-  void reset();
-};
-
-#endif // SHARE_GC_G1_G1SHAREDDIRTYCARDQUEUE_HPP

src/hotspot/share/runtime/mutexLocker.cpp

@@ -74,7 +74,6 @@ Mutex*   NonJavaThreadsListSync_lock  = NULL;
 Monitor* CGC_lock                     = NULL;
 Monitor* STS_lock                     = NULL;
 Monitor* G1OldGCCount_lock            = NULL;
-Mutex*   Shared_DirtyCardQ_lock       = NULL;
 Mutex*   G1DetachedRefinementStats_lock = NULL;
 Mutex*   MarkStackFreeList_lock       = NULL;
 Mutex*   MarkStackChunkList_lock      = NULL;
@@ -217,8 +216,6 @@ void mutex_init() {
   if (UseG1GC) {
     def(G1OldGCCount_lock          , PaddedMonitor, leaf,        true,  _safepoint_check_always);
 
-    def(Shared_DirtyCardQ_lock     , PaddedMutex  , access + 1,  true,  _safepoint_check_never);
-
     def(G1DetachedRefinementStats_lock, PaddedMutex, leaf    ,   true, _safepoint_check_never);
 
     def(FreeList_lock              , PaddedMutex  , leaf     ,   true,  _safepoint_check_never);

src/hotspot/share/runtime/mutexLocker.hpp

@@ -68,9 +68,6 @@ extern Monitor* CGC_lock;                        // used for coordination betwee
                                                  // fore- & background GC threads.
 extern Monitor* STS_lock;                        // used for joining/leaving SuspendibleThreadSet.
 extern Monitor* G1OldGCCount_lock;               // in support of "concurrent" full gc
-extern Mutex*   Shared_DirtyCardQ_lock;          // Lock protecting dirty card
-                                                 // queue shared by
-                                                 // non-Java threads.
 extern Mutex*   G1DetachedRefinementStats_lock;  // Lock protecting detached refinement stats
 extern Mutex*   MarkStackFreeList_lock;          // Protects access to the global mark stack free list.
 extern Mutex*   MarkStackChunkList_lock;         // Protects access to the global mark stack chunk list.