From 761f81455beb7772d1ff0585d67297deb8de83cf Mon Sep 17 00:00:00 2001 From: kbarrett Date: Wed, 22 Apr 2015 14:06:49 -0400 Subject: [PATCH] 8078023: verify_no_cset_oops found reclaimed humongous object in SATB buffer Summary: Removed no longer valid checking of SATB buffers Reviewed-by: jmasa, pliden --- .../gc_implementation/g1/concurrentMark.cpp | 111 +++++++----------- .../gc_implementation/g1/concurrentMark.hpp | 11 +- .../gc_implementation/g1/g1CollectedHeap.cpp | 24 +--- .../vm/gc_implementation/g1/satbQueue.cpp | 28 ----- .../vm/gc_implementation/g1/satbQueue.hpp | 9 -- 5 files changed, 49 insertions(+), 134 deletions(-) diff --git a/src/share/vm/gc_implementation/g1/concurrentMark.cpp b/src/share/vm/gc_implementation/g1/concurrentMark.cpp index e57367436..45fceb837 100644 --- a/src/share/vm/gc_implementation/g1/concurrentMark.cpp +++ b/src/share/vm/gc_implementation/g1/concurrentMark.cpp @@ -3059,9 +3059,7 @@ ConcurrentMark::claim_region(uint worker_id) { #ifndef PRODUCT enum VerifyNoCSetOopsPhase { VerifyNoCSetOopsStack, - VerifyNoCSetOopsQueues, - VerifyNoCSetOopsSATBCompleted, - VerifyNoCSetOopsSATBThread + VerifyNoCSetOopsQueues }; class VerifyNoCSetOopsClosure : public OopClosure, public ObjectClosure { @@ -3074,8 +3072,6 @@ private: switch (_phase) { case VerifyNoCSetOopsStack: return "Stack"; case VerifyNoCSetOopsQueues: return "Queue"; - case VerifyNoCSetOopsSATBCompleted: return "Completed SATB Buffers"; - case VerifyNoCSetOopsSATBThread: return "Thread SATB Buffers"; default: ShouldNotReachHere(); } return NULL; @@ -3102,7 +3098,7 @@ public: virtual void do_oop(narrowOop* p) { // We should not come across narrow oops while scanning marking - // stacks and SATB buffers. + // stacks ShouldNotReachHere(); } @@ -3111,10 +3107,7 @@ public: } }; -void ConcurrentMark::verify_no_cset_oops(bool verify_stacks, - bool verify_enqueued_buffers, - bool verify_thread_buffers, - bool verify_fingers) { +void ConcurrentMark::verify_no_cset_oops() { assert(SafepointSynchronize::is_at_safepoint(), "should be at a safepoint"); if (!G1CollectedHeap::heap()->mark_in_progress()) { return; @@ -3122,65 +3115,47 @@ void ConcurrentMark::verify_no_cset_oops(bool verify_stacks, VerifyNoCSetOopsClosure cl; - if (verify_stacks) { - // Verify entries on the global mark stack - cl.set_phase(VerifyNoCSetOopsStack); - _markStack.oops_do(&cl); + // Verify entries on the global mark stack + cl.set_phase(VerifyNoCSetOopsStack); + _markStack.oops_do(&cl); - // Verify entries on the task queues - for (uint i = 0; i < _max_worker_id; i += 1) { - cl.set_phase(VerifyNoCSetOopsQueues, i); - CMTaskQueue* queue = _task_queues->queue(i); - queue->oops_do(&cl); - } - } - - SATBMarkQueueSet& satb_qs = JavaThread::satb_mark_queue_set(); - - // Verify entries on the enqueued SATB buffers - if (verify_enqueued_buffers) { - cl.set_phase(VerifyNoCSetOopsSATBCompleted); - satb_qs.iterate_completed_buffers_read_only(&cl); - } - - // Verify entries on the per-thread SATB buffers - if (verify_thread_buffers) { - cl.set_phase(VerifyNoCSetOopsSATBThread); - satb_qs.iterate_thread_buffers_read_only(&cl); - } - - if (verify_fingers) { - // Verify the global finger - HeapWord* global_finger = finger(); - if (global_finger != NULL && global_finger < _heap_end) { - // The global finger always points to a heap region boundary. We - // use heap_region_containing_raw() to get the containing region - // given that the global finger could be pointing to a free region - // which subsequently becomes continues humongous. If that - // happens, heap_region_containing() will return the bottom of the - // corresponding starts humongous region and the check below will - // not hold any more. - // Since we always iterate over all regions, we might get a NULL HeapRegion - // here. - HeapRegion* global_hr = _g1h->heap_region_containing_raw(global_finger); - guarantee(global_hr == NULL || global_finger == global_hr->bottom(), - err_msg("global finger: "PTR_FORMAT" region: "HR_FORMAT, - p2i(global_finger), HR_FORMAT_PARAMS(global_hr))); - } - - // Verify the task fingers - assert(parallel_marking_threads() <= _max_worker_id, "sanity"); - for (int i = 0; i < (int) parallel_marking_threads(); i += 1) { - CMTask* task = _tasks[i]; - HeapWord* task_finger = task->finger(); - if (task_finger != NULL && task_finger < _heap_end) { - // See above note on the global finger verification. - HeapRegion* task_hr = _g1h->heap_region_containing_raw(task_finger); - guarantee(task_hr == NULL || task_finger == task_hr->bottom() || - !task_hr->in_collection_set(), - err_msg("task finger: "PTR_FORMAT" region: "HR_FORMAT, - p2i(task_finger), HR_FORMAT_PARAMS(task_hr))); - } + // Verify entries on the task queues + for (uint i = 0; i < _max_worker_id; i += 1) { + cl.set_phase(VerifyNoCSetOopsQueues, i); + CMTaskQueue* queue = _task_queues->queue(i); + queue->oops_do(&cl); + } + + // Verify the global finger + HeapWord* global_finger = finger(); + if (global_finger != NULL && global_finger < _heap_end) { + // The global finger always points to a heap region boundary. We + // use heap_region_containing_raw() to get the containing region + // given that the global finger could be pointing to a free region + // which subsequently becomes continues humongous. If that + // happens, heap_region_containing() will return the bottom of the + // corresponding starts humongous region and the check below will + // not hold any more. + // Since we always iterate over all regions, we might get a NULL HeapRegion + // here. + HeapRegion* global_hr = _g1h->heap_region_containing_raw(global_finger); + guarantee(global_hr == NULL || global_finger == global_hr->bottom(), + err_msg("global finger: "PTR_FORMAT" region: "HR_FORMAT, + p2i(global_finger), HR_FORMAT_PARAMS(global_hr))); + } + + // Verify the task fingers + assert(parallel_marking_threads() <= _max_worker_id, "sanity"); + for (int i = 0; i < (int) parallel_marking_threads(); i += 1) { + CMTask* task = _tasks[i]; + HeapWord* task_finger = task->finger(); + if (task_finger != NULL && task_finger < _heap_end) { + // See above note on the global finger verification. + HeapRegion* task_hr = _g1h->heap_region_containing_raw(task_finger); + guarantee(task_hr == NULL || task_finger == task_hr->bottom() || + !task_hr->in_collection_set(), + err_msg("task finger: "PTR_FORMAT" region: "HR_FORMAT, + p2i(task_finger), HR_FORMAT_PARAMS(task_hr))); } } } diff --git a/src/share/vm/gc_implementation/g1/concurrentMark.hpp b/src/share/vm/gc_implementation/g1/concurrentMark.hpp index b1c6674a8..0663ebe96 100644 --- a/src/share/vm/gc_implementation/g1/concurrentMark.hpp +++ b/src/share/vm/gc_implementation/g1/concurrentMark.hpp @@ -793,14 +793,9 @@ public: } // Verify that there are no CSet oops on the stacks (taskqueues / - // global mark stack), enqueued SATB buffers, per-thread SATB - // buffers, and fingers (global / per-task). The boolean parameters - // decide which of the above data structures to verify. If marking - // is not in progress, it's a no-op. - void verify_no_cset_oops(bool verify_stacks, - bool verify_enqueued_buffers, - bool verify_thread_buffers, - bool verify_fingers) PRODUCT_RETURN; + // global mark stack) and fingers (global / per-task). + // If marking is not in progress, it's a no-op. + void verify_no_cset_oops() PRODUCT_RETURN; bool isPrevMarked(oop p) const { assert(p != NULL && p->is_oop(), "expected an oop"); diff --git a/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp b/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp index a659aad10..06b660cfc 100644 --- a/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp +++ b/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp @@ -4093,14 +4093,9 @@ G1CollectedHeap::do_collection_pause_at_safepoint(double target_pause_time_ms) { assert(check_cset_fast_test(), "Inconsistency in the InCSetState table."); _cm->note_start_of_gc(); - // We should not verify the per-thread SATB buffers given that - // we have not filtered them yet (we'll do so during the - // GC). We also call this after finalize_cset() to + // We call this after finalize_cset() to // ensure that the CSet has been finalized. - _cm->verify_no_cset_oops(true /* verify_stacks */, - true /* verify_enqueued_buffers */, - false /* verify_thread_buffers */, - true /* verify_fingers */); + _cm->verify_no_cset_oops(); if (_hr_printer.is_active()) { HeapRegion* hr = g1_policy()->collection_set(); @@ -4123,16 +4118,6 @@ G1CollectedHeap::do_collection_pause_at_safepoint(double target_pause_time_ms) { // Actually do the work... evacuate_collection_set(evacuation_info); - // We do this to mainly verify the per-thread SATB buffers - // (which have been filtered by now) since we didn't verify - // them earlier. No point in re-checking the stacks / enqueued - // buffers given that the CSet has not changed since last time - // we checked. - _cm->verify_no_cset_oops(false /* verify_stacks */, - false /* verify_enqueued_buffers */, - true /* verify_thread_buffers */, - true /* verify_fingers */); - free_collection_set(g1_policy()->collection_set(), evacuation_info); eagerly_reclaim_humongous_regions(); @@ -4215,10 +4200,7 @@ G1CollectedHeap::do_collection_pause_at_safepoint(double target_pause_time_ms) { // We redo the verification but now wrt to the new CSet which // has just got initialized after the previous CSet was freed. - _cm->verify_no_cset_oops(true /* verify_stacks */, - true /* verify_enqueued_buffers */, - true /* verify_thread_buffers */, - true /* verify_fingers */); + _cm->verify_no_cset_oops(); _cm->note_end_of_gc(); // This timing is only used by the ergonomics to handle our pause target. diff --git a/src/share/vm/gc_implementation/g1/satbQueue.cpp b/src/share/vm/gc_implementation/g1/satbQueue.cpp index 62e77d22e..2b89835f5 100644 --- a/src/share/vm/gc_implementation/g1/satbQueue.cpp +++ b/src/share/vm/gc_implementation/g1/satbQueue.cpp @@ -182,12 +182,6 @@ bool ObjPtrQueue::should_enqueue_buffer() { return should_enqueue; } -void ObjPtrQueue::apply_closure(ObjectClosure* cl) { - if (_buf != NULL) { - apply_closure_to_buffer(cl, _buf, _index, _sz); - } -} - void ObjPtrQueue::apply_closure_and_empty(ObjectClosure* cl) { if (_buf != NULL) { apply_closure_to_buffer(cl, _buf, _index, _sz); @@ -319,28 +313,6 @@ bool SATBMarkQueueSet::apply_closure_to_completed_buffer(ObjectClosure* cl) { } } -void SATBMarkQueueSet::iterate_completed_buffers_read_only(ObjectClosure* cl) { - assert(SafepointSynchronize::is_at_safepoint(), "Must be at safepoint."); - assert(cl != NULL, "pre-condition"); - - BufferNode* nd = _completed_buffers_head; - while (nd != NULL) { - void** buf = BufferNode::make_buffer_from_node(nd); - ObjPtrQueue::apply_closure_to_buffer(cl, buf, 0, _sz); - nd = nd->next(); - } -} - -void SATBMarkQueueSet::iterate_thread_buffers_read_only(ObjectClosure* cl) { - assert(SafepointSynchronize::is_at_safepoint(), "Must be at safepoint."); - assert(cl != NULL, "pre-condition"); - - for (JavaThread* t = Threads::first(); t; t = t->next()) { - t->satb_mark_queue().apply_closure(cl); - } - shared_satb_queue()->apply_closure(cl); -} - #ifndef PRODUCT // Helpful for debugging diff --git a/src/share/vm/gc_implementation/g1/satbQueue.hpp b/src/share/vm/gc_implementation/g1/satbQueue.hpp index f98c34579..596904d06 100644 --- a/src/share/vm/gc_implementation/g1/satbQueue.hpp +++ b/src/share/vm/gc_implementation/g1/satbQueue.hpp @@ -41,9 +41,6 @@ private: // Filter out unwanted entries from the buffer. void filter(); - // Apply the closure to all elements. - void apply_closure(ObjectClosure* cl); - // Apply the closure to all elements and empty the buffer; void apply_closure_and_empty(ObjectClosure* cl); @@ -105,12 +102,6 @@ public: // completed buffers exist, return false. bool apply_closure_to_completed_buffer(ObjectClosure* closure); - // Apply the given closure on enqueued and currently-active buffers - // respectively. Both methods are read-only, i.e., they do not - // modify any of the buffers. - void iterate_completed_buffers_read_only(ObjectClosure* cl); - void iterate_thread_buffers_read_only(ObjectClosure* cl); - #ifndef PRODUCT // Helpful for debugging void print_all(const char* msg); -- GitLab