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.

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

@@ -467,36 +467,59 @@ void G1YoungCollector::set_young_collection_default_active_worker_threads(){
   log_info(gc,task)("Using %u workers of %u for evacuation", active_workers, workers()->max_workers());
 }
 
-void G1YoungCollector::flush_dirty_card_queues() {
+void G1YoungCollector::retire_tlabs() {
+  Ticks start = Ticks::now();
+  _g1h->retire_tlabs();
+  double retire_time = (Ticks::now() - start).seconds() * MILLIUNITS;
+  phase_times()->record_prepare_tlab_time_ms(retire_time);
+}
+
+void G1YoungCollector::concatenate_dirty_card_logs_and_stats() {
   Ticks start = Ticks::now();
   G1DirtyCardQueueSet& qset = G1BarrierSet::dirty_card_queue_set();
   size_t old_cards = qset.num_cards();
-  qset.concatenate_logs();
-  size_t added_cards = qset.num_cards() - old_cards;
-  Tickspan concat_time = Ticks::now() - start;
-  policy()->record_concatenate_dirty_card_logs(concat_time, added_cards);
+  qset.concatenate_logs_and_stats();
+  size_t pending_cards = qset.num_cards();
+  size_t thread_buffer_cards = pending_cards - old_cards;
+  policy()->record_concurrent_refinement_stats(pending_cards, thread_buffer_cards);
+  double concat_time = (Ticks::now() - start).seconds() * MILLIUNITS;
+  phase_times()->record_concatenate_dirty_card_logs_time_ms(concat_time);
+}
+
+#ifdef ASSERT
+void G1YoungCollector::verify_empty_dirty_card_logs() const {
+  struct Verifier : public ThreadClosure {
+    size_t _buffer_size;
+    Verifier() : _buffer_size(G1BarrierSet::dirty_card_queue_set().buffer_size()) {}
+    void do_thread(Thread* t) override {
+      G1DirtyCardQueue& queue = G1ThreadLocalData::dirty_card_queue(t);
+      assert((queue.buffer() == nullptr) || (queue.index() == _buffer_size),
+             "non-empty dirty card queue for thread");
+    }
+  } verifier;
+  Threads::threads_do(&verifier);
 }
+#endif // ASSERT
+
+void G1YoungCollector::pre_evacuate_collection_set(G1EvacInfo* evacuation_info) {
+  // Flush early, so later phases don't need to account for per-thread stuff.
+  // Flushes deferred card marks, so must precede concatenating logs.
+  retire_tlabs();
+
+  // Flush early, so later phases don't need to account for per-thread stuff.
+  concatenate_dirty_card_logs_and_stats();
+
+  calculate_collection_set(evacuation_info, policy()->max_pause_time_ms());
 
-void G1YoungCollector::pre_evacuate_collection_set(G1EvacInfo* evacuation_info, G1ParScanThreadStateSet* per_thread_states) {
   // Please see comment in g1CollectedHeap.hpp and
   // G1CollectedHeap::ref_processing_init() to see how
   // reference processing currently works in G1.
   ref_processor_stw()->start_discovery(false /* always_clear */);
 
-   _evac_failure_regions.pre_collection(_g1h->max_reserved_regions());
+  _evac_failure_regions.pre_collection(_g1h->max_reserved_regions());
 
   _g1h->gc_prologue(false);
 
-  {
-    Ticks start = Ticks::now();
-    _g1h->retire_tlabs();
-    phase_times()->record_prepare_tlab_time_ms((Ticks::now() - start).seconds() * 1000.0);
-  }
-
-  // Flush dirty card queues to qset, so later phases don't need to account
-  // for partially filled per-thread queues and such.
-  flush_dirty_card_queues();
-
   hot_card_cache()->reset_hot_cache_claimed_index();
 
   // Initialize the GC alloc regions.
@@ -519,7 +542,7 @@ void G1YoungCollector::pre_evacuate_collection_set(G1EvacInfo* evacuation_info,
   }
 
   assert(_g1h->verifier()->check_region_attr_table(), "Inconsistency in the region attributes table.");
-  per_thread_states->preserved_marks_set()->assert_empty();
+  verify_empty_dirty_card_logs();
 
 #if COMPILER2_OR_JVMCI
   DerivedPointerTable::clear();
@@ -1013,19 +1036,6 @@ bool G1YoungCollector::evacuation_failed() const {
   return _evac_failure_regions.evacuation_failed();
 }
 
-class G1PreservedMarksSet : public PreservedMarksSet {
-public:
-
-  G1PreservedMarksSet(uint num_workers) : PreservedMarksSet(true /* in_c_heap */) {
-    init(num_workers);
-  }
-
-  virtual ~G1PreservedMarksSet() {
-    assert_empty();
-    reclaim();
-  }
-};
-
 G1YoungCollector::G1YoungCollector(GCCause::Cause gc_cause) :
   _g1h(G1CollectedHeap::heap()),
   _gc_cause(gc_cause),
@@ -1073,20 +1083,14 @@ void G1YoungCollector::collect() {
     // other trivial setup above).
     policy()->record_young_collection_start();
 
-    calculate_collection_set(jtm.evacuation_info(), policy()->max_pause_time_ms());
+    pre_evacuate_collection_set(jtm.evacuation_info());
 
-    G1RedirtyCardsQueueSet rdcqs(G1BarrierSet::dirty_card_queue_set().allocator());
-    G1PreservedMarksSet preserved_marks_set(workers()->active_workers());
     G1ParScanThreadStateSet per_thread_states(_g1h,
-                                              &rdcqs,
-                                              &preserved_marks_set,
                                               workers()->active_workers(),
                                               collection_set()->young_region_length(),
                                               collection_set()->optional_region_length(),
                                               &_evac_failure_regions);
 
-    pre_evacuate_collection_set(jtm.evacuation_info(), &per_thread_states);
-
     bool may_do_optional_evacuation = collection_set()->optional_region_length() != 0;
     // Actually do the work...
     evacuate_initial_collection_set(&per_thread_states, may_do_optional_evacuation);

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

@@ -98,9 +98,11 @@ class G1YoungCollector {
 
   void set_young_collection_default_active_worker_threads();
 
-  void flush_dirty_card_queues();
+  void retire_tlabs();
+  void concatenate_dirty_card_logs_and_stats();
+  void verify_empty_dirty_card_logs() const NOT_DEBUG_RETURN;
 
-  void pre_evacuate_collection_set(G1EvacInfo* evacuation_info, G1ParScanThreadStateSet* pss);
+  void pre_evacuate_collection_set(G1EvacInfo* evacuation_info);
   // Actually do the work of evacuating the parts of the collection set.
   // The has_optional_evacuation_work flag for the initial collection set
   // evacuation indicates whether one or more optional evacuation steps may