8227719: G1 Pending cards estimation too conservative in cost prediction
authortschatzl
Wed, 24 Jul 2019 11:49:39 +0200
changeset 57507 f6b30bd6804e
parent 57506 36e4e50b4255
child 57508 28ab01c06755
8227719: G1 Pending cards estimation too conservative in cost prediction Summary: Instead of using a coarse prediction for the log buffers, accumulate the actual number directly. Reviewed-by: kbarrett, sangheki
src/hotspot/share/gc/g1/g1CollectedHeap.cpp
src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp
src/hotspot/share/gc/g1/g1ConcurrentRefineThread.cpp
src/hotspot/share/gc/g1/g1DirtyCardQueue.cpp
src/hotspot/share/gc/g1/g1DirtyCardQueue.hpp
src/hotspot/share/gc/g1/g1RedirtyCardsQueue.cpp
src/hotspot/share/gc/g1/g1RedirtyCardsQueue.hpp
--- a/src/hotspot/share/gc/g1/g1CollectedHeap.cpp	Wed Jul 24 11:49:39 2019 +0200
+++ b/src/hotspot/share/gc/g1/g1CollectedHeap.cpp	Wed Jul 24 11:49:39 2019 +0200
@@ -1080,7 +1080,7 @@
 
   // Discard all remembered set updates.
   G1BarrierSet::dirty_card_queue_set().abandon_logs();
-  assert(G1BarrierSet::dirty_card_queue_set().completed_buffers_num() == 0,
+  assert(G1BarrierSet::dirty_card_queue_set().num_completed_buffers() == 0,
          "DCQS should be empty");
   redirty_cards_queue_set().verify_empty();
 }
@@ -1957,7 +1957,7 @@
   while (dcqs.apply_closure_during_gc(cl, worker_i)) {
     n_completed_buffers++;
   }
-  assert(dcqs.completed_buffers_num() == 0, "Completed buffers exist!");
+  assert(dcqs.num_completed_buffers() == 0, "Completed buffers exist!");
   phase_times()->record_thread_work_item(G1GCPhaseTimes::MergeLB, worker_i, n_completed_buffers, G1GCPhaseTimes::MergeLBProcessedBuffers);
 }
 
@@ -2613,10 +2613,9 @@
   Threads::threads_do(&count_from_threads);
 
   G1DirtyCardQueueSet& dcqs = G1BarrierSet::dirty_card_queue_set();
-  size_t buffer_size = dcqs.buffer_size();
-  size_t buffer_num = dcqs.completed_buffers_num();
-
-  return buffer_size * buffer_num + count_from_threads._cards;
+  dcqs.verify_num_entries_in_completed_buffers();
+
+  return dcqs.num_entries_in_completed_buffers() + count_from_threads._cards;
 }
 
 bool G1CollectedHeap::is_potential_eager_reclaim_candidate(HeapRegion* r) const {
--- a/src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp	Wed Jul 24 11:49:39 2019 +0200
+++ b/src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp	Wed Jul 24 11:49:39 2019 +0200
@@ -397,7 +397,7 @@
     dcqs.set_max_completed_buffers(red_zone());
   }
 
-  size_t curr_queue_size = dcqs.completed_buffers_num();
+  size_t curr_queue_size = dcqs.num_completed_buffers();
   if ((dcqs.max_completed_buffers() > 0) &&
       (curr_queue_size >= yellow_zone())) {
     dcqs.set_completed_buffers_padding(curr_queue_size);
@@ -430,7 +430,7 @@
 bool G1ConcurrentRefine::do_refinement_step(uint worker_id) {
   G1DirtyCardQueueSet& dcqs = G1BarrierSet::dirty_card_queue_set();
 
-  size_t curr_buffer_num = dcqs.completed_buffers_num();
+  size_t curr_buffer_num = dcqs.num_completed_buffers();
   // If the number of the buffers falls down into the yellow zone,
   // that means that the transition period after the evacuation pause has ended.
   // Since the value written to the DCQS is the same for all threads, there is no
--- a/src/hotspot/share/gc/g1/g1ConcurrentRefineThread.cpp	Wed Jul 24 11:49:39 2019 +0200
+++ b/src/hotspot/share/gc/g1/g1ConcurrentRefineThread.cpp	Wed Jul 24 11:49:39 2019 +0200
@@ -104,7 +104,7 @@
     size_t buffers_processed = 0;
     log_debug(gc, refine)("Activated worker %d, on threshold: " SIZE_FORMAT ", current: " SIZE_FORMAT,
                           _worker_id, _cr->activation_threshold(_worker_id),
-                           G1BarrierSet::dirty_card_queue_set().completed_buffers_num());
+                           G1BarrierSet::dirty_card_queue_set().num_completed_buffers());
 
     {
       SuspendibleThreadSetJoiner sts_join;
@@ -126,7 +126,7 @@
     log_debug(gc, refine)("Deactivated worker %d, off threshold: " SIZE_FORMAT
                           ", current: " SIZE_FORMAT ", processed: " SIZE_FORMAT,
                           _worker_id, _cr->deactivation_threshold(_worker_id),
-                          G1BarrierSet::dirty_card_queue_set().completed_buffers_num(),
+                          G1BarrierSet::dirty_card_queue_set().num_completed_buffers(),
                           buffers_processed);
 
     if (os::supports_vtime()) {
--- a/src/hotspot/share/gc/g1/g1DirtyCardQueue.cpp	Wed Jul 24 11:49:39 2019 +0200
+++ b/src/hotspot/share/gc/g1/g1DirtyCardQueue.cpp	Wed Jul 24 11:49:39 2019 +0200
@@ -84,7 +84,7 @@
   _cbl_mon(NULL),
   _completed_buffers_head(NULL),
   _completed_buffers_tail(NULL),
-  _n_completed_buffers(0),
+  _num_entries_in_completed_buffers(0),
   _process_completed_buffers_threshold(ProcessCompletedBuffersThresholdNever),
   _process_completed_buffers(false),
   _notify_when_complete(notify_when_complete),
@@ -133,42 +133,56 @@
     _completed_buffers_tail->set_next(cbn);
     _completed_buffers_tail = cbn;
   }
-  _n_completed_buffers++;
+  _num_entries_in_completed_buffers += buffer_size() - cbn->index();
 
   if (!process_completed_buffers() &&
-      (_n_completed_buffers > process_completed_buffers_threshold())) {
+      (num_completed_buffers() > process_completed_buffers_threshold())) {
     set_process_completed_buffers(true);
     if (_notify_when_complete) {
       _cbl_mon->notify_all();
     }
   }
-  assert_completed_buffers_list_len_correct_locked();
+  verify_num_entries_in_completed_buffers();
 }
 
 BufferNode* G1DirtyCardQueueSet::get_completed_buffer(size_t stop_at) {
   MutexLocker x(_cbl_mon, Mutex::_no_safepoint_check_flag);
 
-  if (_n_completed_buffers <= stop_at) {
+  if (num_completed_buffers() <= stop_at) {
     return NULL;
   }
 
-  assert(_n_completed_buffers > 0, "invariant");
+  assert(num_completed_buffers() > 0, "invariant");
   assert(_completed_buffers_head != NULL, "invariant");
   assert(_completed_buffers_tail != NULL, "invariant");
 
   BufferNode* bn = _completed_buffers_head;
-  _n_completed_buffers--;
+  _num_entries_in_completed_buffers -= buffer_size() - bn->index();
   _completed_buffers_head = bn->next();
   if (_completed_buffers_head == NULL) {
-    assert(_n_completed_buffers == 0, "invariant");
+    assert(num_completed_buffers() == 0, "invariant");
     _completed_buffers_tail = NULL;
     set_process_completed_buffers(false);
   }
-  assert_completed_buffers_list_len_correct_locked();
+  verify_num_entries_in_completed_buffers();
   bn->set_next(NULL);
   return bn;
 }
 
+#ifdef ASSERT
+void G1DirtyCardQueueSet::verify_num_entries_in_completed_buffers() const {
+  size_t actual = 0;
+  BufferNode* cur = _completed_buffers_head;
+  while (cur != NULL) {
+    actual += buffer_size() - cur->index();
+    cur = cur->next();
+  }
+  assert(actual == _num_entries_in_completed_buffers,
+         "Num entries in completed buffers should be " SIZE_FORMAT " but are " SIZE_FORMAT,
+         _num_entries_in_completed_buffers, actual);
+}
+#endif
+
 void G1DirtyCardQueueSet::abandon_completed_buffers() {
   BufferNode* buffers_to_delete = NULL;
   {
@@ -176,7 +190,7 @@
     buffers_to_delete = _completed_buffers_head;
     _completed_buffers_head = NULL;
     _completed_buffers_tail = NULL;
-    _n_completed_buffers = 0;
+    _num_entries_in_completed_buffers = 0;
     set_process_completed_buffers(false);
   }
   while (buffers_to_delete != NULL) {
@@ -189,26 +203,13 @@
 
 void G1DirtyCardQueueSet::notify_if_necessary() {
   MutexLocker x(_cbl_mon, Mutex::_no_safepoint_check_flag);
-  if (_n_completed_buffers > process_completed_buffers_threshold()) {
+  if (num_completed_buffers() > process_completed_buffers_threshold()) {
     set_process_completed_buffers(true);
     if (_notify_when_complete)
       _cbl_mon->notify();
   }
 }
 
-#ifdef ASSERT
-void G1DirtyCardQueueSet::assert_completed_buffers_list_len_correct_locked() {
-  assert_lock_strong(_cbl_mon);
-  size_t n = 0;
-  for (BufferNode* bn = _completed_buffers_head; bn != NULL; bn = bn->next()) {
-    ++n;
-  }
-  assert(n == _n_completed_buffers,
-         "Completed buffer length is wrong: counted: " SIZE_FORMAT
-         ", expected: " SIZE_FORMAT, n, _n_completed_buffers);
-}
-#endif // ASSERT
-
 // Merge lists of buffers. Notify the processing threads.
 // The source queue is emptied as a result. The queues
 // must share the monitor.
@@ -227,12 +228,12 @@
     _completed_buffers_tail->set_next(from._head);
     _completed_buffers_tail = from._tail;
   }
-  _n_completed_buffers += from._count;
+  _num_entries_in_completed_buffers += from._entry_count;
 
   assert(_completed_buffers_head == NULL && _completed_buffers_tail == NULL ||
          _completed_buffers_head != NULL && _completed_buffers_tail != NULL,
          "Sanity");
-  assert_completed_buffers_list_len_correct_locked();
+  verify_num_entries_in_completed_buffers();
 }
 
 bool G1DirtyCardQueueSet::apply_closure_to_buffer(G1CardTableEntryClosure* cl,
@@ -278,7 +279,7 @@
     // add of padding could overflow, which is treated as unlimited.
     size_t max_buffers = max_completed_buffers();
     size_t limit = max_buffers + completed_buffers_padding();
-    if ((completed_buffers_num() > limit) && (limit >= max_buffers)) {
+    if ((num_completed_buffers() > limit) && (limit >= max_buffers)) {
       if (mut_process_buffer(node)) {
         return true;
       }
--- a/src/hotspot/share/gc/g1/g1DirtyCardQueue.hpp	Wed Jul 24 11:49:39 2019 +0200
+++ b/src/hotspot/share/gc/g1/g1DirtyCardQueue.hpp	Wed Jul 24 11:49:39 2019 +0200
@@ -69,7 +69,9 @@
   Monitor* _cbl_mon;  // Protects the fields below.
   BufferNode* _completed_buffers_head;
   BufferNode* _completed_buffers_tail;
-  volatile size_t _n_completed_buffers;
+
+  // Number of actual entries in the list of completed buffers.
+  volatile size_t _num_entries_in_completed_buffers;
 
   size_t _process_completed_buffers_threshold;
   volatile bool _process_completed_buffers;
@@ -77,8 +79,6 @@
   // If true, notify_all on _cbl_mon when the threshold is reached.
   bool _notify_when_complete;
 
-  void assert_completed_buffers_list_len_correct_locked() NOT_DEBUG_RETURN;
-
   void abandon_completed_buffers();
 
   // Apply the closure to the elements of "node" from it's index to
@@ -150,8 +150,17 @@
   // return a completed buffer from the list.  Otherwise, return NULL.
   BufferNode* get_completed_buffer(size_t stop_at = 0);
 
-  // The number of buffers in the list.  Racy...
-  size_t completed_buffers_num() const { return _n_completed_buffers; }
+  // The number of buffers in the list. Derived as an approximation from the number
+  // of entries in the buffers. Racy.
+  size_t num_completed_buffers() const {
+    return (num_entries_in_completed_buffers() + buffer_size() - 1) / buffer_size();
+  }
+  // The number of entries in completed buffers. Read without synchronization.
+  size_t num_entries_in_completed_buffers() const { return _num_entries_in_completed_buffers; }
+
+  // Verify that _num_entries_in_completed_buffers is equal to the sum of actual entries
+  // in the completed buffers.
+  void verify_num_entries_in_completed_buffers() const NOT_DEBUG_RETURN;
 
   bool process_completed_buffers() { return _process_completed_buffers; }
   void set_process_completed_buffers(bool x) { _process_completed_buffers = x; }
--- a/src/hotspot/share/gc/g1/g1RedirtyCardsQueue.cpp	Wed Jul 24 11:49:39 2019 +0200
+++ b/src/hotspot/share/gc/g1/g1RedirtyCardsQueue.cpp	Wed Jul 24 11:49:39 2019 +0200
@@ -31,15 +31,15 @@
 // G1RedirtyCardsBufferList
 
 G1RedirtyCardsBufferList::G1RedirtyCardsBufferList() :
-  _head(NULL), _tail(NULL), _count(0) {}
+  _head(NULL), _tail(NULL), _entry_count(0) {}
 
 G1RedirtyCardsBufferList::G1RedirtyCardsBufferList(BufferNode* head,
                                                    BufferNode* tail,
-                                                   size_t count) :
-  _head(head), _tail(tail), _count(count)
+                                                   size_t entry_count) :
+  _head(head), _tail(tail), _entry_count(entry_count)
 {
   assert((_head == NULL) == (_tail == NULL), "invariant");
-  assert((_head == NULL) == (_count == 0), "invariant");
+  assert((_head == NULL) == (_entry_count == 0), "invariant");
 }
 
 // G1RedirtyCardsQueueBase::LocalQSet
@@ -55,11 +55,11 @@
 G1RedirtyCardsQueueBase::LocalQSet::~LocalQSet() {
   assert(_buffers._head == NULL, "unflushed qset");
   assert(_buffers._tail == NULL, "invariant");
-  assert(_buffers._count == 0, "invariant");
+  assert(_buffers._entry_count == 0, "invariant");
 }
 
 void G1RedirtyCardsQueueBase::LocalQSet::enqueue_completed_buffer(BufferNode* node) {
-  ++_buffers._count;
+  _buffers._entry_count += buffer_size() - node->index();
   node->set_next(_buffers._head);
   _buffers._head = node;
   if (_buffers._tail == NULL) {
@@ -67,8 +67,7 @@
   }
 }
 
-G1RedirtyCardsBufferList
-G1RedirtyCardsQueueBase::LocalQSet::take_all_completed_buffers() {
+G1RedirtyCardsBufferList G1RedirtyCardsQueueBase::LocalQSet::take_all_completed_buffers() {
   G1RedirtyCardsBufferList result = _buffers;
   _buffers = G1RedirtyCardsBufferList();
   return result;
@@ -103,7 +102,7 @@
 G1RedirtyCardsQueueSet::G1RedirtyCardsQueueSet() :
   PtrQueueSet(),
   _list(),
-  _count(0),
+  _entry_count(0),
   _tail(NULL)
   DEBUG_ONLY(COMMA _collecting(true))
 {}
@@ -116,7 +115,7 @@
 void G1RedirtyCardsQueueSet::verify_empty() const {
   assert(_list.empty(), "precondition");
   assert(_tail == NULL, "invariant");
-  assert(_count == 0, "invariant");
+  assert(_entry_count == 0, "invariant");
 }
 #endif // ASSERT
 
@@ -127,9 +126,9 @@
 
 G1RedirtyCardsBufferList G1RedirtyCardsQueueSet::take_all_completed_buffers() {
   DEBUG_ONLY(_collecting = false;)
-  G1RedirtyCardsBufferList result(_list.pop_all(), _tail, _count);
+  G1RedirtyCardsBufferList result(_list.pop_all(), _tail, _entry_count);
   _tail = NULL;
-  _count = 0;
+  _entry_count = 0;
   DEBUG_ONLY(_collecting = true;)
   return result;
 }
@@ -146,7 +145,7 @@
 
 void G1RedirtyCardsQueueSet::enqueue_completed_buffer(BufferNode* node) {
   assert(_collecting, "precondition");
-  Atomic::inc(&_count);
+  Atomic::add(buffer_size() - node->index(), &_entry_count);
   _list.push(*node);
   update_tail(node);
 }
@@ -156,7 +155,7 @@
   const G1RedirtyCardsBufferList from = src->take_all_completed_buffers();
   if (from._head != NULL) {
     assert(from._tail != NULL, "invariant");
-    Atomic::add(from._count, &_count);
+    Atomic::add(from._entry_count, &_entry_count);
     _list.prepend(*from._head, *from._tail);
     update_tail(from._tail);
   }
--- a/src/hotspot/share/gc/g1/g1RedirtyCardsQueue.hpp	Wed Jul 24 11:49:39 2019 +0200
+++ b/src/hotspot/share/gc/g1/g1RedirtyCardsQueue.hpp	Wed Jul 24 11:49:39 2019 +0200
@@ -36,10 +36,10 @@
 struct G1RedirtyCardsBufferList {
   BufferNode* _head;
   BufferNode* _tail;
-  size_t _count;
+  size_t _entry_count;
 
   G1RedirtyCardsBufferList();
-  G1RedirtyCardsBufferList(BufferNode* head, BufferNode* tail, size_t count);
+  G1RedirtyCardsBufferList(BufferNode* head, BufferNode* tail, size_t entry_count);
 };
 
 // Provide G1RedirtyCardsQueue with a thread-local qset.  It provides an
@@ -100,7 +100,7 @@
   DEFINE_PAD_MINUS_SIZE(1, DEFAULT_CACHE_LINE_SIZE, 0);
   BufferNode::Stack _list;
   DEFINE_PAD_MINUS_SIZE(2, DEFAULT_CACHE_LINE_SIZE, sizeof(size_t));
-  volatile size_t _count;
+  volatile size_t _entry_count;
   DEFINE_PAD_MINUS_SIZE(3, DEFAULT_CACHE_LINE_SIZE, sizeof(BufferNode*));
   BufferNode* _tail;
   DEBUG_ONLY(mutable bool _collecting;)