# HG changeset patch
# User kbarrett
# Date 1429130277 14400
#      Wed Apr 15 16:37:57 2015 -0400
# Node ID f3a595af5c8f69e62ca1a3ac2ba70ace56269d9a
# Parent  bb2eff506faeca23b54d9f202d1770e5cb6b0a6a
8075466: SATB queue pre-filter verify found reclaimed humongous object
Summary: Removed pre-filter verify, and made filtering more careful.
Reviewed-by: brutisso, tschatzl

diff --git a/src/share/vm/gc_implementation/g1/satbQueue.cpp b/src/share/vm/gc_implementation/g1/satbQueue.cpp
--- a/src/share/vm/gc_implementation/g1/satbQueue.cpp
+++ b/src/share/vm/gc_implementation/g1/satbQueue.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2001, 2014, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2001, 2015, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -35,30 +35,67 @@
 PRAGMA_FORMAT_MUTE_WARNINGS_FOR_GCC
 
 void ObjPtrQueue::flush() {
-  // The buffer might contain refs into the CSet. We have to filter it
-  // first before we flush it, otherwise we might end up with an
-  // enqueued buffer with refs into the CSet which breaks our invariants.
+  // Filter now to possibly save work later.  If filtering empties the
+  // buffer then flush_impl can deallocate the buffer.
   filter();
   flush_impl();
 }
 
-// This method removes entries from an SATB buffer that will not be
-// useful to the concurrent marking threads. An entry is removed if it
-// satisfies one of the following conditions:
+// Return true if a SATB buffer entry refers to an object that
+// requires marking.
 //
-// * it points to an object outside the G1 heap (G1's concurrent
-//     marking only visits objects inside the G1 heap),
-// * it points to an object that has been allocated since marking
-//     started (according to SATB those objects do not need to be
-//     visited during marking), or
-// * it points to an object that has already been marked (no need to
-//     process it again).
+// The entry must point into the G1 heap.  In particular, it must not
+// be a NULL pointer.  NULL pointers are pre-filtered and never
+// inserted into a SATB buffer.
 //
-// The rest of the entries will be retained and are compacted towards
-// the top of the buffer. Note that, because we do not allow old
-// regions in the CSet during marking, all objects on the CSet regions
-// are young (eden or survivors) and therefore implicitly live. So any
-// references into the CSet will be removed during filtering.
+// An entry that is below the NTAMS pointer for the containing heap
+// region requires marking. Such an entry must point to a valid object.
+//
+// An entry that is at least the NTAMS pointer for the containing heap
+// region might be any of the following, none of which should be marked.
+//
+// * A reference to an object allocated since marking started.
+//   According to SATB, such objects are implicitly kept live and do
+//   not need to be dealt with via SATB buffer processing.
+//
+// * A reference to a young generation object. Young objects are
+//   handled separately and are not marked by concurrent marking.
+//
+// * A stale reference to a young generation object. If a young
+//   generation object reference is recorded and not filtered out
+//   before being moved by a young collection, the reference becomes
+//   stale.
+//
+// * A stale reference to an eagerly reclaimed humongous object.  If a
+//   humongous object is recorded and then reclaimed, the reference
+//   becomes stale.
+//
+// The stale reference cases are implicitly handled by the NTAMS
+// comparison. Because of the possibility of stale references, buffer
+// processing must be somewhat circumspect and not assume entries
+// in an unfiltered buffer refer to valid objects.
+
+inline bool requires_marking(const void* entry, G1CollectedHeap* heap) {
+  // Includes rejection of NULL pointers.
+  assert(heap->is_in_reserved(entry),
+         err_msg("Non-heap pointer in SATB buffer: " PTR_FORMAT, p2i(entry)));
+
+  HeapRegion* region = heap->heap_region_containing_raw(entry);
+  assert(region != NULL, err_msg("No region for " PTR_FORMAT, p2i(entry)));
+  if (entry >= region->next_top_at_mark_start()) {
+    return false;
+  }
+
+  assert(((oop)entry)->is_oop(true /* ignore mark word */),
+         err_msg("Invalid oop in SATB buffer: " PTR_FORMAT, p2i(entry)));
+
+  return true;
+}
+
+// This method removes entries from a SATB buffer that will not be
+// useful to the concurrent marking threads.  Entries are retained if
+// they require marking and are not already marked. Retained entries
+// are compacted toward the top of the buffer.
 
 void ObjPtrQueue::filter() {
   G1CollectedHeap* g1h = G1CollectedHeap::heap();
@@ -80,26 +117,25 @@
     assert(i > 0, "we should have at least one more entry to process");
     i -= oopSize;
     debug_only(entries += 1;)
-    oop* p = (oop*) &buf[byte_index_to_index((int) i)];
-    oop obj = *p;
+    void** p = &buf[byte_index_to_index((int) i)];
+    void* entry = *p;
     // NULL the entry so that unused parts of the buffer contain NULLs
     // at the end. If we are going to retain it we will copy it to its
     // final place. If we have retained all entries we have visited so
     // far, we'll just end up copying it to the same place.
     *p = NULL;
 
-    bool retain = g1h->is_obj_ill(obj);
-    if (retain) {
+    if (requires_marking(entry, g1h) && !g1h->isMarkedNext((oop)entry)) {
       assert(new_index > 0, "we should not have already filled up the buffer");
       new_index -= oopSize;
       assert(new_index >= i,
              "new_index should never be below i, as we alwaysr compact 'up'");
-      oop* new_p = (oop*) &buf[byte_index_to_index((int) new_index)];
+      void** new_p = &buf[byte_index_to_index((int) new_index)];
       assert(new_p >= p, "the destination location should never be below "
              "the source as we always compact 'up'");
       assert(*new_p == NULL,
              "we should have already cleared the destination location");
-      *new_p = obj;
+      *new_p = entry;
       debug_only(retained += 1;)
     }
   }
@@ -186,17 +222,6 @@
 }
 #endif // PRODUCT
 
-#ifdef ASSERT
-void ObjPtrQueue::verify_oops_in_buffer() {
-  if (_buf == NULL) return;
-  for (size_t i = _index; i < _sz; i += oopSize) {
-    oop obj = (oop)_buf[byte_index_to_index((int)i)];
-    assert(obj != NULL && obj->is_oop(true /* ignore mark word */),
-           "Not an oop");
-  }
-}
-#endif
-
 #ifdef _MSC_VER // the use of 'this' below gets a warning, make it go away
 #pragma warning( disable:4355 ) // 'this' : used in base member initializer list
 #endif // _MSC_VER
@@ -216,7 +241,6 @@
 }
 
 void SATBMarkQueueSet::handle_zero_index_for_thread(JavaThread* t) {
-  DEBUG_ONLY(t->satb_mark_queue().verify_oops_in_buffer();)
   t->satb_mark_queue().handle_zero_index();
 }
 
diff --git a/src/share/vm/gc_implementation/g1/satbQueue.hpp b/src/share/vm/gc_implementation/g1/satbQueue.hpp
--- a/src/share/vm/gc_implementation/g1/satbQueue.hpp
+++ b/src/share/vm/gc_implementation/g1/satbQueue.hpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2001, 2014, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2001, 2015, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -72,8 +72,6 @@
   void print(const char* name);
   static void print(const char* name, void** buf, size_t index, size_t sz);
 #endif // PRODUCT
-
-  void verify_oops_in_buffer() NOT_DEBUG_RETURN;
 };
 
 class SATBMarkQueueSet: public PtrQueueSet {