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

[Alex Menkov]

On Wed, 28 May 2025 07:10:57 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Alex Menkov has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   move to ThreadService
>
> 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.

The logic comes from `java_lang_Thread::async_get_stack_trace()` (implementation of `java.lang.Thread.getStackTrace()`) I don't see why the handshake needs to be executed on JavaThread.

> 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?

AFAIU locking mode should not be lightweight, but in the case mark.has_monitor() returns false, so no need to additional checks

> 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?

I suppose it's not necessary, just for safety

> 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?

not needed anymore

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/25425#discussion_r2112569181
PR Review Comment: https://git.openjdk.org/jdk/pull/25425#discussion_r2112597974
PR Review Comment: https://git.openjdk.org/jdk/pull/25425#discussion_r2112616322
PR Review Comment: https://git.openjdk.org/jdk/pull/25425#discussion_r2112616667