RFR: 8346792: serviceability/jvmti/vthread/GetThreadState/GetThreadState.java testObjectWaitMillis failed [v9]

[David Holmes]

On Wed, 22 Jan 2025 17:28:49 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> Below is the root cause of the bug described by Patricio:
>> The root of the issue is similar to [JDK-8345543](https://bugs.openjdk.org/browse/JDK-8345543), but instead of `JavaThreadBlockedOnMonitorEnterState`, the interaction here is with its equivalent for Object.wait, `JavaThreadInObjectWaitState`.
>> 
>> We change the `Thread.State` of the carrier to `TIMED_WAITING` before we try preempting the `vthread`. So we can see the `Thread.State` of the vthread as `TIMED_WAITING` before even calling `try_preempt()` (when the state is `RUNNING` we return the `Thread.State` of the carrier). Then when we check for `JVMTI_THREAD_STATE_BLOCKED_ON_MONITOR_ENTER`, we caught the `vthread` in `afterYield()` still with the transitional state of `TIMED_WAITING`, which is mapped to `Thread.State.RUNNABLE`.
>> 
>> The fix is to delay the state change to `JavaThreadStatus::IN_OBJECT_WAIT*` the point after block with the `try_preempt()` call. The fix is not elegant and hacky but I do not see a better solution. The ugliness comes from the fact that the `ObjectMonitor::wait()` is called from two places: `ObjectSynchronizer::wait()` and `ObjectSynchronizer::waitUninterruptibly()`. We do not need the `JavaThreadInObjectWaitState` in the second cases, so the object is defined in the `JVM_MonitorWait()` but not in the `ObjectMonitor::wait()`.
>> Maybe, deep refactoring the`JavaThreadInObjectWaitState` base class `JavaThreadStatusChanger` could be a better solution but it is going to be much more intrusive.
>> 
>> Testing:
>>  - Checked the failed test does not fail with the fix: `serviceability/jvmti/vthread/GetThreadState/GetThreadState.java`
>>  - Ran mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   review: cleanup in jvm.cpp

I find it exceedingly difficult to follow the ObjectMonitor code now that we have so much special handling for virtual threads. I think a future RFE may be needed to split things out so we take the regular or virtual paths at a higher level and simplify the code flow even if we have some duplication.

The current changes do seem to preserve the regular thread behaviour as requested. But I'm concerned that regular threads and virtual threads perform JVMTI callbacks in different states - something seems inherently flawed in the design if we need to do that. And the solution is not to change what regular threads do to match virtual threads IMO!

Suggested change below.

Thanks

src/hotspot/share/runtime/objectMonitor.cpp line 1653:

> 1651: }
> 1652: 
> 1653: void ObjectMonitor::post_monitor_wait(JavaThread *current, jlong millis) {

Not sure this is worthwhile factoring out for a few reasons:
1. the actual code is only 2 lines
2.  the comment only makes sense when a path that will park follows this code - which is now only one of the three cases
3.  It is confusing to see this method and the static `post_monitor_wait_event`

I think we can just inline the code where needed and place the comment after L1725 (as I don't think it even applies to the virtual thread case prior to that).

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

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/23126#pullrequestreview-2568889056
PR Review Comment: https://git.openjdk.org/jdk/pull/23126#discussion_r1926379416