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
--- a/hotspot/src/share/vm/gc_implementation/g1/concurrentMark.cpp Fri Dec 16 02:14:27 2011 -0500
+++ b/hotspot/src/share/vm/gc_implementation/g1/concurrentMark.cpp Fri Dec 16 11:40:00 2011 -0800
@@ -1117,12 +1117,9 @@
// 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 @@
// 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 @@
// 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 @@
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 @@
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 @@
HeapRegion::FinalCountClaimValue),
"sanity check");
} else {
+ n_workers = 1;
g1_par_count_task.work(0);
}
@@ -1851,7 +1854,6 @@
(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 @@
}
}
- 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 @@
// 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 @@
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 @@
}
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);
--- a/hotspot/src/share/vm/gc_implementation/g1/concurrentMark.hpp Fri Dec 16 02:14:27 2011 -0500
+++ b/hotspot/src/share/vm/gc_implementation/g1/concurrentMark.hpp Fri Dec 16 11:40:00 2011 -0800
@@ -718,7 +718,7 @@
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
--- a/hotspot/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp Fri Dec 16 02:14:27 2011 -0500
+++ b/hotspot/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp Fri Dec 16 11:40:00 2011 -0800
@@ -3787,8 +3787,9 @@
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 @@
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 @@
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 @@
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 @@
// 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 @@
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) {