6850846: G1: extend G1 marking verification

Summary: extend G1 marking verification to use either the "prev" or "next" marking information, as appropriate.
Reviewed-by: johnc, ysr

src/share/vm/gc_implementation/g1/concurrentMark.cpp

@@ -1157,6 +1157,13 @@ void ConcurrentMark::checkpointRootsFinal(bool clear_all_soft_refs) {
   } else {
     // We're done with marking.
     JavaThread::satb_mark_queue_set().set_active_all_threads(false);
+
+    if (VerifyDuringGC) {
+      g1h->prepare_for_verify();
+      g1h->verify(/* allow_dirty */      true,
+                  /* silent */           false,
+                  /* use_prev_marking */ false);
+    }
   }
 
 #if VERIFY_OBJS_PROCESSED
@@ -1747,12 +1754,12 @@ void ConcurrentMark::cleanup() {
   // races with it goes around and waits for completeCleanup to finish.
   g1h->increment_total_collections();
 
-#ifndef PRODUCT
   if (VerifyDuringGC) {
-    G1CollectedHeap::heap()->prepare_for_verify();
-    G1CollectedHeap::heap()->verify(true,false);
+    g1h->prepare_for_verify();
+    g1h->verify(/* allow_dirty */      true,
+                /* silent */           false,
+                /* use_prev_marking */ true);
   }
-#endif
 }
 
 void ConcurrentMark::completeCleanup() {

src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp

@@ -2136,17 +2136,22 @@ public:
 };
 
 class VerifyObjsInRegionClosure: public ObjectClosure {
+private:
   G1CollectedHeap* _g1h;
   size_t _live_bytes;
   HeapRegion *_hr;
+  bool _use_prev_marking;
 public:
-  VerifyObjsInRegionClosure(HeapRegion *hr) : _live_bytes(0), _hr(hr) {
+  // use_prev_marking == true  -> use "prev" marking information,
+  // use_prev_marking == false -> use "next" marking information
+  VerifyObjsInRegionClosure(HeapRegion *hr, bool use_prev_marking)
+    : _live_bytes(0), _hr(hr), _use_prev_marking(use_prev_marking) {
     _g1h = G1CollectedHeap::heap();
   }
   void do_object(oop o) {
     VerifyLivenessOopClosure isLive(_g1h);
     assert(o != NULL, "Huh?");
-    if (!_g1h->is_obj_dead(o)) {
+    if (!_g1h->is_obj_dead_cond(o, _use_prev_marking)) {
       o->oop_iterate(&isLive);
       if (!_hr->obj_allocated_since_prev_marking(o))
         _live_bytes += (o->size() * HeapWordSize);
@@ -2185,17 +2190,22 @@ public:
 };
 
 class VerifyRegionClosure: public HeapRegionClosure {
-public:
+private:
   bool _allow_dirty;
   bool _par;
-  VerifyRegionClosure(bool allow_dirty, bool par = false)
-    : _allow_dirty(allow_dirty), _par(par) {}
+  bool _use_prev_marking;
+public:
+  // use_prev_marking == true  -> use "prev" marking information,
+  // use_prev_marking == false -> use "next" marking information
+  VerifyRegionClosure(bool allow_dirty, bool par, bool use_prev_marking)
+    : _allow_dirty(allow_dirty), _par(par),
+      _use_prev_marking(use_prev_marking) {}
   bool doHeapRegion(HeapRegion* r) {
     guarantee(_par || r->claim_value() == HeapRegion::InitialClaimValue,
               "Should be unclaimed at verify points.");
     if (!r->continuesHumongous()) {
-      VerifyObjsInRegionClosure not_dead_yet_cl(r);
-      r->verify(_allow_dirty);
+      VerifyObjsInRegionClosure not_dead_yet_cl(r, _use_prev_marking);
+      r->verify(_allow_dirty, _use_prev_marking);
       r->object_iterate(&not_dead_yet_cl);
       guarantee(r->max_live_bytes() >= not_dead_yet_cl.live_bytes(),
                 "More live objects than counted in last complete marking.");
@@ -2208,10 +2218,13 @@ class VerifyRootsClosure: public OopsInGenClosure {
 private:
   G1CollectedHeap* _g1h;
   bool             _failures;
-
+  bool             _use_prev_marking;
 public:
-  VerifyRootsClosure() :
-    _g1h(G1CollectedHeap::heap()), _failures(false) { }
+  // use_prev_marking == true  -> use "prev" marking information,
+  // use_prev_marking == false -> use "next" marking information
+  VerifyRootsClosure(bool use_prev_marking) :
+    _g1h(G1CollectedHeap::heap()), _failures(false),
+    _use_prev_marking(use_prev_marking) { }
 
   bool failures() { return _failures; }
 
@@ -2222,7 +2235,7 @@ public:
   void do_oop(oop* p) {
     oop obj = *p;
     if (obj != NULL) {
-      if (_g1h->is_obj_dead(obj)) {
+      if (_g1h->is_obj_dead_cond(obj, _use_prev_marking)) {
         gclog_or_tty->print_cr("Root location "PTR_FORMAT" "
                                "points to dead obj "PTR_FORMAT, p, (void*) obj);
         obj->print_on(gclog_or_tty);
@@ -2238,24 +2251,35 @@ class G1ParVerifyTask: public AbstractGangTask {
 private:
   G1CollectedHeap* _g1h;
   bool _allow_dirty;
+  bool _use_prev_marking;
 
 public:
-  G1ParVerifyTask(G1CollectedHeap* g1h, bool allow_dirty) :
+  // use_prev_marking == true  -> use "prev" marking information,
+  // use_prev_marking == false -> use "next" marking information
+  G1ParVerifyTask(G1CollectedHeap* g1h, bool allow_dirty,
+                  bool use_prev_marking) :
     AbstractGangTask("Parallel verify task"),
-    _g1h(g1h), _allow_dirty(allow_dirty) { }
+    _g1h(g1h), _allow_dirty(allow_dirty),
+    _use_prev_marking(use_prev_marking) { }
 
   void work(int worker_i) {
     HandleMark hm;
-    VerifyRegionClosure blk(_allow_dirty, true);
+    VerifyRegionClosure blk(_allow_dirty, true, _use_prev_marking);
     _g1h->heap_region_par_iterate_chunked(&blk, worker_i,
                                           HeapRegion::ParVerifyClaimValue);
   }
 };
 
 void G1CollectedHeap::verify(bool allow_dirty, bool silent) {
+  verify(allow_dirty, silent, /* use_prev_marking */ true);
+}
+
+void G1CollectedHeap::verify(bool allow_dirty,
+                             bool silent,
+                             bool use_prev_marking) {
   if (SafepointSynchronize::is_at_safepoint() || ! UseTLAB) {
     if (!silent) { gclog_or_tty->print("roots "); }
-    VerifyRootsClosure rootsCl;
+    VerifyRootsClosure rootsCl(use_prev_marking);
     process_strong_roots(false,
                          SharedHeap::SO_AllClasses,
                          &rootsCl,
@@ -2266,7 +2290,7 @@ void G1CollectedHeap::verify(bool allow_dirty, bool silent) {
       assert(check_heap_region_claim_values(HeapRegion::InitialClaimValue),
              "sanity check");
 
-      G1ParVerifyTask task(this, allow_dirty);
+      G1ParVerifyTask task(this, allow_dirty, use_prev_marking);
       int n_workers = workers()->total_workers();
       set_par_threads(n_workers);
       workers()->run_task(&task);
@@ -2280,7 +2304,7 @@ void G1CollectedHeap::verify(bool allow_dirty, bool silent) {
       assert(check_heap_region_claim_values(HeapRegion::InitialClaimValue),
              "sanity check");
     } else {
-      VerifyRegionClosure blk(allow_dirty);
+      VerifyRegionClosure blk(allow_dirty, false, use_prev_marking);
       _hrs->iterate(&blk);
     }
     if (!silent) gclog_or_tty->print("remset ");

src/share/vm/gc_implementation/g1/g1CollectedHeap.hpp

@@ -1049,6 +1049,17 @@ public:
   virtual void prepare_for_verify();
 
   // Perform verification.
+
+  // use_prev_marking == true  -> use "prev" marking information,
+  // use_prev_marking == false -> use "next" marking information
+  // NOTE: Only the "prev" marking information is guaranteed to be
+  // consistent most of the time, so most calls to this should use
+  // use_prev_marking == true. Currently, there is only one case where
+  // this is called with use_prev_marking == false, which is to verify
+  // the "next" marking information at the end of remark.
+  void verify(bool allow_dirty, bool silent, bool use_prev_marking);
+
+  // Override; it uses the "prev" marking information
   virtual void verify(bool allow_dirty, bool silent);
   virtual void print() const;
   virtual void print_on(outputStream* st) const;
@@ -1125,6 +1136,18 @@ public:
   bool isMarkedPrev(oop obj) const;
   bool isMarkedNext(oop obj) const;
 
+  // use_prev_marking == true  -> use "prev" marking information,
+  // use_prev_marking == false -> use "next" marking information
+  bool is_obj_dead_cond(const oop obj,
+                        const HeapRegion* hr,
+                        const bool use_prev_marking) const {
+    if (use_prev_marking) {
+      return is_obj_dead(obj, hr);
+    } else {
+      return is_obj_ill(obj, hr);
+    }
+  }
+
   // Determine if an object is dead, given the object and also
   // the region to which the object belongs. An object is dead
   // iff a) it was not allocated since the last mark and b) it
@@ -1162,8 +1185,19 @@ public:
   // Added if it is in permanent gen it isn't dead.
   // Added if it is NULL it isn't dead.
 
-  bool is_obj_dead(oop obj) {
-    HeapRegion* hr = heap_region_containing(obj);
+  // use_prev_marking == true  -> use "prev" marking information,
+  // use_prev_marking == false -> use "next" marking information
+  bool is_obj_dead_cond(const oop obj,
+                        const bool use_prev_marking) {
+    if (use_prev_marking) {
+      return is_obj_dead(obj);
+    } else {
+      return is_obj_ill(obj);
+    }
+  }
+
+  bool is_obj_dead(const oop obj) {
+    const HeapRegion* hr = heap_region_containing(obj);
     if (hr == NULL) {
       if (Universe::heap()->is_in_permanent(obj))
         return false;
@@ -1173,8 +1207,8 @@ public:
     else return is_obj_dead(obj, hr);
   }
 
-  bool is_obj_ill(oop obj) {
-    HeapRegion* hr = heap_region_containing(obj);
+  bool is_obj_ill(const oop obj) {
+    const HeapRegion* hr = heap_region_containing(obj);
     if (hr == NULL) {
       if (Universe::heap()->is_in_permanent(obj))
         return false;

src/share/vm/gc_implementation/g1/heapRegion.cpp

@@ -40,15 +40,19 @@ FilterOutOfRegionClosure::FilterOutOfRegionClosure(HeapRegion* r,
 {}
 
 class VerifyLiveClosure: public OopClosure {
+private:
   G1CollectedHeap* _g1h;
   CardTableModRefBS* _bs;
   oop _containing_obj;
   bool _failures;
   int _n_failures;
+  bool _use_prev_marking;
 public:
-  VerifyLiveClosure(G1CollectedHeap* g1h) :
+  // use_prev_marking == true  -> use "prev" marking information,
+  // use_prev_marking == false -> use "next" marking information
+  VerifyLiveClosure(G1CollectedHeap* g1h, bool use_prev_marking) :
     _g1h(g1h), _bs(NULL), _containing_obj(NULL),
-    _failures(false), _n_failures(0)
+    _failures(false), _n_failures(0), _use_prev_marking(use_prev_marking)
   {
     BarrierSet* bs = _g1h->barrier_set();
     if (bs->is_a(BarrierSet::CardTableModRef))
@@ -68,11 +72,13 @@ public:
 
   void do_oop(oop* p) {
     assert(_containing_obj != NULL, "Precondition");
-    assert(!_g1h->is_obj_dead(_containing_obj), "Precondition");
+    assert(!_g1h->is_obj_dead_cond(_containing_obj, _use_prev_marking),
+           "Precondition");
     oop obj = *p;
     if (obj != NULL) {
       bool failed = false;
-      if (!_g1h->is_in_closed_subset(obj) || _g1h->is_obj_dead(obj)) {
+      if (!_g1h->is_in_closed_subset(obj) ||
+          _g1h->is_obj_dead_cond(obj, _use_prev_marking)) {
         if (!_failures) {
           gclog_or_tty->print_cr("");
           gclog_or_tty->print_cr("----------");
@@ -647,19 +653,23 @@ void HeapRegion::print_on(outputStream* st) const {
   G1OffsetTableContigSpace::print_on(st);
 }
 
+void HeapRegion::verify(bool allow_dirty) const {
+  verify(allow_dirty, /* use_prev_marking */ true);
+}
+
 #define OBJ_SAMPLE_INTERVAL 0
 #define BLOCK_SAMPLE_INTERVAL 100
 
 // This really ought to be commoned up into OffsetTableContigSpace somehow.
 // We would need a mechanism to make that code skip dead objects.
 
-void HeapRegion::verify(bool allow_dirty) const {
+void HeapRegion::verify(bool allow_dirty, bool use_prev_marking) const {
   G1CollectedHeap* g1 = G1CollectedHeap::heap();
   HeapWord* p = bottom();
   HeapWord* prev_p = NULL;
   int objs = 0;
   int blocks = 0;
-  VerifyLiveClosure vl_cl(g1);
+  VerifyLiveClosure vl_cl(g1, use_prev_marking);
   while (p < top()) {
     size_t size = oop(p)->size();
     if (blocks == BLOCK_SAMPLE_INTERVAL) {
@@ -671,7 +681,7 @@ void HeapRegion::verify(bool allow_dirty) const {
     }
     if (objs == OBJ_SAMPLE_INTERVAL) {
       oop obj = oop(p);
-      if (!g1->is_obj_dead(obj, this)) {
+      if (!g1->is_obj_dead_cond(obj, this, use_prev_marking)) {
         obj->verify();
         vl_cl.set_containing_obj(obj);
         obj->oop_iterate(&vl_cl);

src/share/vm/gc_implementation/g1/heapRegion.hpp

@@ -782,7 +782,16 @@ class HeapRegion: public G1OffsetTableContigSpace {
   void print() const;
   void print_on(outputStream* st) const;
 
-  // Override
+  // use_prev_marking == true  -> use "prev" marking information,
+  // use_prev_marking == false -> use "next" marking information
+  // NOTE: Only the "prev" marking information is guaranteed to be
+  // consistent most of the time, so most calls to this should use
+  // use_prev_marking == true. Currently, there is only one case where
+  // this is called with use_prev_marking == false, which is to verify
+  // the "next" marking information at the end of remark.
+  void verify(bool allow_dirty, bool use_prev_marking) const;
+
+  // Override; it uses the "prev" marking information
   virtual void verify(bool allow_dirty) const;
 
 #ifdef DEBUG