Skip to content

Commit

Permalink
8329488: Move OopStorage code from safepoint cleanup and remove safep…
Browse files Browse the repository at this point in the history 
…oint cleanup code

Reviewed-by: kbarrett, eosterlund
  • Loading branch information
@coleenp
coleenp committed Apr 12, 2024
1 parent 77a217d commit 3e9c381
Show file tree
Hide file tree
Showing 14 changed files with 54 additions and 281 deletions.
82 changes: 32 additions & 50 deletions src/hotspot/share/gc/shared/oopStorage.cpp
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2018, 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2018, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -410,7 +410,7 @@ OopStorage::Block::block_for_ptr(const OopStorage* owner, const oop* ptr) {
// allocations until some entries in it are released.
//
// release() is performed lock-free. (Note: This means it can't notify the
// service thread of pending cleanup work. It must be lock-free because
// ServiceThread of pending cleanup work. It must be lock-free because
// it is called in all kinds of contexts where even quite low ranked locks
// may be held.) release() first looks up the block for
// the entry, using address alignment to find the enclosing block (thereby
Expand Down Expand Up @@ -705,7 +705,7 @@ void OopStorage::Block::release_entries(uintx releasing, OopStorage* owner) {
// Only request cleanup for to-empty transitions, not for from-full.
// There isn't any rush to process from-full transitions. Allocation
// will reduce deferrals before allocating new blocks, so may process
// some. And the service thread will drain the entire deferred list
// some. And the ServiceThread will drain the entire deferred list
// if there are any pending to-empty transitions.
if (releasing == old_allocated) {
owner->record_needs_cleanup();
Expand Down Expand Up @@ -880,67 +880,51 @@ bool OopStorage::should_report_num_dead() const {
}

// Managing service thread notifications.
//
// We don't want cleanup work to linger indefinitely, but we also don't want
// to run the service thread too often. We're also very limited in what we
// can do in a release operation, where cleanup work is created.
//

// When a release operation changes a block's state to empty, it records the
// need for cleanup in both the associated storage object and in the global
// request state. A safepoint cleanup task notifies the service thread when
// request state. The ServiceThread checks at timed intervals if
// there may be cleanup work for any storage object, based on the global
// request state. But that notification is deferred if the service thread
// has run recently, and we also avoid duplicate notifications. The service
// thread updates the timestamp and resets the state flags on every iteration.
// request state. We don't want to run empty block cleanup too often in the
// face of frequent explicit ServiceThread wakeups, hence the defer period.

// Global cleanup request state.
static volatile bool needs_cleanup_requested = false;

// Flag for avoiding duplicate notifications.
static bool needs_cleanup_triggered = false;
// Time after which a cleanup is permitted.
static jlong cleanup_permit_time = 0;

// Time after which a notification can be made.
static jlong cleanup_trigger_permit_time = 0;

// Minimum time since last service thread check before notification is
// permitted. The value of 500ms was an arbitrary choice; frequent, but not
// too frequent.
const jlong cleanup_trigger_defer_period = 500 * NANOSECS_PER_MILLISEC;

void OopStorage::trigger_cleanup_if_needed() {
MonitorLocker ml(Service_lock, Monitor::_no_safepoint_check_flag);
if (Atomic::load(&needs_cleanup_requested) &&
!needs_cleanup_triggered &&
(os::javaTimeNanos() > cleanup_trigger_permit_time)) {
needs_cleanup_triggered = true;
ml.notify_all();
}
}
// Minimum time between ServiceThread cleanups.
// The value of 500ms was an arbitrary choice; frequent, but not too frequent.
const jlong cleanup_defer_period = 500 * NANOSECS_PER_MILLISEC;

bool OopStorage::has_cleanup_work_and_reset() {
assert_lock_strong(Service_lock);
cleanup_trigger_permit_time =
os::javaTimeNanos() + cleanup_trigger_defer_period;
needs_cleanup_triggered = false;
// Set the request flag false and return its old value.
// Needs to be atomic to avoid dropping a concurrent request.
// Can't use Atomic::xchg, which may not support bool.
return Atomic::cmpxchg(&needs_cleanup_requested, true, false);

if (Atomic::load_acquire(&needs_cleanup_requested) &&
os::javaTimeNanos() > cleanup_permit_time) {
cleanup_permit_time =
os::javaTimeNanos() + cleanup_defer_period;
// Set the request flag false and return its old value.
Atomic::release_store(&needs_cleanup_requested, false);
return true;
} else {
return false;
}
}

// Record that cleanup is needed, without notifying the Service thread.
// Used by release(), where we can't lock even Service_lock.
// Record that cleanup is needed, without notifying the Service thread, because
// we can't lock the Service_lock. Used by release().
void OopStorage::record_needs_cleanup() {
// Set local flag first, else service thread could wake up and miss
// the request. This order may instead (rarely) unnecessarily notify.
// Set local flag first, else ServiceThread could wake up and miss
// the request.
Atomic::release_store(&_needs_cleanup, true);
Atomic::release_store_fence(&needs_cleanup_requested, true);
}

bool OopStorage::delete_empty_blocks() {
// Service thread might have oopstorage work, but not for this object.
// Check for deferred updates even though that's not a service thread
// trigger; since we're here, we might as well process them.
// ServiceThread might have oopstorage work, but not for this object.
// But check for deferred updates, which might provide cleanup work.
if (!Atomic::load_acquire(&_needs_cleanup) &&
(Atomic::load_acquire(&_deferred_updates) == nullptr)) {
return false;
Expand Down Expand Up @@ -986,7 +970,7 @@ bool OopStorage::delete_empty_blocks() {
// Don't interfere with an active concurrent iteration.
// Instead, give up immediately. There is more work to do,
// but don't re-notify, to avoid useless spinning of the
// service thread. Instead, iteration completion notifies.
// ServiceThread. Instead, iteration completion notifies.
if (_concurrent_iteration_count > 0) return true;
_active_array->remove(block);
}
Expand All @@ -998,10 +982,8 @@ bool OopStorage::delete_empty_blocks() {
ThreadBlockInVM tbiv(JavaThread::current());
}
}
// Exceeded work limit or can't delete last block. This will
// cause the service thread to loop, giving other subtasks an
// opportunity to run too. There's no need for a notification,
// because we are part of the service thread (unless gtesting).
// Exceeded work limit or can't delete last block so still needs cleanup
// for the next time.
record_needs_cleanup();
return true;
}
Expand Down
13 changes: 1 addition & 12 deletions src/hotspot/share/jfr/metadata/metadata.xml
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<?xml version="1.0" encoding="utf-8"?>

<!--
Copyright (c) 2012, 2023, Oracle and/or its affiliates. All rights reserved.
Copyright (c) 2012, 2024, Oracle and/or its affiliates. All rights reserved.
DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -684,17 +684,6 @@
<Field type="int" name="iterations" label="Iterations" description="Number of state check iterations" />
</Event>

<Event name="SafepointCleanup" category="Java Virtual Machine, Runtime, Safepoint" label="Safepoint Cleanup" description="Safepointing begin running cleanup tasks"
thread="true">
<Field type="ulong" name="safepointId" label="Safepoint Identifier" relation="SafepointId" />
</Event>

<Event name="SafepointCleanupTask" category="Java Virtual Machine, Runtime, Safepoint" label="Safepoint Cleanup Task" description="Safepointing begin running cleanup tasks"
thread="true">
<Field type="ulong" name="safepointId" label="Safepoint Identifier" relation="SafepointId" />
<Field type="string" name="name" label="Task Name" description="The task name" />
</Event>

<Event name="SafepointEnd" category="Java Virtual Machine, Runtime, Safepoint" label="Safepoint End" description="Safepointing end" thread="true">
<Field type="ulong" name="safepointId" label="Safepoint Identifier" relation="SafepointId" />
</Event>
Expand Down
5 changes: 5 additions & 0 deletions src/hotspot/share/runtime/globals.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1282,6 +1282,11 @@ const int ObjectAlignmentInBytes = 8;
"(0 means none)") \
range(0, max_jint) \
\
product(intx, ServiceThreadCleanupInterval, 1000, DIAGNOSTIC, \
"Wake the ServiceThread to do periodic cleanup checks every so " \
"many milliseconds (0 means none)") \
range(0, max_jint) \
\
product(double, SafepointTimeoutDelay, 10000, \
"Delay in milliseconds for option SafepointTimeout; " \
"supports sub-millisecond resolution with fractional values.") \
Expand Down
109 changes: 4 additions & 105 deletions src/hotspot/share/runtime/safepoint.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,12 +80,6 @@ static void post_safepoint_begin_event(EventSafepointBegin& event,
}
}

static void post_safepoint_cleanup_event(EventSafepointCleanup& event, uint64_t safepoint_id) {
if (event.should_commit()) {
event.set_safepointId(safepoint_id);
event.commit();
}
}

static void post_safepoint_synchronize_event(EventSafepointStateSynchronization& event,
uint64_t safepoint_id,
Expand All @@ -101,16 +95,6 @@ static void post_safepoint_synchronize_event(EventSafepointStateSynchronization&
}
}

static void post_safepoint_cleanup_task_event(EventSafepointCleanupTask& event,
uint64_t safepoint_id,
const char* name) {
if (event.should_commit()) {
event.set_safepointId(safepoint_id);
event.set_name(name);
event.commit();
}
}

static void post_safepoint_end_event(EventSafepointEnd& event, uint64_t safepoint_id) {
if (event.should_commit()) {
event.set_safepointId(safepoint_id);
Expand Down Expand Up @@ -435,14 +419,7 @@ void SafepointSynchronize::begin() {

SafepointTracing::synchronized(nof_threads, initial_running, _nof_threads_hit_polling_page);

// We do the safepoint cleanup first since a GC related safepoint
// needs cleanup to be completed before running the GC op.
EventSafepointCleanup cleanup_event;
do_cleanup_tasks();
post_safepoint_cleanup_event(cleanup_event, _safepoint_id);

post_safepoint_begin_event(begin_event, _safepoint_id, nof_threads, _current_jni_active_count);
SafepointTracing::cleanup();
}

void SafepointSynchronize::disarm_safepoint() {
Expand Down Expand Up @@ -507,68 +484,6 @@ void SafepointSynchronize::end() {
post_safepoint_end_event(event, safepoint_id());
}

class ParallelCleanupTask : public WorkerTask {
private:
SubTasksDone _subtasks;

class Tracer {
private:
const char* _name;
EventSafepointCleanupTask _event;
TraceTime _timer;

public:
Tracer(const char* name) :
_name(name),
_event(),
_timer(name, TRACETIME_LOG(Info, safepoint, cleanup)) {}
~Tracer() {
post_safepoint_cleanup_task_event(_event, SafepointSynchronize::safepoint_id(), _name);
}
};

public:
ParallelCleanupTask() :
WorkerTask("Parallel Safepoint Cleanup"),
_subtasks(SafepointSynchronize::SAFEPOINT_CLEANUP_NUM_TASKS) {}

uint expected_num_workers() const {
uint workers = 0;

return MAX2<uint>(1, workers);
}

void work(uint worker_id) {
if (_subtasks.try_claim_task(SafepointSynchronize::SAFEPOINT_CLEANUP_REQUEST_OOPSTORAGE_CLEANUP)) {
// Don't bother reporting event or time for this very short operation.
// To have any utility we'd also want to report whether needed.
OopStorage::trigger_cleanup_if_needed();
}

_subtasks.all_tasks_claimed();
}
};

// Various cleaning tasks that should be done periodically at safepoints.
void SafepointSynchronize::do_cleanup_tasks() {

TraceTime timer("safepoint cleanup tasks", TRACETIME_LOG(Info, safepoint, cleanup));

CollectedHeap* heap = Universe::heap();
assert(heap != nullptr, "heap not initialized yet?");
ParallelCleanupTask cleanup;
WorkerThreads* cleanup_workers = heap->safepoint_workers();
const uint expected_num_workers = cleanup.expected_num_workers();
if (cleanup_workers != nullptr && expected_num_workers > 1) {
// Parallel cleanup using GC provided thread pool.
const uint num_workers = MIN2(expected_num_workers, cleanup_workers->active_workers());
cleanup_workers->run_task(&cleanup, num_workers);
} else {
// Serial cleanup using VMThread.
cleanup.work(0);
}
}

// Methods for determining if a JavaThread is safepoint safe.

// False means unsafe with undetermined state.
Expand Down Expand Up @@ -946,15 +861,13 @@ void ThreadSafepointState::handle_polling_page_exception() {

jlong SafepointTracing::_last_safepoint_begin_time_ns = 0;
jlong SafepointTracing::_last_safepoint_sync_time_ns = 0;
jlong SafepointTracing::_last_safepoint_cleanup_time_ns = 0;
jlong SafepointTracing::_last_safepoint_end_time_ns = 0;
jlong SafepointTracing::_last_app_time_ns = 0;
int SafepointTracing::_nof_threads = 0;
int SafepointTracing::_nof_running = 0;
int SafepointTracing::_page_trap = 0;
VM_Operation::VMOp_Type SafepointTracing::_current_type;
jlong SafepointTracing::_max_sync_time = 0;
jlong SafepointTracing::_max_cleanup_time = 0;
jlong SafepointTracing::_max_vmop_time = 0;
uint64_t SafepointTracing::_op_count[VM_Operation::VMOp_Terminating] = {0};

Expand All @@ -970,7 +883,7 @@ static void print_header(outputStream* st) {

st->print("VM Operation "
"[ threads: total initial_running ]"
"[ time: sync cleanup vmop total ]");
"[ time: sync vmop total ]");

st->print_cr(" page_trap_count");
}
Expand Down Expand Up @@ -999,11 +912,9 @@ void SafepointTracing::statistics_log() {
_nof_threads,
_nof_running);
ls.print("[ "
INT64_FORMAT_W(10) " " INT64_FORMAT_W(10) " "
INT64_FORMAT_W(10) " " INT64_FORMAT_W(10) " ]",
INT64_FORMAT_W(10) " " INT64_FORMAT_W(10) " " INT64_FORMAT_W(10) " ]",
(int64_t)(_last_safepoint_sync_time_ns - _last_safepoint_begin_time_ns),
(int64_t)(_last_safepoint_cleanup_time_ns - _last_safepoint_sync_time_ns),
(int64_t)(_last_safepoint_end_time_ns - _last_safepoint_cleanup_time_ns),
(int64_t)(_last_safepoint_end_time_ns - _last_safepoint_sync_time_ns),
(int64_t)(_last_safepoint_end_time_ns - _last_safepoint_begin_time_ns));

ls.print_cr(INT32_FORMAT_W(16), _page_trap);
Expand All @@ -1024,8 +935,6 @@ void SafepointTracing::statistics_exit_log() {

log_info(safepoint, stats)("Maximum sync time " INT64_FORMAT" ns",
(int64_t)(_max_sync_time));
log_info(safepoint, stats)("Maximum cleanup time " INT64_FORMAT" ns",
(int64_t)(_max_cleanup_time));
log_info(safepoint, stats)("Maximum vm operation time (except for Exit VM operation) "
INT64_FORMAT " ns",
(int64_t)(_max_vmop_time));
Expand All @@ -1038,7 +947,6 @@ void SafepointTracing::begin(VM_Operation::VMOp_Type type) {
// update the time stamp to begin recording safepoint time
_last_safepoint_begin_time_ns = os::javaTimeNanos();
_last_safepoint_sync_time_ns = 0;
_last_safepoint_cleanup_time_ns = 0;

_last_app_time_ns = _last_safepoint_begin_time_ns - _last_safepoint_end_time_ns;
_last_safepoint_end_time_ns = 0;
Expand All @@ -1054,19 +962,12 @@ void SafepointTracing::synchronized(int nof_threads, int nof_running, int traps)
RuntimeService::record_safepoint_synchronized(_last_safepoint_sync_time_ns - _last_safepoint_begin_time_ns);
}

void SafepointTracing::cleanup() {
_last_safepoint_cleanup_time_ns = os::javaTimeNanos();
}

void SafepointTracing::end() {
_last_safepoint_end_time_ns = os::javaTimeNanos();

if (_max_sync_time < (_last_safepoint_sync_time_ns - _last_safepoint_begin_time_ns)) {
_max_sync_time = _last_safepoint_sync_time_ns - _last_safepoint_begin_time_ns;
}
if (_max_cleanup_time < (_last_safepoint_cleanup_time_ns - _last_safepoint_sync_time_ns)) {
_max_cleanup_time = _last_safepoint_cleanup_time_ns - _last_safepoint_sync_time_ns;
}
if (_max_vmop_time < (_last_safepoint_end_time_ns - _last_safepoint_sync_time_ns)) {
_max_vmop_time = _last_safepoint_end_time_ns - _last_safepoint_sync_time_ns;
}
Expand All @@ -1078,14 +979,12 @@ void SafepointTracing::end() {
"Safepoint \"%s\", "
"Time since last: " JLONG_FORMAT " ns, "
"Reaching safepoint: " JLONG_FORMAT " ns, "
"Cleanup: " JLONG_FORMAT " ns, "
"At safepoint: " JLONG_FORMAT " ns, "
"Total: " JLONG_FORMAT " ns",
VM_Operation::name(_current_type),
_last_app_time_ns,
_last_safepoint_sync_time_ns - _last_safepoint_begin_time_ns,
_last_safepoint_cleanup_time_ns - _last_safepoint_sync_time_ns,
_last_safepoint_end_time_ns - _last_safepoint_cleanup_time_ns,
_last_safepoint_end_time_ns - _last_safepoint_sync_time_ns,
_last_safepoint_end_time_ns - _last_safepoint_begin_time_ns
);

Expand Down
Loading

1 comment on commit 3e9c381

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

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

Review

Issues

Please sign in to comment.