Comments by ayang, kbarrett, iwalulya

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

@@ -104,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 to "which". All of these cards must be Dirty.
-  inline void mark_dirty_as(size_t start_card_index, size_t num_cards, CardValue which);
+  // 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,7 +77,7 @@ inline size_t G1CardTable::mark_region_dirty(size_t start_card_index, size_t num
   return result;
 }
 
-inline void G1CardTable::mark_dirty_as(size_t start_card_index, size_t num_cards, CardValue which) {
+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) {

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

@@ -3821,11 +3821,11 @@ class G1EvacuateRegionsBaseTask : public AbstractGangTask {
 
 class G1EvacuateRegionsTask : public G1EvacuateRegionsBaseTask {
   G1RootProcessor* _root_processor;
-  bool _may_do_optional_evacuation;
+  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, _may_do_optional_evacuation);
+    _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);
   }
 
@@ -3847,15 +3847,15 @@ class G1EvacuateRegionsTask : public G1EvacuateRegionsBaseTask {
                         G1ScannerTasksQueueSet* task_queues,
                         G1RootProcessor* root_processor,
                         uint num_workers,
-                        bool may_do_optional_evacuation) :
+                        bool has_optional_evacuation_work) :
     G1EvacuateRegionsBaseTask("G1 Evacuate Regions", per_thread_states, task_queues, num_workers),
     _root_processor(root_processor),
-    _may_do_optional_evacuation(may_do_optional_evacuation)
+    _has_optional_evacuation_work(has_optional_evacuation_work)
   { }
 };
 
 void G1CollectedHeap::evacuate_initial_collection_set(G1ParScanThreadStateSet* per_thread_states,
-                                                      bool may_do_optional_evacuation) {
+                                                      bool has_optional_evacuation_work) {
   G1GCPhaseTimes* p = phase_times();
 
   {
@@ -3875,7 +3875,7 @@ void G1CollectedHeap::evacuate_initial_collection_set(G1ParScanThreadStateSet* p
                                       _task_queues,
                                       &root_processor,
                                       num_workers,
-                                      may_do_optional_evacuation);
+                                      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
@@ -3886,7 +3886,7 @@ 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(may_do_optional_evacuation);
+  rem_set()->complete_evac_phase(has_optional_evacuation_work);
 }
 
 class G1EvacuateOptionalRegionsTask : public G1EvacuateRegionsBaseTask {

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

@@ -782,10 +782,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.
-  // The may_do_optional_evacuation flag indicates whether some or more optional evacuation
-  // steps will follow.
+  // The has_optional_evacuation_work flag for the initial collection set
+  // evacuation indicates whether some 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 may_do_optional_evacuation);
+                                       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

@@ -691,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_dirty_as(first_card, num_cards, _scanned_card_value);
+    _ct->change_dirty_cards_to(first_card, num_cards, _scanned_card_value);
     do_claimed_block(region_idx, first_card, num_cards);
     _blocks_scanned++;
   }