On Thu, 13 Nov 2025 19:16:57 GMT, Patricio Chilano Mateo
<[email protected]> wrote:
>> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java
>> line 388:
>>
>>> 386: try {
>>> 387: Thread.sleep(1000);
>>> 388: } catch(Exception e) {}
>>
>> Why is this 1 second delay needed?
>
> We need to give time for the resumer thread to block on the `threadLock`
> before we release it. We could reduce the amount of sleep time though.
Agreed. I pretty much said the same thing about the resumer thread in the new
doWork2 transaction
diagram (and the new doWork3 transaction diagram).
>> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java
>> line 428:
>>
>>> 426: // launch the waiter thread
>>> 427: synchronized (barrierLaunch) {
>>> 428: waiter = new
>>> SuspendWithObjectMonitorWaitWorker("waiter", 1);
>>
>> This change to `wait` for `1` instead of `0` requires this comment to be
>> updated from:
>>
>> // TS_READY_TO_NOTIFY is set after the main thread has
>> // entered threadLock so a spurious wakeup can't get the
>> // waiter thread out of this threadLock.wait(0) call:
>>
>> to:
>>
>> // TS_READY_TO_NOTIFY is set after the main thread has
>> // entered threadLock so a spurious wakeup can't get the
>> // waiter thread out of this threadLock.wait(0) call in
>> // doWork1 or doWork2. doWork3 passes a one so that the
>> // wait() can terminate early and block on reentry.
>>
>>
>> I'm having trouble seeing why this third test case is necessary. We do a
>> short `wait(1)`
>> in this test case instead of a `wait(0)` so we terminate the `wait(1)` with
>> a timeout instead
>> of a `notify()` from the main thread.
>>
>> In all worker test cases, the main thread grabs the threadsLock when the
>> "waiter" thread
>> calls `wait()`, the main thread does a `notify()`, the main thread waits
>> until the worker
>> thread contends on threadsLock and finally the main thread suspends the
>> worker thread.
>> The only thing that I see that the `wait(1)` brings to the party is that the
>> worker3 thread
>> might get to re-entry block on threadsLock via a timeout instead of a notify.
>>
>> What am I missing here?
>
> The `notify()` call doesn’t unpark the waiter thread, it just moves it from
> the `_wait_set` to the `_entry_list`. The wait(timeout) is there to allow it
> to run again so that it is suspended in `reenter_internal()`. But currently
> the timings will not exercise this case well. I added some comments to fix it.
Please see the notes in the new doWork3 transaction diagram. I think I've
covered this case.
>> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java
>> line 454:
>>
>>> 452: try {
>>> 453: Thread.sleep(1000);
>>> 454: } catch(Exception e) {}
>>
>> Why is this 1 second delay needed?
>
> This sleep is not needed.
I have a comment about this in the new doWork3 transaction diagram.
>> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java
>> line 509:
>>
>>> 507: try {
>>> 508: Thread.sleep(1000);
>>> 509: } catch(Exception e) {}
>>
>> Why is this 1 second delay needed?
>
> Same here, we need to give time for the resumer thread to block on the
> threadLock before we release it.
Agreed. I pretty much said the same thing about the resumer thread in the new
doWork3 transaction
diagram (and the new doWork2 transaction diagram).
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2524862590
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2524866785
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2524870502
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2524875174
