# HG changeset patch
# User tschatzl
# Date 1562570680 -7200
# Node ID 5919b273def6fbfcd06548f6d3900b486b990563
# Parent  78a2b1bb15cf3d6a431f45c073794d1c93f254d5
8224741: Optimize the scan area during the Scan Heap Roots phase
Summary: When scanning card blocks, remember the last address already scanned. Continue scanning from this address.
Reviewed-by: kbarrett, lkorinth

diff -r 78a2b1bb15cf -r 5919b273def6 src/hotspot/share/gc/g1/g1RemSet.cpp
--- a/src/hotspot/share/gc/g1/g1RemSet.cpp	Fri Jul 05 23:31:32 2019 +0200
+++ b/src/hotspot/share/gc/g1/g1RemSet.cpp	Mon Jul 08 09:24:40 2019 +0200
@@ -912,11 +912,20 @@
   Tickspan _rem_set_root_scan_time;
   Tickspan _rem_set_trim_partially_time;
 
-  void scan_memregion(uint region_idx_for_card, MemRegion mr) {
+  // The address to which this thread already scanned (walked the heap) up to during
+  // card scanning (exclusive).
+  HeapWord* _scanned_to;
+
+  HeapWord* scan_memregion(uint region_idx_for_card, MemRegion mr) {
     HeapRegion* const card_region = _g1h->region_at(region_idx_for_card);
     G1ScanCardClosure card_cl(_g1h, _pss);
-    card_region->oops_on_card_seq_iterate_careful<true>(mr, &card_cl);
+
+    HeapWord* const scanned_to = card_region->oops_on_memregion_seq_iterate_careful<true>(mr, &card_cl);
+    assert(scanned_to != NULL, "Should be able to scan range");
+    assert(scanned_to >= mr.end(), "Scanned to " PTR_FORMAT " less than range " PTR_FORMAT, p2i(scanned_to), p2i(mr.end()));
+
     _pss->trim_queue_partially();
+    return scanned_to;
   }
 
   void do_claimed_block(uint const region_idx_for_card, size_t const first_card, size_t const num_cards) {
@@ -931,8 +940,12 @@
       return;
     }
 
-    MemRegion mr(card_start, MIN2(card_start + ((size_t)num_cards << BOTConstants::LogN_words), top));
-    scan_memregion(region_idx_for_card, mr);
+    HeapWord* scan_end = MIN2(card_start + (num_cards << BOTConstants::LogN_words), top);
+    if (_scanned_to >= scan_end) {
+      return;
+    }
+    MemRegion mr(MAX2(card_start, _scanned_to), scan_end);
+    _scanned_to = scan_memregion(region_idx_for_card, mr);
 
     _cards_scanned += num_cards;
   }
@@ -951,6 +964,12 @@
 
     G1CardTableChunkClaimer claim(_scan_state, region_idx);
 
+    // Set the current scan "finger" to NULL for every heap region to scan. Since
+    // the claim value is monotonically increasing, the check to not scan below this
+    // will filter out objects spanning chunks within the region too then, as opposed
+    // to resetting this value for every claim.
+    _scanned_to = NULL;
+
     while (claim.has_next()) {
       size_t const region_card_base_idx = ((size_t)region_idx << HeapRegion::LogCardsPerRegion) + claim.value();
       CardTable::CardValue* const base_addr = _ct->byte_for_index(region_card_base_idx);
@@ -994,7 +1013,8 @@
     _blocks_scanned(0),
     _chunks_claimed(0),
     _rem_set_root_scan_time(),
-    _rem_set_trim_partially_time() {
+    _rem_set_trim_partially_time(),
+    _scanned_to(NULL) {
   }
 
   bool do_heap_region(HeapRegion* r) {
@@ -1292,7 +1312,7 @@
   assert(!dirty_region.is_empty(), "sanity");
 
   G1ConcurrentRefineOopClosure conc_refine_cl(_g1h, worker_i);
-  if (r->oops_on_card_seq_iterate_careful<false>(dirty_region, &conc_refine_cl)) {
+  if (r->oops_on_memregion_seq_iterate_careful<false>(dirty_region, &conc_refine_cl) != NULL) {
     _num_conc_refined_cards++; // Unsynchronized update, only used for logging.
     return;
   }
diff -r 78a2b1bb15cf -r 5919b273def6 src/hotspot/share/gc/g1/heapRegion.hpp
--- a/src/hotspot/share/gc/g1/heapRegion.hpp	Fri Jul 05 23:31:32 2019 +0200
+++ b/src/hotspot/share/gc/g1/heapRegion.hpp	Mon Jul 08 09:24:40 2019 +0200
@@ -289,14 +289,17 @@
   // for the collection set.
   double _predicted_elapsed_time_ms;
 
-  // Iterate over the references in a humongous objects and apply the given closure
-  // to them.
+  // Iterate over the references covered by the given MemRegion in a humongous
+  // object and apply the given closure to them.
   // Humongous objects are allocated directly in the old-gen. So we need special
   // handling for concurrent processing encountering an in-progress allocation.
+  // Returns the address after the last actually scanned or NULL if the area could
+  // not be scanned (That should only happen when invoked concurrently with the
+  // mutator).
   template <class Closure, bool is_gc_active>
-  inline bool do_oops_on_card_in_humongous(MemRegion mr,
-                                           Closure* cl,
-                                           G1CollectedHeap* g1h);
+  inline HeapWord* do_oops_on_memregion_in_humongous(MemRegion mr,
+                                                     Closure* cl,
+                                                     G1CollectedHeap* g1h);
 
   // Returns the block size of the given (dead, potentially having its class unloaded) object
   // starting at p extending to at most the prev TAMS using the given mark bitmap.
@@ -645,18 +648,16 @@
     }
   }
 
-  // Iterate over the objects overlapping part of a card, applying cl
+  // Iterate over the objects overlapping the given memory region, applying cl
   // to all references in the region.  This is a helper for
   // G1RemSet::refine_card*, and is tightly coupled with them.
-  // mr is the memory region covered by the card, trimmed to the
-  // allocated space for this region.  Must not be empty.
+  // mr must not be empty. Must be trimmed to the allocated/parseable space in this region.
   // This region must be old or humongous.
-  // Returns true if the designated objects were successfully
-  // processed, false if an unparsable part of the heap was
-  // encountered; that only happens when invoked concurrently with the
-  // mutator.
+  // Returns the next unscanned address if the designated objects were successfully
+  // processed, NULL if an unparseable part of the heap was encountered (That should
+  // only happen when invoked concurrently with the mutator).
   template <bool is_gc_active, class Closure>
-  inline bool oops_on_card_seq_iterate_careful(MemRegion mr, Closure* cl);
+  inline HeapWord* oops_on_memregion_seq_iterate_careful(MemRegion mr, Closure* cl);
 
   size_t recorded_rs_length() const        { return _recorded_rs_length; }
   double predicted_elapsed_time_ms() const { return _predicted_elapsed_time_ms; }
diff -r 78a2b1bb15cf -r 5919b273def6 src/hotspot/share/gc/g1/heapRegion.inline.hpp
--- a/src/hotspot/share/gc/g1/heapRegion.inline.hpp	Fri Jul 05 23:31:32 2019 +0200
+++ b/src/hotspot/share/gc/g1/heapRegion.inline.hpp	Mon Jul 08 09:24:40 2019 +0200
@@ -257,9 +257,9 @@
 }
 
 template <class Closure, bool is_gc_active>
-bool HeapRegion::do_oops_on_card_in_humongous(MemRegion mr,
-                                              Closure* cl,
-                                              G1CollectedHeap* g1h) {
+HeapWord* HeapRegion::do_oops_on_memregion_in_humongous(MemRegion mr,
+                                                        Closure* cl,
+                                                        G1CollectedHeap* g1h) {
   assert(is_humongous(), "precondition");
   HeapRegion* sr = humongous_start_region();
   oop obj = oop(sr->bottom());
@@ -271,41 +271,48 @@
   // since the allocating thread could have performed a write to the
   // card that might be missed otherwise.
   if (!is_gc_active && (obj->klass_or_null_acquire() == NULL)) {
-    return false;
+    return NULL;
   }
 
   // We have a well-formed humongous object at the start of sr.
   // Only filler objects follow a humongous object in the containing
   // regions, and we can ignore those.  So only process the one
   // humongous object.
-  if (!g1h->is_obj_dead(obj, sr)) {
-    if (obj->is_objArray() || (sr->bottom() < mr.start())) {
-      // objArrays are always marked precisely, so limit processing
-      // with mr.  Non-objArrays might be precisely marked, and since
-      // it's humongous it's worthwhile avoiding full processing.
-      // However, the card could be stale and only cover filler
-      // objects.  That should be rare, so not worth checking for;
-      // instead let it fall out from the bounded iteration.
-      obj->oop_iterate(cl, mr);
-    } else {
-      // If obj is not an objArray and mr contains the start of the
-      // obj, then this could be an imprecise mark, and we need to
-      // process the entire object.
-      obj->oop_iterate(cl);
-    }
+  if (g1h->is_obj_dead(obj, sr)) {
+    // The object is dead. There can be no other object in this region, so return
+    // the end of that region.
+    return end();
   }
-  return true;
+  if (obj->is_objArray() || (sr->bottom() < mr.start())) {
+    // objArrays are always marked precisely, so limit processing
+    // with mr.  Non-objArrays might be precisely marked, and since
+    // it's humongous it's worthwhile avoiding full processing.
+    // However, the card could be stale and only cover filler
+    // objects.  That should be rare, so not worth checking for;
+    // instead let it fall out from the bounded iteration.
+    obj->oop_iterate(cl, mr);
+    return mr.end();
+  } else {
+    // If obj is not an objArray and mr contains the start of the
+    // obj, then this could be an imprecise mark, and we need to
+    // process the entire object.
+    int size = obj->oop_iterate_size(cl);
+    // We have scanned to the end of the object, but since there can be no objects
+    // after this humongous object in the region, we can return the end of the
+    // region if it is greater.
+    return MAX2((HeapWord*)obj + size, mr.end());
+  }
 }
 
 template <bool is_gc_active, class Closure>
-bool HeapRegion::oops_on_card_seq_iterate_careful(MemRegion mr,
-                                                  Closure* cl) {
+HeapWord* HeapRegion::oops_on_memregion_seq_iterate_careful(MemRegion mr,
+                                                       Closure* cl) {
   assert(MemRegion(bottom(), end()).contains(mr), "Card region not in heap region");
   G1CollectedHeap* g1h = G1CollectedHeap::heap();
 
   // Special handling for humongous regions.
   if (is_humongous()) {
-    return do_oops_on_card_in_humongous<Closure, is_gc_active>(mr, cl, g1h);
+    return do_oops_on_memregion_in_humongous<Closure, is_gc_active>(mr, cl, g1h);
   }
   assert(is_old() || is_archive(), "Wrongly trying to iterate over region %u type %s", _hrm_index, get_type_str());
 
@@ -334,7 +341,7 @@
 #endif
 
   const G1CMBitMap* const bitmap = g1h->concurrent_mark()->prev_mark_bitmap();
-  do {
+  while (true) {
     oop obj = oop(cur);
     assert(oopDesc::is_oop(obj, true), "Not an oop at " PTR_FORMAT, p2i(cur));
     assert(obj->klass_or_null() != NULL,
@@ -342,6 +349,7 @@
 
     size_t size;
     bool is_dead = is_obj_dead_with_size(obj, bitmap, &size);
+    bool is_precise = false;
 
     cur += size;
     if (!is_dead) {
@@ -355,11 +363,13 @@
         obj->oop_iterate(cl);
       } else {
         obj->oop_iterate(cl, mr);
+        is_precise = true;
       }
     }
-  } while (cur < end);
-
-  return true;
+    if (cur >= end) {
+      return is_precise ? end : cur;
+    }
+  }
 }
 
 #endif // SHARE_GC_G1_HEAPREGION_INLINE_HPP