RFR: 8357650: ThreadSnapshot to take snapshot of thread for thread dumps [v2]

[David Holmes]

On Tue, 27 May 2025 22:48:15 GMT, Alex Menkov <amenkov at 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