8252752: Clear card table for old regions during scan in G1

Reviewed-by: kbarrett, iwalulya, ayang

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

@@ -84,6 +84,7 @@ class G1CardTable : public CardTable {
   }
 
   static CardValue g1_young_card_val() { return g1_young_gen; }
+  static CardValue g1_scanned_card_val() { return g1_card_already_scanned; }
 
   void verify_g1_young_region(MemRegion mr) PRODUCT_RETURN;
   void g1_mark_as_young(const MemRegion& mr);
@@ -103,8 +104,8 @@ class G1CardTable : public CardTable {
   // be inaccurate as it does not perform the dirtying atomically.
   inline size_t mark_region_dirty(size_t start_card_index, size_t num_cards);
 
-  // Mark the given range of cards as Scanned. All of these cards must be Dirty.
-  inline void mark_as_scanned(size_t start_card_index, size_t num_cards);
+  // Change the given range of dirty cards to "which". All of these cards must be Dirty.
+  inline void change_dirty_cards_to(size_t start_card_index, size_t num_cards, CardValue which);
 
   inline uint region_idx_for(CardValue* p);
 

src/hotspot/share/gc/g1/g1CardTable.inline.hpp

@@ -77,14 +77,14 @@ inline size_t G1CardTable::mark_region_dirty(size_t start_card_index, size_t num
   return result;
 }
 
-inline void G1CardTable::mark_as_scanned(size_t start_card_index, size_t num_cards) {
+inline void G1CardTable::change_dirty_cards_to(size_t start_card_index, size_t num_cards, CardValue which) {
   CardValue* start = &_byte_map[start_card_index];
   CardValue* const end = start + num_cards;
   while (start < end) {
     CardValue value = *start;
     assert(value == dirty_card_val(),
            "Must have been dirty %d start " PTR_FORMAT " " PTR_FORMAT, value, p2i(start), p2i(end));
-    *start++ = g1_card_already_scanned;
+    *start++ = which;
   }
 }
 

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

@@ -3045,10 +3045,11 @@ void G1CollectedHeap::do_collection_pause_at_safepoint_helper(double target_paus
                                                   collection_set()->optional_region_length());
         pre_evacuate_collection_set(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);
+        evacuate_initial_collection_set(&per_thread_states, may_do_optional_evacuation);
 
-        if (_collection_set.optional_region_length() != 0) {
+        if (may_do_optional_evacuation) {
           evacuate_optional_collection_set(&per_thread_states);
         }
         post_evacuate_collection_set(evacuation_info, &rdcqs, &per_thread_states);
@@ -3814,10 +3815,11 @@ class G1EvacuateRegionsBaseTask : public AbstractGangTask {
 
 class G1EvacuateRegionsTask : public G1EvacuateRegionsBaseTask {
   G1RootProcessor* _root_processor;
+  bool _has_optional_evacuation_work;
 
   void scan_roots(G1ParScanThreadState* pss, uint worker_id) {
     _root_processor->evacuate_roots(pss, worker_id);
-    _g1h->rem_set()->scan_heap_roots(pss, worker_id, G1GCPhaseTimes::ScanHR, G1GCPhaseTimes::ObjCopy);
+    _g1h->rem_set()->scan_heap_roots(pss, worker_id, G1GCPhaseTimes::ScanHR, G1GCPhaseTimes::ObjCopy, _has_optional_evacuation_work);
     _g1h->rem_set()->scan_collection_set_regions(pss, worker_id, G1GCPhaseTimes::ScanHR, G1GCPhaseTimes::CodeRoots, G1GCPhaseTimes::ObjCopy);
   }
 
@@ -3838,13 +3840,16 @@ class G1EvacuateRegionsTask : public G1EvacuateRegionsBaseTask {
                         G1ParScanThreadStateSet* per_thread_states,
                         G1ScannerTasksQueueSet* task_queues,
                         G1RootProcessor* root_processor,
-                        uint num_workers) :
+                        uint num_workers,
+                        bool has_optional_evacuation_work) :
     G1EvacuateRegionsBaseTask("G1 Evacuate Regions", per_thread_states, task_queues, num_workers),
-    _root_processor(root_processor)
+    _root_processor(root_processor),
+    _has_optional_evacuation_work(has_optional_evacuation_work)
   { }
 };
 
-void G1CollectedHeap::evacuate_initial_collection_set(G1ParScanThreadStateSet* per_thread_states) {
+void G1CollectedHeap::evacuate_initial_collection_set(G1ParScanThreadStateSet* per_thread_states,
+                                                      bool has_optional_evacuation_work) {
   G1GCPhaseTimes* p = phase_times();
 
   {
@@ -3859,7 +3864,12 @@ void G1CollectedHeap::evacuate_initial_collection_set(G1ParScanThreadStateSet* p
   Ticks start_processing = Ticks::now();
   {
     G1RootProcessor root_processor(this, num_workers);
-    G1EvacuateRegionsTask g1_par_task(this, per_thread_states, _task_queues, &root_processor, num_workers);
+    G1EvacuateRegionsTask g1_par_task(this,
+                                      per_thread_states,
+                                      _task_queues,
+                                      &root_processor,
+                                      num_workers,
+                                      has_optional_evacuation_work);
     task_time = run_task_timed(&g1_par_task);
     // Closing the inner scope will execute the destructor for the G1RootProcessor object.
     // To extract its code root fixup time we measure total time of this scope and
@@ -3869,12 +3879,14 @@ void G1CollectedHeap::evacuate_initial_collection_set(G1ParScanThreadStateSet* p
 
   p->record_initial_evac_time(task_time.seconds() * 1000.0);
   p->record_or_add_code_root_fixup_time((total_processing - task_time).seconds() * 1000.0);
+
+  rem_set()->complete_evac_phase(has_optional_evacuation_work);
 }
 
 class G1EvacuateOptionalRegionsTask : public G1EvacuateRegionsBaseTask {
 
   void scan_roots(G1ParScanThreadState* pss, uint worker_id) {
-    _g1h->rem_set()->scan_heap_roots(pss, worker_id, G1GCPhaseTimes::OptScanHR, G1GCPhaseTimes::OptObjCopy);
+    _g1h->rem_set()->scan_heap_roots(pss, worker_id, G1GCPhaseTimes::OptScanHR, G1GCPhaseTimes::OptObjCopy, true /* remember_already_scanned_cards */);
     _g1h->rem_set()->scan_collection_set_regions(pss, worker_id, G1GCPhaseTimes::OptScanHR, G1GCPhaseTimes::OptCodeRoots, G1GCPhaseTimes::OptObjCopy);
   }
 
@@ -3934,6 +3946,8 @@ void G1CollectedHeap::evacuate_optional_collection_set(G1ParScanThreadStateSet*
       evacuate_next_optional_regions(per_thread_states);
       phase_times()->record_or_add_optional_evac_time((Ticks::now() - start).seconds() * 1000.0);
     }
+
+    rem_set()->complete_evac_phase(true /* has_more_than_one_evacuation_phase */);
   }
 
   _collection_set.abandon_optional_collection_set(per_thread_states);

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

@@ -785,7 +785,23 @@ class G1CollectedHeap : public CollectedHeap {
   void calculate_collection_set(G1EvacuationInfo& evacuation_info, double target_pause_time_ms);
 
   // Actually do the work of evacuating the parts of the collection set.
-  void evacuate_initial_collection_set(G1ParScanThreadStateSet* per_thread_states);
+  // The has_optional_evacuation_work flag for the initial collection set
+  // evacuation indicates whether one or more optional evacuation steps may
+  // follow.
+  // If not set, G1 can avoid clearing the card tables of regions that we scan
+  // for roots from the heap: when scanning the card table for dirty cards after
+  // all remembered sets have been dumped onto it, for optional evacuation we
+  // mark these cards as "Scanned" to know that we do not need to re-scan them
+  // in the additional optional evacuation passes. This means that in the "Clear
+  // Card Table" phase we need to clear those marks. However, if there is no
+  // optional evacuation, g1 can immediately clean the dirty cards it encounters
+  // as nobody else will be looking at them again, saving the clear card table
+  // work later.
+  // This case is very common (young only collections and most mixed gcs), so
+  // depending on the ratio between scanned and evacuated regions (which g1 always
+  // needs to clear), this is a big win.
+  void evacuate_initial_collection_set(G1ParScanThreadStateSet* per_thread_states,
+                                       bool has_optional_evacuation_work);
   void evacuate_optional_collection_set(G1ParScanThreadStateSet* per_thread_states);
 private:
   // Evacuate the next set of optional regions.

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

@@ -131,8 +131,17 @@ class G1RemSetScanState : public CHeapObj<mtGC> {
   }
 
 private:
-  // The complete set of regions which card table needs to be cleared at the end of GC because
-  // we scribbled all over them.
+  // The complete set of regions which card table needs to be cleared at the end
+  // of GC because we scribbled over these card tables.
+  //
+  // Regions may be added for two reasons:
+  // - they were part of the collection set: they may contain g1_young_card_val
+  // or regular card marks that we never scan so we must always clear their card
+  // table
+  // - or in case g1 does an optional evacuation pass, g1 marks the cards in there
+  // as g1_scanned_card_val. If G1 only did an initial evacuation pass, the
+  // scanning already cleared these cards. In that case they are not in this set
+  // at the end of the collection.
   G1DirtyRegions* _all_dirty_regions;
   // The set of regions which card table needs to be scanned for new dirty cards
   // in the current evacuation pass.
@@ -321,16 +330,22 @@ class G1RemSetScanState : public CHeapObj<mtGC> {
   }
 
   void prepare_for_merge_heap_roots() {
-    _all_dirty_regions->merge(_next_dirty_regions);
+    assert(_next_dirty_regions->size() == 0, "next dirty regions must be empty");
 
-    _next_dirty_regions->reset();
     for (size_t i = 0; i < _max_reserved_regions; i++) {
       _card_table_scan_state[i] = 0;
     }
 
     ::memset(_region_scan_chunks, false, _num_total_scan_chunks * sizeof(*_region_scan_chunks));
   }
 
+  void complete_evac_phase(bool merge_dirty_regions) {
+    if (merge_dirty_regions) {
+      _all_dirty_regions->merge(_next_dirty_regions);
+    }
+    _next_dirty_regions->reset();
+  }
+
   // Returns whether the given region contains cards we need to scan. The remembered
   // set and other sources may contain cards that
   // - are in uncommitted regions
@@ -374,8 +389,6 @@ class G1RemSetScanState : public CHeapObj<mtGC> {
   }
 
   void cleanup(WorkGang* workers) {
-    _all_dirty_regions->merge(_next_dirty_regions);
-
     clear_card_table(workers);
 
     delete _all_dirty_regions;
@@ -448,7 +461,7 @@ class G1RemSetScanState : public CHeapObj<mtGC> {
 #ifdef ASSERT
     HeapRegion* hr = G1CollectedHeap::heap()->region_at(region);
     assert(hr->in_collection_set(),
-           "Only add young regions to all dirty regions directly but %u is %s",
+           "Only add collection set regions to all dirty regions directly but %u is %s",
            hr->hrm_index(), hr->get_short_type_str());
 #endif
     _all_dirty_regions->add_dirty_region(region);
@@ -641,6 +654,7 @@ class G1ScanHRForRegionClosure : public HeapRegionClosure {
   // The address to which this thread already scanned (walked the heap) up to during
   // card scanning (exclusive).
   HeapWord* _scanned_to;
+  G1CardTable::CardValue _scanned_card_value;
 
   HeapWord* scan_memregion(uint region_idx_for_card, MemRegion mr) {
     HeapRegion* const card_region = _g1h->region_at(region_idx_for_card);
@@ -677,7 +691,7 @@ class G1ScanHRForRegionClosure : public HeapRegionClosure {
   }
 
   ALWAYSINLINE void do_card_block(uint const region_idx, size_t const first_card, size_t const num_cards) {
-    _ct->mark_as_scanned(first_card, num_cards);
+    _ct->change_dirty_cards_to(first_card, num_cards, _scanned_card_value);
     do_claimed_block(region_idx, first_card, num_cards);
     _blocks_scanned++;
   }
@@ -727,7 +741,8 @@ class G1ScanHRForRegionClosure : public HeapRegionClosure {
   G1ScanHRForRegionClosure(G1RemSetScanState* scan_state,
                            G1ParScanThreadState* pss,
                            uint worker_id,
-                           G1GCPhaseTimes::GCParPhases phase) :
+                           G1GCPhaseTimes::GCParPhases phase,
+                           bool remember_already_scanned_cards) :
     _g1h(G1CollectedHeap::heap()),
     _ct(_g1h->card_table()),
     _bot(_g1h->bot()),
@@ -740,7 +755,9 @@ class G1ScanHRForRegionClosure : public HeapRegionClosure {
     _chunks_claimed(0),
     _rem_set_root_scan_time(),
     _rem_set_trim_partially_time(),
-    _scanned_to(NULL) {
+    _scanned_to(NULL),
+    _scanned_card_value(remember_already_scanned_cards ? G1CardTable::g1_scanned_card_val()
+                                                       : G1CardTable::clean_card_val()) {
   }
 
   bool do_heap_region(HeapRegion* r) {
@@ -765,10 +782,11 @@ class G1ScanHRForRegionClosure : public HeapRegionClosure {
 };
 
 void G1RemSet::scan_heap_roots(G1ParScanThreadState* pss,
-                            uint worker_id,
-                            G1GCPhaseTimes::GCParPhases scan_phase,
-                            G1GCPhaseTimes::GCParPhases objcopy_phase) {
-  G1ScanHRForRegionClosure cl(_scan_state, pss, worker_id, scan_phase);
+                               uint worker_id,
+                               G1GCPhaseTimes::GCParPhases scan_phase,
+                               G1GCPhaseTimes::GCParPhases objcopy_phase,
+                               bool remember_already_scanned_cards) {
+  G1ScanHRForRegionClosure cl(_scan_state, pss, worker_id, scan_phase, remember_already_scanned_cards);
   _scan_state->iterate_dirty_regions_from(&cl, worker_id);
 
   G1GCPhaseTimes* p = _g1p->phase_times();
@@ -891,18 +909,11 @@ void G1RemSet::scan_collection_set_regions(G1ParScanThreadState* pss,
 void G1RemSet::prepare_region_for_scan(HeapRegion* region) {
   uint hrm_index = region->hrm_index();
 
-  if (region->in_collection_set()) {
-    // Young regions had their card table marked as young at their allocation;
-    // we need to make sure that these marks are cleared at the end of GC, *but*
-    // they should not be scanned for cards.
-    // So directly add them to the "all_dirty_regions".
-    // Same for regions in the (initial) collection set: they may contain cards from
-    // the log buffers, make sure they are cleaned.
-    _scan_state->add_all_dirty_region(hrm_index);
-  } else if (region->is_old_or_humongous_or_archive()) {
+  if (region->is_old_or_humongous_or_archive()) {
     _scan_state->set_scan_top(hrm_index, region->top());
   } else {
-    assert(region->is_free(), "Should only be free region at this point %s", region->get_type_str());
+    assert(region->in_collection_set() || region->is_free(),
+           "Should only be free or in the collection set at this point %s", region->get_type_str());
   }
 }
 
@@ -984,13 +995,34 @@ class G1MergeHeapRootsTask : public AbstractGangTask {
       }
     }
 
-    virtual bool do_heap_region(HeapRegion* r) {
+    // Helper to put the remembered set cards for these regions onto the card
+    // table.
+    //
+    // Called directly for humongous starts regions because we should not add
+    // humongous eager reclaim candidates to the "all" list of regions to
+    // clear the card table by default as we do not know yet whether this region
+    // will be reclaimed (and reused).
+    // If the humongous region contains dirty cards, g1 will scan them
+    // because dumping the remembered set entries onto the card table will add
+    // the humongous region to the "dirty" region list to scan. Then scanning
+    // either clears the card during scan (if there is only an initial evacuation
+    // pass) or the "dirty" list will be merged with the "all" list later otherwise.
+    // (And there is no problem either way if the region does not contain dirty
+    // cards).
+    void dump_rem_set_for_region(HeapRegion* r) {
       assert(r->in_collection_set() || r->is_starts_humongous(), "must be");
 
       HeapRegionRemSet* rem_set = r->rem_set();
       if (!rem_set->is_empty()) {
         rem_set->iterate_prts(*this);
       }
+    }
+
+    virtual bool do_heap_region(HeapRegion* r) {
+      assert(r->in_collection_set(), "must be");
+
+      _scan_state->add_all_dirty_region(r->hrm_index());
+      dump_rem_set_for_region(r);
 
       return false;
     }
@@ -1022,7 +1054,7 @@ class G1MergeHeapRootsTask : public AbstractGangTask {
       guarantee(r->rem_set()->occupancy_less_or_equal_than(G1RSetSparseRegionEntries),
                 "Found a not-small remembered set here. This is inconsistent with previous assumptions.");
 
-      _cl.do_heap_region(r);
+      _cl.dump_rem_set_for_region(r);
 
       // We should only clear the card based remembered set here as we will not
       // implicitly rebuild anything else during eager reclaim. Note that at the moment
@@ -1239,6 +1271,10 @@ void G1RemSet::merge_heap_roots(bool initial_evacuation) {
   }
 }
 
+void G1RemSet::complete_evac_phase(bool has_more_than_one_evacuation_phase) {
+  _scan_state->complete_evac_phase(has_more_than_one_evacuation_phase);
+}
+
 void G1RemSet::exclude_region_from_scan(uint region_idx) {
   _scan_state->clear_scan_top(region_idx);
 }

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

@@ -84,13 +84,15 @@ class G1RemSet: public CHeapObj<mtGC> {
   void scan_heap_roots(G1ParScanThreadState* pss,
                        uint worker_id,
                        G1GCPhaseTimes::GCParPhases scan_phase,
-                       G1GCPhaseTimes::GCParPhases objcopy_phase);
+                       G1GCPhaseTimes::GCParPhases objcopy_phase,
+                       bool remember_already_scanned_cards);
 
   // Merge cards from various sources (remembered sets, hot card cache, log buffers)
   // and calculate the cards that need to be scanned later (via scan_heap_roots()).
   // If initial_evacuation is set, this is called during the initial evacuation.
   void merge_heap_roots(bool initial_evacuation);
 
+  void complete_evac_phase(bool has_more_than_one_evacuation_phase);
   // Prepare for and cleanup after scanning the heap roots. Must be called
   // once before and after in sequential code.
   void prepare_for_scan_heap_roots();