8296419: [REDO] JDK-8295319: pending_cards_at_gc_start doesn't include cards in thread buffers

Reviewed-by: tschatzl, sjohanss

Author: Kim Barrett

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

@@ -1063,7 +1063,7 @@ void G1CollectedHeap::abort_refinement() {
   }
 
   // Discard all remembered set updates and reset refinement statistics.
-  G1BarrierSet::dirty_card_queue_set().abandon_logs();
+  G1BarrierSet::dirty_card_queue_set().abandon_logs_and_stats();
   assert(G1BarrierSet::dirty_card_queue_set().num_cards() == 0,
          "DCQS should be empty");
   concurrent_refine()->get_and_reset_refinement_stats();

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

@@ -530,16 +530,13 @@ bool G1DirtyCardQueueSet::refine_completed_buffer_concurrently(uint worker_id,
   return true;
 }
 
-void G1DirtyCardQueueSet::abandon_logs() {
+void G1DirtyCardQueueSet::abandon_logs_and_stats() {
   assert_at_safepoint();
-  abandon_completed_buffers();
-  _detached_refinement_stats.reset();
 
   // Disable mutator refinement until concurrent refinement decides otherwise.
   set_mutator_refinement_threshold(SIZE_MAX);
 
-  // Since abandon is done only at safepoints, we can safely manipulate
-  // these queues.
+  // Iterate over all the threads, resetting per-thread queues and stats.
   struct AbandonThreadLogClosure : public ThreadClosure {
     G1DirtyCardQueueSet& _qset;
     AbandonThreadLogClosure(G1DirtyCardQueueSet& qset) : _qset(qset) {}
@@ -550,9 +547,16 @@ void G1DirtyCardQueueSet::abandon_logs() {
     }
   } closure(*this);
   Threads::threads_do(&closure);
+
+  enqueue_all_paused_buffers();
+  abandon_completed_buffers();
+
+  // Reset stats from detached threads.
+  MutexLocker ml(G1DetachedRefinementStats_lock, Mutex::_no_safepoint_check_flag);
+  _detached_refinement_stats.reset();
 }
 
-void G1DirtyCardQueueSet::concatenate_logs() {
+void G1DirtyCardQueueSet::concatenate_logs_and_stats() {
   assert_at_safepoint();
 
   // Disable mutator refinement until concurrent refinement decides otherwise.
@@ -562,47 +566,39 @@ void G1DirtyCardQueueSet::concatenate_logs() {
   // the global list of logs.
   struct ConcatenateThreadLogClosure : public ThreadClosure {
     G1DirtyCardQueueSet& _qset;
-    ConcatenateThreadLogClosure(G1DirtyCardQueueSet& qset) : _qset(qset) {}
+    G1ConcurrentRefineStats _total_stats;
+
+    ConcatenateThreadLogClosure(G1DirtyCardQueueSet& qset) :
+      _qset{qset}, _total_stats{} {}
+
     virtual void do_thread(Thread* t) {
       G1DirtyCardQueue& queue = G1ThreadLocalData::dirty_card_queue(t);
+      // Flush the buffer if non-empty.  Flush before accumulating and
+      // resetting stats, since flushing may modify the stats.
       if ((queue.buffer() != nullptr) &&
           (queue.index() != _qset.buffer_size())) {
         _qset.flush_queue(queue);
       }
+      G1ConcurrentRefineStats& qstats = *queue.refinement_stats();
+      _total_stats += qstats;
+      qstats.reset();
     }
   } closure(*this);
   Threads::threads_do(&closure);
+  _concatenated_refinement_stats = closure._total_stats;
 
   enqueue_all_paused_buffers();
   verify_num_cards();
-}
-
-G1ConcurrentRefineStats G1DirtyCardQueueSet::get_and_reset_refinement_stats() {
-  assert_at_safepoint();
-
-  // Since we're at a safepoint, there aren't any races with recording of
-  // detached refinement stats.  In particular, there's no risk of double
-  // counting a thread that detaches after we've examined it but before
-  // we've processed the detached stats.
-
-  // Collect and reset stats for attached threads.
-  struct CollectStats : public ThreadClosure {
-    G1ConcurrentRefineStats _total_stats;
-    virtual void do_thread(Thread* t) {
-      G1DirtyCardQueue& dcq = G1ThreadLocalData::dirty_card_queue(t);
-      G1ConcurrentRefineStats& stats = *dcq.refinement_stats();
-      _total_stats += stats;
-      stats.reset();
-    }
-  } closure;
-  Threads::threads_do(&closure);
 
   // Collect and reset stats from detached threads.
   MutexLocker ml(G1DetachedRefinementStats_lock, Mutex::_no_safepoint_check_flag);
-  closure._total_stats += _detached_refinement_stats;
+  _concatenated_refinement_stats += _detached_refinement_stats;
   _detached_refinement_stats.reset();
+}
 
-  return closure._total_stats;
+G1ConcurrentRefineStats G1DirtyCardQueueSet::concatenated_refinement_stats() const {
+  assert_at_safepoint();
+  return _concatenated_refinement_stats;
 }
 
 void G1DirtyCardQueueSet::record_detached_refinement_stats(G1ConcurrentRefineStats* stats) {

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

@@ -95,10 +95,10 @@ class G1DirtyCardQueueSet: public PtrQueueSet {
   //
   // The paused buffers are conceptually an extension of the completed buffers
   // queue, and operations which need to deal with all of the queued buffers
-  // (such as concatenate_logs) also need to deal with any paused buffers.  In
-  // general, if a safepoint performs a GC then the paused buffers will be
-  // processed as part of it, and there won't be any paused buffers after a
-  // GC safepoint.
+  // (such as concatenating or abandoning logs) also need to deal with any
+  // paused buffers.  In general, if a safepoint performs a GC then the paused
+  // buffers will be processed as part of it, and there won't be any paused
+  // buffers after a GC safepoint.
   class PausedBuffers {
     class PausedList : public CHeapObj<mtGC> {
       BufferNode* volatile _head;
@@ -175,6 +175,7 @@ class G1DirtyCardQueueSet: public PtrQueueSet {
 
   G1FreeIdSet _free_ids;
 
+  G1ConcurrentRefineStats _concatenated_refinement_stats;
   G1ConcurrentRefineStats _detached_refinement_stats;
 
   // Verify _num_cards == sum of cards in the completed queue.
@@ -267,17 +268,21 @@ class G1DirtyCardQueueSet: public PtrQueueSet {
                                             size_t stop_at,
                                             G1ConcurrentRefineStats* stats);
 
-  // If a full collection is happening, reset partial logs, and release
-  // completed ones: the full collection will make them all irrelevant.
-  void abandon_logs();
+  // If a full collection is happening, reset per-thread refinement stats and
+  // partial logs, and release completed logs. The full collection will make
+  // them all irrelevant.
+  // precondition: at safepoint.
+  void abandon_logs_and_stats();
 
-  // If any threads have partial logs, add them to the global list of logs.
-  void concatenate_logs();
+  // Collect and reset all the per-thread refinement stats.  If any threads
+  // have partial logs then add them to the global list.
+  // precondition: at safepoint.
+  void concatenate_logs_and_stats();
 
   // Return the total of mutator refinement stats for all threads.
-  // Also resets the stats for the threads.
   // precondition: at safepoint.
-  G1ConcurrentRefineStats get_and_reset_refinement_stats();
+  // precondition: only call after concatenate_logs_and_stats.
+  G1ConcurrentRefineStats concatenated_refinement_stats() const;
 
   // Accumulate refinement stats from threads that are detaching.
   void record_detached_refinement_stats(G1ConcurrentRefineStats* stats);

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

@@ -566,7 +566,7 @@ G1ParScanThreadState* G1ParScanThreadStateSet::state_for_worker(uint worker_id)
   if (_states[worker_id] == NULL) {
     _states[worker_id] =
       new G1ParScanThreadState(_g1h, rdcqs(),
-                               _preserved_marks_set->get(worker_id),
+                               _preserved_marks_set.get(worker_id),
                                worker_id, _n_workers,
                                _young_cset_length, _optional_cset_length,
                                _evac_failure_regions);
@@ -687,22 +687,21 @@ void G1ParScanThreadState::update_numa_stats(uint node_index) {
 }
 
 G1ParScanThreadStateSet::G1ParScanThreadStateSet(G1CollectedHeap* g1h,
-                                                 G1RedirtyCardsQueueSet* rdcqs,
-                                                 PreservedMarksSet* preserved_marks_set,
                                                  uint n_workers,
                                                  size_t young_cset_length,
                                                  size_t optional_cset_length,
                                                  G1EvacFailureRegions* evac_failure_regions) :
     _g1h(g1h),
-    _rdcqs(rdcqs),
-    _preserved_marks_set(preserved_marks_set),
+    _rdcqs(G1BarrierSet::dirty_card_queue_set().allocator()),
+    _preserved_marks_set(true /* in_c_heap */),
     _states(NEW_C_HEAP_ARRAY(G1ParScanThreadState*, n_workers, mtGC)),
     _surviving_young_words_total(NEW_C_HEAP_ARRAY(size_t, young_cset_length + 1, mtGC)),
     _young_cset_length(young_cset_length),
     _optional_cset_length(optional_cset_length),
     _n_workers(n_workers),
     _flushed(false),
     _evac_failure_regions(evac_failure_regions) {
+  _preserved_marks_set.init(n_workers);
   for (uint i = 0; i < n_workers; ++i) {
     _states[i] = NULL;
   }
@@ -713,4 +712,6 @@ G1ParScanThreadStateSet::~G1ParScanThreadStateSet() {
   assert(_flushed, "thread local state from the per thread states should have been flushed");
   FREE_C_HEAP_ARRAY(G1ParScanThreadState*, _states);
   FREE_C_HEAP_ARRAY(size_t, _surviving_young_words_total);
+  _preserved_marks_set.assert_empty();
+  _preserved_marks_set.reclaim();
 }

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

@@ -33,6 +33,7 @@
 #include "gc/shared/ageTable.hpp"
 #include "gc/shared/copyFailedInfo.hpp"
 #include "gc/shared/partialArrayTaskStepper.hpp"
+#include "gc/shared/preservedMarks.hpp"
 #include "gc/shared/stringdedup/stringDedup.hpp"
 #include "gc/shared/taskqueue.hpp"
 #include "memory/allocation.hpp"
@@ -229,8 +230,8 @@ class G1ParScanThreadState : public CHeapObj<mtGC> {
 
 class G1ParScanThreadStateSet : public StackObj {
   G1CollectedHeap* _g1h;
-  G1RedirtyCardsQueueSet* _rdcqs;
-  PreservedMarksSet* _preserved_marks_set;
+  G1RedirtyCardsQueueSet _rdcqs;
+  PreservedMarksSet _preserved_marks_set;
   G1ParScanThreadState** _states;
   size_t* _surviving_young_words_total;
   size_t _young_cset_length;
@@ -241,16 +242,14 @@ class G1ParScanThreadStateSet : public StackObj {
 
  public:
   G1ParScanThreadStateSet(G1CollectedHeap* g1h,
-                          G1RedirtyCardsQueueSet* rdcqs,
-                          PreservedMarksSet* preserved_marks_set,
                           uint n_workers,
                           size_t young_cset_length,
                           size_t optional_cset_length,
                           G1EvacFailureRegions* evac_failure_regions);
   ~G1ParScanThreadStateSet();
 
-  G1RedirtyCardsQueueSet* rdcqs() { return _rdcqs; }
-  PreservedMarksSet* preserved_marks_set() { return _preserved_marks_set; }
+  G1RedirtyCardsQueueSet* rdcqs() { return &_rdcqs; }
+  PreservedMarksSet* preserved_marks_set() { return &_preserved_marks_set; }
 
   void flush_stats();
   void record_unused_optional_region(HeapRegion* hr);

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

@@ -586,12 +586,14 @@ static void log_refinement_stats(const char* kind, const G1ConcurrentRefineStats
             stats.dirtied_cards());
 }
 
-void G1Policy::record_concurrent_refinement_stats() {
-  G1DirtyCardQueueSet& dcqs = G1BarrierSet::dirty_card_queue_set();
-  _pending_cards_at_gc_start = dcqs.num_cards();
+void G1Policy::record_concurrent_refinement_stats(size_t pending_cards,
+                                                  size_t thread_buffer_cards) {
+  _pending_cards_at_gc_start = pending_cards;
+  _analytics->report_dirtied_cards_in_thread_buffers(thread_buffer_cards);
 
   // Collect per-thread stats, mostly from mutator activity.
-  G1ConcurrentRefineStats mut_stats = dcqs.get_and_reset_refinement_stats();
+  G1DirtyCardQueueSet& dcqs = G1BarrierSet::dirty_card_queue_set();
+  G1ConcurrentRefineStats mut_stats = dcqs.concatenated_refinement_stats();
 
   // Collect specialized concurrent refinement thread stats.
   G1ConcurrentRefine* cr = _g1h->concurrent_refine();
@@ -628,11 +630,6 @@ void G1Policy::record_concurrent_refinement_stats() {
   }
 }
 
-void G1Policy::record_concatenate_dirty_card_logs(Tickspan concat_time, size_t num_cards) {
-  _analytics->report_dirtied_cards_in_thread_buffers(num_cards);
-  phase_times()->record_concatenate_dirty_card_logs_time_ms(concat_time.seconds() * MILLIUNITS);
-}
-
 void G1Policy::record_young_collection_start() {
   Ticks now = Ticks::now();
   // We only need to do this here as the policy will only be applied
@@ -647,8 +644,6 @@ void G1Policy::record_young_collection_start() {
 
   phase_times()->record_cur_collection_start_sec(now.seconds());
 
-  record_concurrent_refinement_stats();
-
   _collection_set->reset_bytes_used_before();
 
   // do that for any other surv rate groups

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

@@ -291,9 +291,6 @@ class G1Policy: public CHeapObj<mtGC> {
   // Indicate that we aborted marking before doing any mixed GCs.
   void abort_time_to_mixed_tracking();
 
-  // Record and log stats before not-full collection.
-  void record_concurrent_refinement_stats();
-
 public:
 
   G1Policy(STWGCTimer* gc_timer);
@@ -312,8 +309,6 @@ class G1Policy: public CHeapObj<mtGC> {
   // This should be called after the heap is resized.
   void record_new_heap_size(uint new_number_of_regions);
 
-  void record_concatenate_dirty_card_logs(Tickspan concat_time, size_t num_cards);
-
   void init(G1CollectedHeap* g1h, G1CollectionSet* collection_set);
 
   // Record the start and end of the young gc pause.
@@ -411,6 +406,12 @@ class G1Policy: public CHeapObj<mtGC> {
 
   void transfer_survivors_to_cset(const G1SurvivorRegions* survivors);
 
+  // Record and log stats and pending cards before not-full collection.
+  // thread_buffer_cards is the number of cards that were in per-thread
+  // buffers.  pending_cards includes thread_buffer_cards.
+  void record_concurrent_refinement_stats(size_t pending_cards,
+                                          size_t thread_buffer_cards);
+
 private:
   //
   // Survivor regions policy.