On Tue, 27 May 2025 22:48:15 GMT, Alex Menkov <amen...@openjdk.org> wrote:
>> This is first (hotspot) part of the update for
>> `HotSpotDiagnosticMXBean.dumpThreads` and `jcmd Thread.dump_to_file` to
>> include lock information in thread dumps (JDK-8356870).
>> The update has been split into parts to simplify reviewing.
>> The fix contains an implementation of `jdk.internal.vm.ThreadSnapshot` class
>> to gather required information about a thread.
>> Second (dependent) part includes changes in
>> `HotSpotDiagnosticMXBean.dumpThreads`/`jcmd Thread.dump_to_file`, spec
>> updates and tests for the functionality.
>>
>> Testing: new `HotSpotDiagnosticMXBean.dumpThreads`/`jcmd
>> Thread.dump_to_file` functionality was tested in loom repo;
>> sanity tier1 (this fix only)
>
> Alex Menkov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> move to ThreadService
Some initial drive-by comments ...
src/hotspot/share/classfile/javaClasses.cpp line 1875:
> 1873:
> 1874: oop java_lang_Thread::park_blocker(oop java_thread) {
> 1875: return java_thread->obj_field_acquire(_park_blocker_offset);
Where is the releasing store that pairs with this load-acquire?
src/hotspot/share/prims/jvmtiThreadState.hpp line 80:
> 78: // Virtual Thread Mount State Transition (VTMS transition) mechanism
> 79: //
> 80: class JvmtiVTMSTransitionDisabler : public AnyObj {
Why this change?
src/hotspot/share/services/threadService.cpp line 1170:
> 1168: };
> 1169:
> 1170: Handle _java_thread;
Suggestion:
Handle h_java_thread;
The use of the `h_` prefix for a variable that is a handle is not uncommon and
makes it clearer it is a handle, especially when applying the `()` operator.
src/hotspot/share/services/threadService.cpp line 1189:
> 1187: _thread_status(), _name(nullptr),
> 1188: _locks(nullptr), _blocker(), _blocker_owner(nullptr) { }
> 1189: virtual ~GetThreadSnapshotClosure() {
Suggestion:
_locks(nullptr), _blocker(), _blocker_owner(nullptr) {
}
virtual ~GetThreadSnapshotClosure() {
src/hotspot/share/services/threadService.cpp line 1203:
> 1201: }
> 1202:
> 1203: bool read_reset_retry() {
What does the `read` mean in the name?
src/hotspot/share/services/threadService.cpp line 1206:
> 1204: bool ret = _retry_handshake;
> 1205: // If we re-execute the handshake this method need to return false
> 1206: // when the handshake cannot be performed. (E.g. thread terminating)
Unclear what the comment means. The "retry" logic needs to be clearly explained
somewhere.
src/hotspot/share/services/threadService.cpp line 1262:
> 1260: // The first stage of async deflation does not affect any
> field
> 1261: // used by this comparison so the ObjectMonitor* is usable
> here.
> 1262: if (mark.has_monitor()) {
Does this depend on locking mode and non-compact-headers?
src/hotspot/share/services/threadService.cpp line 1434:
> 1432: static Handle allocate(InstanceKlass* klass, TRAPS) {
> 1433: init(klass, CHECK_NH);
> 1434: return klass->allocate_instance_handle(CHECK_NH);
Suggestion:
return klass->allocate_instance_handle(CHECK_NH);
Suggestion:
Handle h_k = klass->allocate_instance_handle(CHECK_NH);
return h_k;
You can't use `CHECK` on a `return` statement.
src/hotspot/share/services/threadService.cpp line 1528:
> 1526: // StackTrace
> 1527: InstanceKlass* ste_klass = vmClasses::StackTraceElement_klass();
> 1528: assert(ste_klass != nullptr, "must be loaded in 1.4+");
Suggestion:
assert(ste_klass != nullptr, "must be loaded");
The 1.4+ is not relevant these days
src/hotspot/share/services/threadService.cpp line 1531:
> 1529: if (ste_klass->should_be_initialized()) {
> 1530: ste_klass->initialize(CHECK_NULL);
> 1531: }
Is this actually necessary? Doesn't this klass always get initialized as part
of VM initialization?
src/hotspot/share/services/threadService.cpp line 1560:
> 1558: // call static StackTraceElement[]
> StackTraceElement.of(StackTraceElement[] stackTrace)
> 1559: // to properly initialize STE.
> 1560: {
Why the additional block scope?
-------------
PR Review: https://git.openjdk.org/jdk/pull/25425#pullrequestreview-2873829619
PR Review Comment: https://git.openjdk.org/jdk/pull/25425#discussion_r2111086253
PR Review Comment: https://git.openjdk.org/jdk/pull/25425#discussion_r2111089461
PR Review Comment: https://git.openjdk.org/jdk/pull/25425#discussion_r2111102216
PR Review Comment: https://git.openjdk.org/jdk/pull/25425#discussion_r2111108369
PR Review Comment: https://git.openjdk.org/jdk/pull/25425#discussion_r2111110893
PR Review Comment: https://git.openjdk.org/jdk/pull/25425#discussion_r2111112004
PR Review Comment: https://git.openjdk.org/jdk/pull/25425#discussion_r2111195897
PR Review Comment: https://git.openjdk.org/jdk/pull/25425#discussion_r2111202668
PR Review Comment: https://git.openjdk.org/jdk/pull/25425#discussion_r2111206814
PR Review Comment: https://git.openjdk.org/jdk/pull/25425#discussion_r2111207890
PR Review Comment: https://git.openjdk.org/jdk/pull/25425#discussion_r2111210020
