8075466: SATB queue pre-filter verify found reclaimed humongous object
Summary: Removed pre-filter verify, and made filtering more careful.
Reviewed-by: brutisso, tschatzl
--- a/hotspot/src/share/vm/gc_implementation/g1/satbQueue.cpp Wed Apr 15 12:16:01 2015 -0400
+++ b/hotspot/src/share/vm/gc_implementation/g1/satbQueue.cpp Wed Apr 15 16:37:57 2015 -0400
@@ -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
@@ -33,30 +33,67 @@
#include "runtime/vmThread.hpp"
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.
+//
+// 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.
+//
+// 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.
//
-// * 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).
+// * A stale reference to an eagerly reclaimed humongous object. If a
+// humongous object is recorded and then reclaimed, the reference
+// becomes stale.
//
-// 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.
+// 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();
@@ -78,26 +115,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 always 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;)
}
}
@@ -184,17 +220,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
@@ -212,7 +237,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();
}
--- a/hotspot/src/share/vm/gc_implementation/g1/satbQueue.hpp Wed Apr 15 12:16:01 2015 -0400
+++ b/hotspot/src/share/vm/gc_implementation/g1/satbQueue.hpp Wed Apr 15 16:37:57 2015 -0400
@@ -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 {