Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8336299: Improve GCLocker stall diagnostics #20277

Closed
wants to merge 6 commits into from
+43 −16
Closed

8336299: Improve GCLocker stall diagnostics #20277

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0a245f8
8336299: Improve GCLocker stall diagnostics
neethu-prasad Jul 16, 2024
b218518
Use newer Ticks/Tickspan API and switch to class with scope to hide
neethu-prasad Jul 30, 2024
c6b66ce
Add missing imports and remove unused ones
neethu-prasad Jul 30, 2024
c53dc9c
Address formating issue and code clean up feedback
neethu-prasad Aug 1, 2024
77fd9d5
address code style feedback
neethu-prasad Aug 7, 2024
6ca6f29
address feedback regarding logger potentially getting instantiated mu…
neethu-prasad Aug 12, 2024
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
31 changes: 14 additions & 17 deletions src/hotspot/share/gc/shared/gcLocker.cpp
View file
Original file line number Diff line number Diff line change
@@ -41,25 +41,23 @@ unsigned int GCLocker::_total_collections = 0;

// GCLockerTimingDebugLogger tracks specific timing information for GC lock waits.
class GCLockerTimingDebugLogger : public StackObj {
private:
const char* _log_message;
Ticks _start;

public:
GCLockerTimingDebugLogger(const char* log_message) :
_log_message(log_message) {
assert(_log_message != nullptr, "GC locker debug message must be set.");
_start = Ticks::now();
}
const char* _log_message;
Ticks _start;

public:
GCLockerTimingDebugLogger(const char* log_message) : _log_message(log_message) {
assert(_log_message != nullptr, "GC locker debug message must be set.");
_start = Ticks::now();
}

~GCLockerTimingDebugLogger() {
~GCLockerTimingDebugLogger() {
Log(gc, jni) log;
if (log.is_debug()) {
ResourceMark rm; // JavaThread::name() allocates to convert to UTF8
const Tickspan elapsed_time = Ticks::now() - _start;
Copy link
Member

Choose a reason for hiding this comment

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

Why is this outside the if logger-enabled check?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please move within the if.

Log(gc, jni) log;
if (log.is_debug()) {
ResourceMark rm; // JavaThread::name() allocates to convert to UTF8
log.debug("%s Resumed after " UINT64_FORMAT "ms. Thread \"%s\".", _log_message, elapsed_time.milliseconds(), Thread::current()->name());
}
log.debug("%s Resumed after " UINT64_FORMAT "ms. Thread \"%s\".", _log_message, elapsed_time.milliseconds(), Thread::current()->name());
Copy link
Member

Choose a reason for hiding this comment

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

Maybe UINT64_FORMAT can be replaced by %zu?

Copy link
Contributor

@tschatzl tschatzl Aug 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Tickspan::milliseconds() returns an uint64_t which is not size_t; the z length modifier is only for size_t and would be wrong to use here on 32 bit systems.

}
}
};

#ifdef ASSERT
@@ -155,7 +153,6 @@ void GCLocker::jni_lock(JavaThread* thread) {
if (needs_gc()) {
log_debug_jni("Blocking thread as there is a pending GC request");
}

while (needs_gc()) {
GCLockerTimingDebugLogger logger("Thread blocked to enter critical region.");
// There's at least one thread that has not left the critical region (CR)