On Fri, 14 Nov 2025 18:07:27 GMT, Daniel D. Daugherty <[email protected]> wrote:
>> Anton Artemov has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 22 commits: >> >> - Merge remote-tracking branch 'origin/master' into >> JDK-8366659-OM-wait-suspend-deadlock >> - 8366659: Fixed the test. >> - Merge remote-tracking branch 'origin/master' into >> JDK-8366659-OM-wait-suspend-deadlock >> - 8366659: Fixed lines in tests. >> - Merge remote-tracking branch 'origin/master' into >> JDK-8366659-OM-wait-suspend-deadlock >> - 8366659: Added a comment to a boolean arg for enter() >> - Merge remote-tracking branch 'origin/master' into >> JDK-8366659-OM-wait-suspend-deadlock >> - Merge remote-tracking branch 'origin/master' into >> JDK-8366659-OM-wait-suspend-deadlock >> - 8366659: Fixed new lines. >> - Merge remote-tracking branch 'origin/master' into >> JDK-8366659-OM-wait-suspend-deadlock >> - ... and 12 more: https://git.openjdk.org/jdk/compare/ff851de8...2ac18b94 > > test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java > line 124: > >> 122: // launch resumer enter threadLock >> 123: // <launch returns> while !READY_TO_NOTIFY resumer running >> 124: // delay 1-second threadLock.wait(1) wait for notify > > We've deleted this sleep, so this line should change from: > > // delay 1-second threadLock.wait(1) wait for notify > > to: > > // : threadLock.wait(1) wait for notify Addressed. > test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java > line 128: > >> 126: // set READY_TO_NOTIFY : >> 127: // threadLock.notify wait finishes : >> 128: // : reenter blocks : > > We added a sleep here, so this line should change from: > > // : reenter blocks : > > to: > > // : delay 200ms reenter blocks : Addressed. > test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java > line 153: > >> 151: // thread allows the waiter thread to loop tightly here: >> 152: // while !READY_TO_NOTIFY >> 153: // threadLock.wait(1) > > Please replace this note with the following: > > // Note: sleep(200ms) here while holding the threadLock to allow the waiter > thread's > // timed wait to finish before we attempt to suspend the waiter thread. Addressed. > test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java > line 237: > >> 235: >> 236: public static void usage() { >> 237: System.err.println("Usage: " + AGENT_LIB + " >> [N][-p][time_max]"); > > The `N` argument is not optional so it should not be surrounded by `[` and > `]`. Addressed in the latest commit. > test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java > line 239: > >> 237: System.err.println("Usage: " + AGENT_LIB + " >> [N][-p][time_max]"); >> 238: System.err.println("where:"); >> 239: System.err.println(" N ::= test case"); > > s/case/case: 1 | 2 | 3/ Addressed in the latest commit. > test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java > line 470: > >> 468: } >> 469: try { >> 470: Thread.sleep(1000); > > Please add something like this above the `sleep`: > > // Delay for 1-second while holding the threadLock to force the > // resumer thread to block on entering the threadLock. Addressed in the latest commit. > test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java > line 552: > >> 550: // wait for the waiter thread to block >> 551: logDebug("before contended enter wait"); >> 552: int retCode = wait4ContendedEnter(waiter); > > Since we are waiting here for ContendedEnter by the waiter thread, do we > really > need the above `sleep(200)` or are we just being polite CPU resource wise? Maybe I do not fully understand the question, but isn't it what differentiates test case 2 from test case 3? > test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java > line 592: > >> 590: } >> 591: try { >> 592: Thread.sleep(1000); > > Please add something like this above the sleep: > > // Delay for 1-second while holding the threadLock to force the > // resumer thread to block on entering the threadLock. Added. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2534136973 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2534137344 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2534137633 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2534135509 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2534135744 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2534138176 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2534136258 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2534136725
