On Tue, 21 Apr 2026 05:45:35 GMT, David Holmes <[email protected]> wrote:
>> Anton Artemov has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8382088: Fixed join timeout value.
>
> src/hotspot/share/runtime/objectMonitor.cpp line 1905:
>
>> 1903: // a thread will have TS_ENTER state and posting the event may hit
>> a suspension point.
>> 1904: // From a debugging perspective, it is more important to have no
>> missing events.
>> 1905: if (interruptible && node.TState != ObjectWaiter::TS_ENTER) {
>
> You need to update the comment at line 1911 as well as there is no re-check
> now. In fact you could just delete the comment on line 1911.
Removed the comment as suggested.
> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java
> line 35:
>
>> 33: * @test
>> 34: * @bug 8382088
>> 35: * @summary Suspend thread right after it timed out in wait()
>
> Suggestion:
>
> * @summary Suspend thread right after it timed out in wait()
Addressed.
> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java
> line 119:
>
>> 117: try {
>> 118: JVMTIUtils.suspendThread(targetThread);
>> 119: is_suspended = true;
>
> You can avoid the extra flag by doing:
>
> JVMTIUtils.suspendThread(targetThread);
> try {
> ...
> finally {
> JVMTIUtils.resumeThread(targetThread);
> }
>
> which is a more idiomatic try/finally block.
Noted, removed the flag.
> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java
> line 174:
>
>> 172: }
>> 173:
>> 174: assertTrue(!targetThread.isAlive(), "target thread is still
>> alive");
>
> You could do a timeless join and let the jtreg timeout deal with unexpected
> cases. That avoids the need to for the isAlive check.
Sure, let jtreg handle this case.
> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java
> line 184:
>
>> 182:
>> 183: // Determined this purely experimentally
>> 184: if ((double)failureCounter / (double)usefulRun >
>> acceptableFailureRate) {
>
> These types of failure thresholds always tend to come back to bite us when
> tests run on a variety of machine and different loads, and different VM flags.
Should we maybe fail in case of a single occurrence then? I updated the test
accordingly. Does not fail after a few thousand runs on all supported platforms.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3117091437
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3117091848
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3117092422
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3117093851
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3117093426
