Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions src/hotspot/share/gc/g1/g1CollectedHeap.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,11 @@ HeapWord* G1CollectedHeap::attempt_allocation_slow(uint node_index, size_t word_
log_warning(gc, alloc)("%s: Retried allocation %u times for %zu words",
Thread::current()->name(), try_count, word_size);
}

if (is_shutting_down()) {
stall_for_vm_shutdown();
return nullptr;
}
}

ShouldNotReachHere();
Expand Down Expand Up @@ -703,6 +708,11 @@ HeapWord* G1CollectedHeap::attempt_allocation_humongous(size_t word_size) {
log_warning(gc, alloc)("%s: Retried allocation %u times for %zu words",
Thread::current()->name(), try_count, word_size);
}

if (is_shutting_down()) {
stall_for_vm_shutdown();
return nullptr;
}
}

ShouldNotReachHere();
Expand Down Expand Up @@ -1504,10 +1514,6 @@ jint G1CollectedHeap::initialize() {
return JNI_OK;
}

bool G1CollectedHeap::concurrent_mark_is_terminating() const {
return _cm_thread->should_terminate();
}

void G1CollectedHeap::stop() {
// Stop all concurrent threads. We do this to make sure these threads
// do not continue to execute and access resources (e.g. logging)
Expand Down Expand Up @@ -1811,7 +1817,7 @@ bool G1CollectedHeap::try_collect_concurrently(GCCause::Cause cause,

// If VMOp skipped initiating concurrent marking cycle because
// we're terminating, then we're done.
if (op.terminating()) {
if (is_shutting_down()) {
LOG_COLLECT_CONCURRENTLY(cause, "skipped: terminating");
return false;
}
Expand Down
3 changes: 0 additions & 3 deletions src/hotspot/share/gc/g1/g1CollectedHeap.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -899,9 +899,6 @@ class G1CollectedHeap : public CollectedHeap {
// specified by the policy object.
jint initialize() override;

// Returns whether concurrent mark threads (and the VM) are about to terminate.
bool concurrent_mark_is_terminating() const;

void safepoint_synchronize_begin() override;
void safepoint_synchronize_end() override;

Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/gc/g1/g1ConcurrentMark.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1883,7 +1883,7 @@ bool G1ConcurrentMark::concurrent_cycle_abort() {
// nothing, but this situation should be extremely rare (a full gc after shutdown
// has been signalled is already rare), and this work should be negligible compared
// to actual full gc work.
if (!cm_thread()->in_progress() && !_g1h->concurrent_mark_is_terminating()) {
if (!cm_thread()->in_progress() && !_g1h->is_shutting_down()) {
return false;
}

Expand Down
3 changes: 2 additions & 1 deletion src/hotspot/share/gc/g1/g1Policy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -662,6 +662,7 @@ bool G1Policy::should_retain_evac_failed_region(uint index) const {
}

void G1Policy::record_pause_start_time() {
assert(!_g1h->is_shutting_down(), "Invariant!");
Ticks now = Ticks::now();
_cur_pause_start_sec = now.seconds();

Expand Down Expand Up @@ -1246,7 +1247,7 @@ void G1Policy::decide_on_concurrent_start_pause() {

// We should not be starting a concurrent start pause if the concurrent mark
// thread is terminating.
if (_g1h->concurrent_mark_is_terminating()) {
if (_g1h->is_shutting_down()) {
return;
}

Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/gc/g1/g1RemSet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1105,7 +1105,7 @@ class G1MergeHeapRootsTask : public WorkerTask {
// There might actually have been scheduled multiple collections, but at that point we do
// not care that much about performance and just do the work multiple times if needed.
return (_g1h->collector_state()->clear_bitmap_in_progress() ||
_g1h->concurrent_mark_is_terminating()) &&
_g1h->is_shutting_down()) &&
hr->is_old();
}

Expand Down
21 changes: 9 additions & 12 deletions src/hotspot/share/gc/g1/g1VMOperations.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@ VM_G1TryInitiateConcMark::VM_G1TryInitiateConcMark(uint gc_count_before,
_mark_in_progress(false),
_cycle_already_in_progress(false),
_whitebox_attached(false),
_terminating(false),
_gc_succeeded(false)
{}

Expand All @@ -83,19 +82,10 @@ void VM_G1TryInitiateConcMark::doit() {

GCCauseSetter x(g1h, _gc_cause);

// Record for handling by caller.
_terminating = g1h->concurrent_mark_is_terminating();

_mark_in_progress = g1h->collector_state()->mark_in_progress();
_cycle_already_in_progress = g1h->concurrent_mark()->cm_thread()->in_progress();

if (_terminating && GCCause::is_user_requested_gc(_gc_cause)) {
// When terminating, the request to initiate a concurrent cycle will be
// ignored by do_collection_pause_at_safepoint; instead it will just do
// a young-only or mixed GC (depending on phase). For a user request
// there's no point in even doing that much, so done. For some non-user
// requests the alternative GC might still be needed.
} else if (!g1h->policy()->force_concurrent_start_if_outside_cycle(_gc_cause)) {
if (!g1h->policy()->force_concurrent_start_if_outside_cycle(_gc_cause)) {
// Failure to force the next GC pause to be a concurrent start indicates
// there is already a concurrent marking cycle in progress. Flags to indicate
// that were already set, so return immediately.
Expand All @@ -119,7 +109,6 @@ VM_G1CollectForAllocation::VM_G1CollectForAllocation(size_t word_size,

void VM_G1CollectForAllocation::doit() {
G1CollectedHeap* g1h = G1CollectedHeap::heap();

GCCauseSetter x(g1h, _gc_cause);
// Try a partial collection of some kind.
g1h->do_collection_pause_at_safepoint();
Expand Down Expand Up @@ -156,6 +145,14 @@ void VM_G1PauseConcurrent::doit() {

bool VM_G1PauseConcurrent::doit_prologue() {
Heap_lock->lock();
G1CollectedHeap* g1h = G1CollectedHeap::heap();
if (g1h->is_shutting_down()) {
Heap_lock->unlock();
// JVM shutdown has started. This ensures that any further operations will be properly aborted
// and will not interfere with the shutdown process.
g1h->concurrent_mark()->abort_marking_threads();
return false;
}
return true;
}

Expand Down
2 changes: 0 additions & 2 deletions src/hotspot/share/gc/g1/g1VMOperations.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ class VM_G1TryInitiateConcMark : public VM_GC_Collect_Operation {
bool _mark_in_progress;
bool _cycle_already_in_progress;
bool _whitebox_attached;
bool _terminating;
// The concurrent start pause may be cancelled for some reasons. Keep track of
// this.
bool _gc_succeeded;
Expand All @@ -63,7 +62,6 @@ class VM_G1TryInitiateConcMark : public VM_GC_Collect_Operation {
bool mark_in_progress() const { return _mark_in_progress; }
bool cycle_already_in_progress() const { return _cycle_already_in_progress; }
bool whitebox_attached() const { return _whitebox_attached; }
bool terminating() const { return _terminating; }
bool gc_succeeded() const { return _gc_succeeded && VM_GC_Operation::gc_succeeded(); }
};

Expand Down
5 changes: 5 additions & 0 deletions src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,11 @@ HeapWord* ParallelScavengeHeap::mem_allocate_work(size_t size, bool is_tlab) {
assert(is_in_or_null(op.result()), "result not in heap");
return op.result();
}

if (is_shutting_down()) {
stall_for_vm_shutdown();
return nullptr;
}
}

// Was the gc-overhead reached inside the safepoint? If so, this mutator
Expand Down
5 changes: 5 additions & 0 deletions src/hotspot/share/gc/serial/serialHeap.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,11 @@ HeapWord* SerialHeap::mem_allocate_work(size_t size, bool is_tlab) {
break;
}

if (is_shutting_down()) {
stall_for_vm_shutdown();
return nullptr;
}

// Give a warning if we seem to be looping forever.
if ((QueuedAllocationWarningCount > 0) &&
(try_count % QueuedAllocationWarningCount == 0)) {
Expand Down
23 changes: 23 additions & 0 deletions src/hotspot/share/gc/shared/collectedHeap.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,12 @@ MetaWord* CollectedHeap::satisfy_failed_metadata_allocation(ClassLoaderData* loa
if (op.gc_succeeded()) {
return op.result();
}

if (is_shutting_down()) {
stall_for_vm_shutdown();
return nullptr;
}

loop_count++;
if ((QueuedAllocationWarningCount > 0) &&
(loop_count % QueuedAllocationWarningCount == 0)) {
Expand Down Expand Up @@ -603,6 +609,23 @@ void CollectedHeap::post_initialize() {
initialize_serviceability();
}

bool CollectedHeap::is_shutting_down() const {
return Universe::is_shutting_down();
}

void CollectedHeap::stall_for_vm_shutdown() {
assert(is_shutting_down(), "Precondition");
// Stall the thread (2 seconds) instead of an indefinite wait to avoid deadlock
// if the VM shutdown triggers a GC.
// The 2-seconds sleep is:
// - long enough to keep daemon threads stalled, while the shutdown
// sequence completes in the common case.
// - short enough to avoid excessive stall time if the shutdown itself
// triggers a GC.
JavaThread::current()->sleep(2 * MILLIUNITS);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer this to the Monitor::wait too. Thanks

log_warning(gc, alloc)("%s: Stall for VM-Shutdown timed out; allocation may fail with OOME", Thread::current()->name());
}

void CollectedHeap::before_exit() {
print_tracing_info();

Expand Down
7 changes: 7 additions & 0 deletions src/hotspot/share/gc/shared/collectedHeap.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,13 @@ class CollectedHeap : public CHeapObj<mtGC> {
// This is the correct place to place such initialization methods.
virtual void post_initialize();

bool is_shutting_down() const;

// If the VM is shutting down, we may have skipped VM_CollectForAllocation.
// In this case, stall the allocation request briefly in the hope that
// the VM shutdown completes before the allocation request returns.
void stall_for_vm_shutdown();

void before_exit();

// Stop and resume concurrent GC threads interfering with safepoint operations
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/gc/shared/gcVMOperations.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ bool VM_GC_Operation::doit_prologue() {
VM_Heap_Sync_Operation::doit_prologue();

// Check invocations
if (skip_operation()) {
if (skip_operation() || Universe::is_shutting_down()) {
// skip collection
Heap_lock->unlock();
if (should_use_gclocker()) {
Expand Down
10 changes: 9 additions & 1 deletion src/hotspot/share/memory/universe.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ int Universe::_base_vtable_size = 0;
bool Universe::_bootstrapping = false;
bool Universe::_module_initialized = false;
bool Universe::_fully_initialized = false;
volatile bool Universe::_is_shutting_down = false;

OopStorage* Universe::_vm_weak = nullptr;
OopStorage* Universe::_vm_global = nullptr;
Expand Down Expand Up @@ -1344,7 +1345,14 @@ static void log_cpu_time() {
}

void Universe::before_exit() {
log_cpu_time();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you move log_cpu_time()? During the review of CPUTimeUsage refactor (#26621) we discussed this choice. Given that it still includes more than just GC I think it should stay in Universe. Also the PR title does not reflect that it would include a refactor of CPUTimeUsage.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main reason was to have the log_cpu_time and AtomicAccess::release_store(&_is_shutting_down, true) under same critical section. Otherwise, we have no guarantee that we don't continue GCs after log_cpu_time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had put log_cpu_time right before calling stop(). The stop() is the method that terminates GC threads, so no synchronization should be needed if I'm not mistaken.

Please correct me if you think I got it wrong here.

Nevertheless, any user of gc_threads_do might still iterate over terminated GC workers thread. Could we consider adding a check or assert in that method?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can have GCs between log_cpu_time and stop(). This reduces chances of that happening if we have log_cpu_time under same lock as setting _is_shutting_down.

Nevertheless, any user of gc_threads_do might still iterate over terminated GC workers thread. Could we consider adding a check or assert in that method?

Yes, we can have the assert in gc_threads_do, I thought this was going to be done as a follow up.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved _is_shutting_down to Universe, which allowed me to restore this log_cpu_time. I also added the assert before reading os::thread_cpu_time

{
// Acquire the Heap_lock to synchronize with VM_Heap_Sync_Operations,
// which may depend on the value of _is_shutting_down flag.
MutexLocker hl(Heap_lock);
log_cpu_time();
AtomicAccess::release_store(&_is_shutting_down, true);
}

heap()->before_exit();

// Print GC/heap related information.
Expand Down
5 changes: 5 additions & 0 deletions src/hotspot/share/memory/universe.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,9 @@ class Universe: AllStatic {
static bool _module_initialized; // true after call_initPhase2 called
static bool _fully_initialized; // true after universe_init and initialize_vtables called

// Shutdown
static volatile bool _is_shutting_down;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If _is_shutting_down is in Universe, should ZGC/ZAbort hook into this too? It could be confusing that ZAbort reports that we are shutting down but the universe field does not report this. I would expect them both to report true.


// the array of preallocated errors with backtraces
static objArrayOop preallocated_out_of_memory_errors();

Expand Down Expand Up @@ -324,6 +327,8 @@ class Universe: AllStatic {
static bool is_module_initialized() { return _module_initialized; }
static bool is_fully_initialized() { return _fully_initialized; }

static bool is_shutting_down() { return AtomicAccess::load_acquire(&_is_shutting_down); }

static bool on_page_boundary(void* addr);
static bool should_fill_in_stack_trace(Handle throwable);
static void check_alignment(uintx size, uintx alignment, const char* name);
Expand Down
1 change: 1 addition & 0 deletions src/hotspot/share/services/cpuTimeUsage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include "memory/universe.hpp"
#include "runtime/globals.hpp"
#include "runtime/os.hpp"
#include "runtime/osThread.hpp"
#include "runtime/perfData.hpp"
#include "runtime/vmThread.hpp"
#include "services/cpuTimeUsage.hpp"
Expand Down