From c4973f8513cc18201911edf0ab529e098e437471 Mon Sep 17 00:00:00 2001 From: johnc Date: Fri, 16 Dec 2011 11:40:00 -0800 Subject: [PATCH] 7120038: G1: ParallelGCThreads==0 is broken Summary: Running G1 with ParallelGCThreads==0 results in various crashes and asserts. Most of these are caused by unguarded references to the worker threads array or an incorrect number of active workers. Reviewed-by: jmasa, tonyp --- .../gc_implementation/g1/concurrentMark.cpp | 53 ++++++++++--------- .../gc_implementation/g1/concurrentMark.hpp | 2 +- .../gc_implementation/g1/g1CollectedHeap.cpp | 21 +++++--- 3 files changed, 41 insertions(+), 35 deletions(-) diff --git a/src/share/vm/gc_implementation/g1/concurrentMark.cpp b/src/share/vm/gc_implementation/g1/concurrentMark.cpp index 785b87d48..e208929a2 100644 --- a/src/share/vm/gc_implementation/g1/concurrentMark.cpp +++ b/src/share/vm/gc_implementation/g1/concurrentMark.cpp @@ -1117,12 +1117,9 @@ public: // Calculates the number of active workers for a concurrent // phase. -int ConcurrentMark::calc_parallel_marking_threads() { - - size_t n_conc_workers; - if (!G1CollectedHeap::use_parallel_gc_threads()) { - n_conc_workers = 1; - } else { +size_t ConcurrentMark::calc_parallel_marking_threads() { + if (G1CollectedHeap::use_parallel_gc_threads()) { + size_t n_conc_workers = 0; if (!UseDynamicNumberOfGCThreads || (!FLAG_IS_DEFAULT(ConcGCThreads) && !ForceDynamicNumberOfGCThreads)) { @@ -1137,9 +1134,13 @@ int ConcurrentMark::calc_parallel_marking_threads() { // Don't scale down "n_conc_workers" by scale_parallel_threads() because // that scaling has already gone into "_max_parallel_marking_threads". } + assert(n_conc_workers > 0, "Always need at least 1"); + return n_conc_workers; } - assert(n_conc_workers > 0, "Always need at least 1"); - return (int) MAX2(n_conc_workers, (size_t) 1); + // If we are not running with any parallel GC threads we will not + // have spawned any marking threads either. Hence the number of + // concurrent workers should be 0. + return 0; } void ConcurrentMark::markFromRoots() { @@ -1151,24 +1152,24 @@ void ConcurrentMark::markFromRoots() { // stop-the-world GC happens even as we mark in this generation. _restart_for_overflow = false; - - // Parallel task terminator is set in "set_phase()". force_overflow_conc()->init(); // _g1h has _n_par_threads - _parallel_marking_threads = calc_parallel_marking_threads(); assert(parallel_marking_threads() <= max_parallel_marking_threads(), "Maximum number of marking threads exceeded"); - _parallel_workers->set_active_workers((int)_parallel_marking_threads); - // Don't set _n_par_threads because it affects MT in proceess_strong_roots() - // and the decisions on that MT processing is made elsewhere. - assert( _parallel_workers->active_workers() > 0, "Should have been set"); - set_phase(_parallel_workers->active_workers(), true /* concurrent */); + size_t active_workers = MAX2((size_t) 1, parallel_marking_threads()); + + // Parallel task terminator is set in "set_phase()" + set_phase(active_workers, true /* concurrent */); CMConcurrentMarkingTask markingTask(this, cmThread()); if (parallel_marking_threads() > 0) { + _parallel_workers->set_active_workers((int)active_workers); + // Don't set _n_par_threads because it affects MT in proceess_strong_roots() + // and the decisions on that MT processing is made elsewhere. + assert(_parallel_workers->active_workers() > 0, "Should have been set"); _parallel_workers->run_task(&markingTask); } else { markingTask.work(0); @@ -1765,8 +1766,7 @@ void ConcurrentMark::cleanup() { HeapRegionRemSet::reset_for_cleanup_tasks(); - g1h->set_par_threads(); - size_t n_workers = g1h->n_par_threads(); + size_t n_workers; // Do counting once more with the world stopped for good measure. G1ParFinalCountTask g1_par_count_task(g1h, nextMarkBitMap(), @@ -1776,8 +1776,10 @@ void ConcurrentMark::cleanup() { HeapRegion::InitialClaimValue), "sanity check"); + g1h->set_par_threads(); + n_workers = g1h->n_par_threads(); assert(g1h->n_par_threads() == (int) n_workers, - "Should not have been reset"); + "Should not have been reset"); g1h->workers()->run_task(&g1_par_count_task); // Done with the parallel phase so reset to 0. g1h->set_par_threads(0); @@ -1786,6 +1788,7 @@ void ConcurrentMark::cleanup() { HeapRegion::FinalCountClaimValue), "sanity check"); } else { + n_workers = 1; g1_par_count_task.work(0); } @@ -1851,7 +1854,6 @@ void ConcurrentMark::cleanup() { (note_end_end - note_end_start)*1000.0); } - // call below, since it affects the metric by which we sort the heap // regions. if (G1ScrubRemSets) { @@ -2329,9 +2331,9 @@ public: } } - CMRemarkTask(ConcurrentMark* cm) : + CMRemarkTask(ConcurrentMark* cm, int active_workers) : AbstractGangTask("Par Remark"), _cm(cm) { - _cm->terminator()->reset_for_reuse(cm->_g1h->workers()->active_workers()); + _cm->terminator()->reset_for_reuse(active_workers); } }; @@ -2357,7 +2359,7 @@ void ConcurrentMark::checkpointRootsFinalWork() { // constructor and pass values of the active workers // through the gang in the task. - CMRemarkTask remarkTask(this); + CMRemarkTask remarkTask(this, active_workers); g1h->set_par_threads(active_workers); g1h->workers()->run_task(&remarkTask); g1h->set_par_threads(0); @@ -2367,7 +2369,7 @@ void ConcurrentMark::checkpointRootsFinalWork() { int active_workers = 1; set_phase(active_workers, false /* concurrent */); - CMRemarkTask remarkTask(this); + CMRemarkTask remarkTask(this, active_workers); // We will start all available threads, even if we decide that the // active_workers will be fewer. The extra ones will just bail out // immediately. @@ -3123,13 +3125,12 @@ void ConcurrentMark::complete_marking_in_collection_set() { } double start = os::elapsedTime(); - int n_workers = g1h->workers()->total_workers(); - G1ParCompleteMarkInCSetTask complete_mark_task(g1h, this); assert(g1h->check_cset_heap_region_claim_values(HeapRegion::InitialClaimValue), "sanity"); if (G1CollectedHeap::use_parallel_gc_threads()) { + int n_workers = g1h->workers()->active_workers(); g1h->set_par_threads(n_workers); g1h->workers()->run_task(&complete_mark_task); g1h->set_par_threads(0); diff --git a/src/share/vm/gc_implementation/g1/concurrentMark.hpp b/src/share/vm/gc_implementation/g1/concurrentMark.hpp index f85e3779f..6383227d9 100644 --- a/src/share/vm/gc_implementation/g1/concurrentMark.hpp +++ b/src/share/vm/gc_implementation/g1/concurrentMark.hpp @@ -718,7 +718,7 @@ public: size_t scale_parallel_threads(size_t n_par_threads); // Calculates the number of GC threads to be used in a concurrent phase. - int calc_parallel_marking_threads(); + size_t calc_parallel_marking_threads(); // The following three are interaction between CM and // G1CollectedHeap diff --git a/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp b/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp index 114a2c792..03280b448 100644 --- a/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp +++ b/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp @@ -3787,8 +3787,9 @@ G1CollectedHeap::do_collection_pause_at_safepoint(double target_pause_time_ms) { double end_time_sec = os::elapsedTime(); double pause_time_ms = (end_time_sec - start_time_sec) * MILLIUNITS; g1_policy()->record_pause_time_ms(pause_time_ms); - int active_gc_threads = workers()->active_workers(); - g1_policy()->record_collection_pause_end(active_gc_threads); + int active_workers = (G1CollectedHeap::use_parallel_gc_threads() ? + workers()->active_workers() : 1); + g1_policy()->record_collection_pause_end(active_workers); MemoryService::track_memory_usage(); @@ -5312,8 +5313,10 @@ void G1CollectedHeap::process_discovered_references() { int active_workers = (G1CollectedHeap::use_parallel_gc_threads() ? workers()->active_workers() : 1); - assert(active_workers == workers()->active_workers(), - "Need to reset active_workers"); + assert(!G1CollectedHeap::use_parallel_gc_threads() || + active_workers == workers()->active_workers(), + "Need to reset active_workers"); + set_par_threads(active_workers); G1ParPreserveCMReferentsTask keep_cm_referents(this, active_workers, _task_queues); @@ -5451,13 +5454,13 @@ void G1CollectedHeap::evacuate_collection_set() { assert(UseDynamicNumberOfGCThreads || n_workers == workers()->total_workers(), "If not dynamic should be using all the workers"); + workers()->set_active_workers(n_workers); set_par_threads(n_workers); } else { assert(n_par_threads() == 0, "Should be the original non-parallel value"); n_workers = 1; } - workers()->set_active_workers(n_workers); G1ParTask g1_par_task(this, _task_queues); @@ -5479,6 +5482,7 @@ void G1CollectedHeap::evacuate_collection_set() { workers()->run_task(&g1_par_task); } else { StrongRootsScope srs(this); + g1_par_task.set_for_termination(n_workers); g1_par_task.work(0); } @@ -5727,8 +5731,8 @@ void G1CollectedHeap::cleanUpCardTable() { // Iterate over the dirty cards region list. G1ParCleanupCTTask cleanup_task(ct_bs, this); - if (ParallelGCThreads > 0) { - set_par_threads(workers()->total_workers()); + if (G1CollectedHeap::use_parallel_gc_threads()) { + set_par_threads(); workers()->run_task(&cleanup_task); set_par_threads(0); } else { @@ -6136,8 +6140,9 @@ HeapRegion* MutatorAllocRegion::allocate_new_region(size_t word_size, void G1CollectedHeap::set_par_threads() { // Don't change the number of workers. Use the value previously set // in the workgroup. + assert(G1CollectedHeap::use_parallel_gc_threads(), "shouldn't be here otherwise"); int n_workers = workers()->active_workers(); - assert(UseDynamicNumberOfGCThreads || + assert(UseDynamicNumberOfGCThreads || n_workers == workers()->total_workers(), "Otherwise should be using the total number of workers"); if (n_workers == 0) { -- GitLab