Fixes

Author: Thomas Schatzl

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

@@ -190,9 +190,6 @@ HeapRegion* G1CollectedHeap::new_region(size_t word_size,
       res = _hrm.allocate_free_region(type, node_index);
     }
   }
-  if (res != nullptr) {
-    concurrent_mark()->clear_statistics(res);
-  }
   return res;
 }
 
@@ -1177,7 +1174,7 @@ G1CollectedHeap::G1CollectedHeap() :
   _is_alive_closure_stw(this),
   _is_subject_to_discovery_stw(this),
   _ref_processor_cm(nullptr),
-  _is_alive_closure_cm(this),
+  _is_alive_closure_cm(nullptr),
   _is_subject_to_discovery_cm(this),
   _region_attr() {
 
@@ -1509,15 +1506,15 @@ void G1CollectedHeap::ref_processing_init() {
   //     * Reference discovery will not need a barrier.
 
   // Concurrent Mark ref processor
-  _is_alive_closure_cm.set_concurrent_mark(concurrent_mark());
+  _is_alive_closure_cm = new G1CMIsAliveClosure(concurrent_mark());
   _ref_processor_cm =
     new ReferenceProcessor(&_is_subject_to_discovery_cm,
                            ParallelGCThreads,                              // degree of mt processing
                            // We discover with the gc worker threads during Remark, so both
                            // thread counts must be considered for discovery.
                            MAX2(ParallelGCThreads, ConcGCThreads),         // degree of mt discovery
                            true,                                           // Reference discovery is concurrent
-                           &_is_alive_closure_cm);                         // is alive closure
+                           _is_alive_closure_cm);                          // is alive closure
 
   // STW ref processor
   _ref_processor_stw =
@@ -2638,8 +2635,6 @@ void G1CollectedHeap::free_region(HeapRegion* hr, FreeRegionList* free_list) {
   if (free_list != nullptr) {
     free_list->add_ordered(hr);
   }
-
-  concurrent_mark()->clear_statistics(hr);
 }
 
 void G1CollectedHeap::retain_region(HeapRegion* hr) {

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

@@ -864,7 +864,7 @@ class G1CollectedHeap : public CollectedHeap {
   // _is_alive_non_header field is optional but doing so prevents
   // unnecessary additions to the discovered lists during reference
   // discovery.
-  G1CMIsAliveClosure _is_alive_closure_cm;
+  G1CMIsAliveClosure* _is_alive_closure_cm;
 
   G1CMSubjectToDiscoveryClosure _is_subject_to_discovery_cm;
 public:

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

@@ -253,9 +253,10 @@ class G1PrintCollectionSetDetailClosure : public HeapRegionClosure {
 
   virtual bool do_heap_region(HeapRegion* r) {
     assert(r->in_collection_set(), "Region %u should be in collection set", r->hrm_index());
+    G1ConcurrentMark* cm = G1CollectedHeap::heap()->concurrent_mark();
     _st->print_cr("  " HR_FORMAT ", TAMS: " PTR_FORMAT " PB: " PTR_FORMAT ", age: %4d",
                   HR_FORMAT_PARAMS(r),
-                  p2i(G1CollectedHeap::heap()->concurrent_mark()->top_at_mark_start(r)),
+                  p2i(cm->top_at_mark_start(r)),
                   p2i(r->parsable_bottom()),
                   r->has_surv_rate_group() ? checked_cast<int>(r->age_in_surv_rate_group()) : -1);
     return false;

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

@@ -77,12 +77,8 @@
 #include "utilities/growableArray.hpp"
 #include "utilities/powerOfTwo.hpp"
 
-G1CMIsAliveClosure::G1CMIsAliveClosure(G1CollectedHeap* g1h) :
-  _g1h(g1h), _cm(g1h->concurrent_mark()) { }
-
-void G1CMIsAliveClosure::set_concurrent_mark(G1ConcurrentMark* cm) {
-  assert(_cm == nullptr, "already set");
-  _cm = cm;
+G1CMIsAliveClosure::G1CMIsAliveClosure(G1ConcurrentMark* cm) : _cm(cm) {
+  assert(cm != nullptr, "must be");
 }
 
 bool G1CMBitMapClosure::do_addr(HeapWord* const addr) {
@@ -857,8 +853,6 @@ class NoteStartOfMarkHRClosure : public HeapRegionClosure {
   bool do_heap_region(HeapRegion* r) override {
     if (r->is_old_or_humongous() && !r->is_collection_set_candidate()) {
       _cm->update_top_at_mark_start(r);
-    } else {
-      _cm->reset_top_at_mark_start(r);
     }
     return false;
   }
@@ -877,18 +871,8 @@ G1PreConcurrentStartTask::G1PreConcurrentStartTask(GCCause::Cause cause, G1Concu
   G1BatchedTask("Pre Concurrent Start", G1CollectedHeap::heap()->phase_times()) {
   add_serial_task(new ResetMarkingStateTask(cm));
   add_parallel_task(new NoteStartOfMarkTask());
-
-  //cm->invalidate_top_at_mark_starts();
 };
 
-#ifdef ASSERT
-void G1ConcurrentMark::invalidate_top_at_mark_starts() {
-  for (uint i = 0; i < G1CollectedHeap::heap()->max_reserved_regions(); i++) {
-    _top_at_mark_starts[i] = nullptr;
-  }
-}
-#endif
-
 void G1ConcurrentMark::pre_concurrent_start(GCCause::Cause cause) {
   assert_at_safepoint_on_vm_thread();
 
@@ -1357,7 +1341,7 @@ void G1ConcurrentMark::remark() {
 
     // Unload Klasses, String, Code Cache, etc.
     if (ClassUnloadingWithConcurrentMark) {
-      G1CMIsAliveClosure is_alive(_g1h);
+      G1CMIsAliveClosure is_alive(this);
       _g1h->unload_classes_and_code("Class Unloading", &is_alive, _gc_timer_cm);
     }
 
@@ -1704,7 +1688,7 @@ class G1CMRefProcProxyTask : public RefProcProxyTask {
 
   void work(uint worker_id) override {
     assert(worker_id < _max_workers, "sanity");
-    G1CMIsAliveClosure is_alive(&_g1h);
+    G1CMIsAliveClosure is_alive(&_cm);
     uint index = (_tm == RefProcThreadModel::Single) ? 0 : worker_id;
     G1CMKeepAliveAndDrainClosure keep_alive(&_cm, _cm.task(index), _tm == RefProcThreadModel::Single);
     BarrierEnqueueDiscoveredFieldClosure enqueue;
@@ -1783,7 +1767,7 @@ void G1ConcurrentMark::weak_refs_work() {
 
   {
     GCTraceTime(Debug, gc, phases) debug("Weak Processing", _gc_timer_cm);
-    G1CMIsAliveClosure is_alive(_g1h);
+    G1CMIsAliveClosure is_alive(this);
     WeakProcessor::weak_oops_do(_g1h->workers(), &is_alive, &do_nothing_cl, 1);
   }
 }

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

@@ -41,11 +41,11 @@
 #include "utilities/numberSeq.hpp"
 
 class ConcurrentGCTimer;
-class G1ConcurrentMarkThread;
 class G1CollectedHeap;
+class G1ConcurrentMark;
+class G1ConcurrentMarkThread;
 class G1CMOopClosure;
 class G1CMTask;
-class G1ConcurrentMark;
 class G1OldTracer;
 class G1RegionToSpaceMapper;
 class G1SurvivorRegions;
@@ -96,11 +96,9 @@ typedef GenericTaskQueueSet<G1CMTaskQueue, mtGC> G1CMTaskQueueSet;
 // are alive. An instance is also embedded into the
 // reference processor as the _is_alive_non_header field
 class G1CMIsAliveClosure : public BoolObjectClosure {
-  G1CollectedHeap* _g1h;
   G1ConcurrentMark* _cm;
 public:
-  G1CMIsAliveClosure(G1CollectedHeap* g1h);
-  void set_concurrent_mark(G1ConcurrentMark* cm);
+  G1CMIsAliveClosure(G1ConcurrentMark* cm);
 
   bool do_object_b(oop obj);
 };
@@ -542,6 +540,8 @@ class G1ConcurrentMark : public CHeapObj<mtGC> {
 
   // Region statistics gathered during marking.
   G1RegionMarkStats* _region_mark_stats;
+  // Top pointer for each region at the start of marking. Must be valid for all committed
+  // regions.
   HeapWord* volatile* _top_at_mark_starts;
   // Top pointer for each region at the start of the rebuild remembered set process
   // for regions which remembered sets need to be rebuilt. A null for a given region
@@ -562,14 +562,16 @@ class G1ConcurrentMark : public CHeapObj<mtGC> {
   // Set live bytes for concurrent marking.
   void set_live_bytes(uint region, size_t live_bytes) { _region_mark_stats[region]._live_words = live_bytes / HeapWordSize; }
 
+  // Update the TAMS for the given region to the current top.
   inline void update_top_at_mark_start(HeapRegion* r);
+  // Reset the TAMS for the given region to bottom of that region.
   inline void reset_top_at_mark_start(HeapRegion* r);
+
   inline HeapWord* top_at_mark_start(const HeapRegion* r) const;
   inline HeapWord* top_at_mark_start(uint region) const;
+  // Returns whether the given object been allocated since marking start (i.e. >= TAMS in that region).
   inline bool obj_allocated_since_mark_start(oop obj) const;
 
-  void invalidate_top_at_mark_starts() NOT_DEBUG_RETURN;
-
   // Sets the internal top_at_region_start for the given region to current top of the region.
   inline void update_top_at_rebuild_start(HeapRegion* r);
   // TARS for the given region during remembered set rebuilding.

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

@@ -54,7 +54,7 @@ inline bool G1CMIsAliveClosure::do_object_b(oop obj) {
   }
 
   // All objects that are marked are live.
-  return _g1h->is_marked(obj);
+  return _cm->is_marked_in_bitmap(obj);
 }
 
 inline bool G1CMSubjectToDiscoveryClosure::do_object_b(oop obj) {

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

@@ -124,6 +124,8 @@ void HeapRegion::hr_clear(bool clear_space) {
 
   rem_set()->clear();
 
+  G1CollectedHeap::heap()->concurrent_mark()->clear_statistics(this);
+  
   _parsable_bottom = bottom();
   _garbage_bytes = 0;
 

src/hotspot/share/memory/iterator.hpp

@@ -41,7 +41,7 @@ class Thread;
 
 // The following classes are C++ `closures` for iterating over objects, roots and spaces
 
-class Closure : public StackObj { };
+class Closure : public CHeapObj<mtGC> { };
 
 // Thread iterator
 class ThreadClosure {

test/hotspot/jtreg/gc/g1/TestNoEagerReclaimOfHumongousRegions.java

@@ -34,7 +34,7 @@
  * @modules java.base/jdk.internal.misc
  * @build jdk.test.whitebox.WhiteBox
  * @run driver jdk.test.lib.helpers.ClassFileInstaller jdk.test.whitebox.WhiteBox
- * @run main/othervm -Xbootclasspath/a:. -Xlog:gc,gc+humongous=debug -XX:+UseG1GC -XX:MaxTenuringThreshold=0 -XX:+UnlockExperimentalVMOptions -XX:G1RemSetArrayOfCardsEntries=32 -XX:G1HeapRegionSize=1m -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI gc.g1.TestNoEagerReclaimOfHumongousRegions
+ * @run main/othervm -Xbootclasspath/a:. -Xlog:gc,gc+region=trace,gc+humongous=debug -XX:+UseG1GC -XX:MaxTenuringThreshold=0 -XX:+UnlockExperimentalVMOptions -XX:G1RemSetArrayOfCardsEntries=32 -XX:G1HeapRegionSize=1m -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI gc.g1.TestNoEagerReclaimOfHumongousRegions
  */
 
 import java.util.LinkedList;