From d5318930e134782e9c1fc5432a45b86c10b73c09 Mon Sep 17 00:00:00 2001 From: tonyp Date: Wed, 6 Aug 2008 11:57:31 -0400 Subject: [PATCH] 6722565: G1: assert !r->is_on_unclean_list() fires Summary: Under certain circumstances, two cleanup threads can claim and process the same region. Reviewed-by: apetrusenko, ysr --- .../gc_implementation/g1/concurrentMark.cpp | 26 +++- .../gc_implementation/g1/g1CollectedHeap.cpp | 137 +++++++++++++++--- .../gc_implementation/g1/g1CollectedHeap.hpp | 5 +- .../g1/g1CollectorPolicy.cpp | 6 +- .../vm/gc_implementation/g1/heapRegion.cpp | 13 +- .../vm/gc_implementation/g1/heapRegion.hpp | 22 ++- 6 files changed, 168 insertions(+), 41 deletions(-) diff --git a/src/share/vm/gc_implementation/g1/concurrentMark.cpp b/src/share/vm/gc_implementation/g1/concurrentMark.cpp index d4975e8b6..036e0e7fc 100644 --- a/src/share/vm/gc_implementation/g1/concurrentMark.cpp +++ b/src/share/vm/gc_implementation/g1/concurrentMark.cpp @@ -1423,7 +1423,8 @@ public: NULL /* CO tracker */); calccl.no_yield(); if (ParallelGCThreads > 0) { - _g1h->heap_region_par_iterate_chunked(&calccl, i, 1); + _g1h->heap_region_par_iterate_chunked(&calccl, i, + HeapRegion::FinalCountClaimValue); } else { _g1h->heap_region_iterate(&calccl); } @@ -1502,7 +1503,8 @@ public: &_par_cleanup_thread_state[i]->list, i); if (ParallelGCThreads > 0) { - _g1h->heap_region_par_iterate_chunked(&g1_note_end, i, 2); + _g1h->heap_region_par_iterate_chunked(&g1_note_end, i, + HeapRegion::NoteEndClaimValue); } else { _g1h->heap_region_iterate(&g1_note_end); } @@ -1545,7 +1547,8 @@ public: void work(int i) { if (ParallelGCThreads > 0) { - _g1rs->scrub_par(_region_bm, _card_bm, i, 3); + _g1rs->scrub_par(_region_bm, _card_bm, i, + HeapRegion::ScrubRemSetClaimValue); } else { _g1rs->scrub(_region_bm, _card_bm); } @@ -1610,10 +1613,18 @@ void ConcurrentMark::cleanup() { G1ParFinalCountTask g1_par_count_task(g1h, nextMarkBitMap(), &_region_bm, &_card_bm); if (ParallelGCThreads > 0) { + assert(g1h->check_heap_region_claim_values( + HeapRegion::InitialClaimValue), + "sanity check"); + int n_workers = g1h->workers()->total_workers(); g1h->set_par_threads(n_workers); g1h->workers()->run_task(&g1_par_count_task); g1h->set_par_threads(0); + + assert(g1h->check_heap_region_claim_values( + HeapRegion::FinalCountClaimValue), + "sanity check"); } else { g1_par_count_task.work(0); } @@ -1654,6 +1665,9 @@ void ConcurrentMark::cleanup() { g1h->set_par_threads(n_workers); g1h->workers()->run_task(&g1_par_note_end_task); g1h->set_par_threads(0); + + assert(g1h->check_heap_region_claim_values(HeapRegion::NoteEndClaimValue), + "sanity check"); } else { g1_par_note_end_task.work(0); } @@ -1665,7 +1679,7 @@ void ConcurrentMark::cleanup() { (note_end_end - note_end_start)*1000.0); } - // Now we "scrub" remembered sets. Note that we must do this before the + // call below, since it affects the metric by which we sort the heap // regions. if (G1ScrubRemSets) { @@ -1676,6 +1690,10 @@ void ConcurrentMark::cleanup() { g1h->set_par_threads(n_workers); g1h->workers()->run_task(&g1_par_scrub_rs_task); g1h->set_par_threads(0); + + assert(g1h->check_heap_region_claim_values( + HeapRegion::ScrubRemSetClaimValue), + "sanity check"); } else { g1_par_scrub_rs_task.work(0); } diff --git a/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp b/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp index 7e61e4f49..43565be9b 100644 --- a/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp +++ b/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp @@ -1715,39 +1715,130 @@ G1CollectedHeap::heap_region_iterate_from(int idx, HeapRegionClosure* cl) { HeapRegion* G1CollectedHeap::region_at(size_t idx) { return _hrs->at(idx); } -const int OverpartitionFactor = 4; void G1CollectedHeap::heap_region_par_iterate_chunked(HeapRegionClosure* cl, int worker, jint claim_value) { - // We break up the heap regions into blocks of size ParallelGCThreads (to - // decrease iteration costs). - const size_t nregions = n_regions(); - const size_t n_thrds = (ParallelGCThreads > 0 ? ParallelGCThreads : 1); - const size_t partitions = n_thrds * OverpartitionFactor; - const size_t BlkSize = MAX2(nregions/partitions, (size_t)1); - const size_t n_blocks = (nregions + BlkSize - 1)/BlkSize; - assert(ParallelGCThreads > 0 || worker == 0, "Precondition"); - const int init_idx = (int) (n_blocks/n_thrds * worker); - for (size_t blk = 0; blk < n_blocks; blk++) { - size_t idx = init_idx + blk; - if (idx >= n_blocks) idx = idx - n_blocks; - size_t reg_idx = idx * BlkSize; - assert(reg_idx < nregions, "Because we rounded blk up."); - HeapRegion* r = region_at(reg_idx); + const size_t regions = n_regions(); + const size_t worker_num = (ParallelGCThreads > 0 ? ParallelGCThreads : 1); + // try to spread out the starting points of the workers + const size_t start_index = regions / worker_num * (size_t) worker; + + // each worker will actually look at all regions + for (size_t count = 0; count < regions; ++count) { + const size_t index = (start_index + count) % regions; + assert(0 <= index && index < regions, "sanity"); + HeapRegion* r = region_at(index); + // we'll ignore "continues humongous" regions (we'll process them + // when we come across their corresponding "start humongous" + // region) and regions already claimed + if (r->claim_value() == claim_value || r->continuesHumongous()) { + continue; + } + // OK, try to claim it if (r->claimHeapRegion(claim_value)) { - for (size_t j = 0; j < BlkSize; j++) { - size_t reg_idx2 = reg_idx + j; - if (reg_idx2 == nregions) break; - HeapRegion* r2 = region_at(reg_idx2); - if (j > 0) r2->set_claim_value(claim_value); - bool res = cl->doHeapRegion(r2); - guarantee(!res, "Should not abort."); + // success! + assert(!r->continuesHumongous(), "sanity"); + if (r->startsHumongous()) { + // If the region is "starts humongous" we'll iterate over its + // "continues humongous" first; in fact we'll do them + // first. The order is important. In on case, calling the + // closure on the "starts humongous" region might de-allocate + // and clear all its "continues humongous" regions and, as a + // result, we might end up processing them twice. So, we'll do + // them first (notice: most closures will ignore them anyway) and + // then we'll do the "starts humongous" region. + for (size_t ch_index = index + 1; ch_index < regions; ++ch_index) { + HeapRegion* chr = region_at(ch_index); + + // if the region has already been claimed or it's not + // "continues humongous" we're done + if (chr->claim_value() == claim_value || + !chr->continuesHumongous()) { + break; + } + + // Noone should have claimed it directly. We can given + // that we claimed its "starts humongous" region. + assert(chr->claim_value() != claim_value, "sanity"); + assert(chr->humongous_start_region() == r, "sanity"); + + if (chr->claimHeapRegion(claim_value)) { + // we should always be able to claim it; noone else should + // be trying to claim this region + + bool res2 = cl->doHeapRegion(chr); + assert(!res2, "Should not abort"); + + // Right now, this holds (i.e., no closure that actually + // does something with "continues humongous" regions + // clears them). We might have to weaken it in the future, + // but let's leave these two asserts here for extra safety. + assert(chr->continuesHumongous(), "should still be the case"); + assert(chr->humongous_start_region() == r, "sanity"); + } else { + guarantee(false, "we should not reach here"); + } + } } + + assert(!r->continuesHumongous(), "sanity"); + bool res = cl->doHeapRegion(r); + assert(!res, "Should not abort"); } } } +#ifdef ASSERT +// This checks whether all regions in the heap have the correct claim +// value. I also piggy-backed on this a check to ensure that the +// humongous_start_region() information on "continues humongous" +// regions is correct. + +class CheckClaimValuesClosure : public HeapRegionClosure { +private: + jint _claim_value; + size_t _failures; + HeapRegion* _sh_region; +public: + CheckClaimValuesClosure(jint claim_value) : + _claim_value(claim_value), _failures(0), _sh_region(NULL) { } + bool doHeapRegion(HeapRegion* r) { + if (r->claim_value() != _claim_value) { + gclog_or_tty->print_cr("Region ["PTR_FORMAT","PTR_FORMAT"), " + "claim value = %d, should be %d", + r->bottom(), r->end(), r->claim_value(), + _claim_value); + ++_failures; + } + if (!r->isHumongous()) { + _sh_region = NULL; + } else if (r->startsHumongous()) { + _sh_region = r; + } else if (r->continuesHumongous()) { + if (r->humongous_start_region() != _sh_region) { + gclog_or_tty->print_cr("Region ["PTR_FORMAT","PTR_FORMAT"), " + "HS = "PTR_FORMAT", should be "PTR_FORMAT, + r->bottom(), r->end(), + r->humongous_start_region(), + _sh_region); + ++_failures; + } + } + return false; + } + size_t failures() { + return _failures; + } +}; + +bool G1CollectedHeap::check_heap_region_claim_values(jint claim_value) { + CheckClaimValuesClosure cl(claim_value); + heap_region_iterate(&cl); + return cl.failures() == 0; +} +#endif // ASSERT + void G1CollectedHeap::collection_set_iterate(HeapRegionClosure* cl) { HeapRegion* r = g1_policy()->collection_set(); while (r != NULL) { diff --git a/src/share/vm/gc_implementation/g1/g1CollectedHeap.hpp b/src/share/vm/gc_implementation/g1/g1CollectedHeap.hpp index b47a983d9..3cb980e20 100644 --- a/src/share/vm/gc_implementation/g1/g1CollectedHeap.hpp +++ b/src/share/vm/gc_implementation/g1/g1CollectedHeap.hpp @@ -873,7 +873,6 @@ public: HeapRegion* region_at(size_t idx); - // Divide the heap region sequence into "chunks" of some size (the number // of regions divided by the number of parallel threads times some // overpartition factor, currently 4). Assumes that this will be called @@ -891,6 +890,10 @@ public: int worker, jint claim_value); +#ifdef ASSERT + bool check_heap_region_claim_values(jint claim_value); +#endif // ASSERT + // Iterate over the regions (if any) in the current collection set. void collection_set_iterate(HeapRegionClosure* blk); diff --git a/src/share/vm/gc_implementation/g1/g1CollectorPolicy.cpp b/src/share/vm/gc_implementation/g1/g1CollectorPolicy.cpp index accea783c..97e697c80 100644 --- a/src/share/vm/gc_implementation/g1/g1CollectorPolicy.cpp +++ b/src/share/vm/gc_implementation/g1/g1CollectorPolicy.cpp @@ -2897,7 +2897,8 @@ public: void work(int i) { ParKnownGarbageHRClosure parKnownGarbageCl(_hrSorted, _chunk_size, i); // Back to zero for the claim value. - _g1->heap_region_par_iterate_chunked(&parKnownGarbageCl, i, 0); + _g1->heap_region_par_iterate_chunked(&parKnownGarbageCl, i, + HeapRegion::InitialClaimValue); jint regions_added = parKnownGarbageCl.marked_regions_added(); _hrSorted->incNumMarkedHeapRegions(regions_added); if (G1PrintParCleanupStats) { @@ -2933,6 +2934,9 @@ record_concurrent_mark_cleanup_end(size_t freed_bytes, ParKnownGarbageTask parKnownGarbageTask(_collectionSetChooser, (int) ChunkSize); _g1->workers()->run_task(&parKnownGarbageTask); + + assert(_g1->check_heap_region_claim_values(HeapRegion::InitialClaimValue), + "sanity check"); } else { KnownGarbageClosure knownGarbagecl(_collectionSetChooser); _g1->heap_region_iterate(&knownGarbagecl); diff --git a/src/share/vm/gc_implementation/g1/heapRegion.cpp b/src/share/vm/gc_implementation/g1/heapRegion.cpp index 824adbf87..7bdde6af0 100644 --- a/src/share/vm/gc_implementation/g1/heapRegion.cpp +++ b/src/share/vm/gc_implementation/g1/heapRegion.cpp @@ -263,8 +263,7 @@ HeapRegion::new_dcto_closure(OopClosure* cl, } void HeapRegion::hr_clear(bool par, bool clear_space) { - _humongous = false; - _humongous_start = false; + _humongous_type = NotHumongous; _humongous_start_region = NULL; _in_collection_set = false; _is_gc_alloc_region = false; @@ -284,7 +283,7 @@ void HeapRegion::hr_clear(bool par, bool clear_space) { // If this is parallel, this will be done later. HeapRegionRemSet* hrrs = rem_set(); if (hrrs != NULL) hrrs->clear(); - _claimed = 0; + _claimed = InitialClaimValue; } zero_marked_bytes(); set_sort_index(-1); @@ -305,7 +304,7 @@ void HeapRegion::calc_gc_efficiency() { // void HeapRegion::set_startsHumongous() { - _humongous_start = true; _humongous = true; + _humongous_type = StartsHumongous; _humongous_start_region = this; assert(end() == _orig_end, "Should be normal before alloc."); } @@ -368,11 +367,11 @@ HeapRegion(G1BlockOffsetSharedArray* sharedOffsetArray, : G1OffsetTableContigSpace(sharedOffsetArray, mr, is_zeroed), _next_fk(HeapRegionDCTOC::NoFilterKind), _hrs_index(-1), - _humongous(false), _humongous_start(false), _humongous_start_region(NULL), + _humongous_type(NotHumongous), _humongous_start_region(NULL), _in_collection_set(false), _is_gc_alloc_region(false), _is_on_free_list(false), _is_on_unclean_list(false), _next_in_special_set(NULL), _orig_end(NULL), - _claimed(0), _evacuation_failed(false), + _claimed(InitialClaimValue), _evacuation_failed(false), _prev_marked_bytes(0), _next_marked_bytes(0), _sort_index(-1), _popularity(NotPopular), _young_type(NotYoung), _next_young_region(NULL), @@ -426,7 +425,7 @@ CompactibleSpace* HeapRegion::next_compaction_space() const { void HeapRegion::set_continuesHumongous(HeapRegion* start) { // The order is important here. start->add_continuingHumongousRegion(this); - _humongous = true; _humongous_start = false; + _humongous_type = ContinuesHumongous; _humongous_start_region = start; } diff --git a/src/share/vm/gc_implementation/g1/heapRegion.hpp b/src/share/vm/gc_implementation/g1/heapRegion.hpp index 17b093b8e..2413893dd 100644 --- a/src/share/vm/gc_implementation/g1/heapRegion.hpp +++ b/src/share/vm/gc_implementation/g1/heapRegion.hpp @@ -168,6 +168,12 @@ class HeapRegion: public G1OffsetTableContigSpace { friend class VMStructs; private: + enum HumongousType { + NotHumongous = 0, + StartsHumongous, + ContinuesHumongous + }; + // The next filter kind that should be used for a "new_dcto_cl" call with // the "traditional" signature. HeapRegionDCTOC::FilterKind _next_fk; @@ -188,8 +194,7 @@ class HeapRegion: public G1OffsetTableContigSpace { // sequence, otherwise -1. int _hrs_index; - bool _humongous; // starts or continues a humongous object - bool _humongous_start; // starts a humongous object + HumongousType _humongous_type; // For a humongous region, region in which it starts. HeapRegion* _humongous_start_region; // For the start region of a humongous sequence, it's original end(). @@ -308,6 +313,13 @@ class HeapRegion: public G1OffsetTableContigSpace { MaxAge = 2, NoOfAges = MaxAge+1 }; + enum ClaimValues { + InitialClaimValue = 0, + FinalCountClaimValue = 1, + NoteEndClaimValue = 2, + ScrubRemSetClaimValue = 3 + }; + // Concurrent refinement requires contiguous heap regions (in which TLABs // might be allocated) to be zero-filled. Each region therefore has a // zero-fill-state. @@ -355,9 +367,9 @@ class HeapRegion: public G1OffsetTableContigSpace { _prev_marked_bytes = _next_marked_bytes = 0; } - bool isHumongous() const { return _humongous; } - bool startsHumongous() const { return _humongous_start; } - bool continuesHumongous() const { return _humongous && ! _humongous_start; } + bool isHumongous() const { return _humongous_type != NotHumongous; } + bool startsHumongous() const { return _humongous_type == StartsHumongous; } + bool continuesHumongous() const { return _humongous_type == ContinuesHumongous; } // For a humongous region, region in which it starts. HeapRegion* humongous_start_region() const { return _humongous_start_region; -- GitLab