On Thu, 23 Apr 2026 09:42:01 GMT, Anton Artemov <[email protected]> wrote:
>> Hi, please consider the following changes: >> >> this is a fix for a bug in the `ObjectMonitor::wait()` method introduced by >> [8366659](https://bugs.openjdk.org/browse/JDK-8366659). This bug leads to >> increased failure rate of `ownedMonitorsAndFrames003`, though the test was >> very seldomly failing before >> [8366659](https://bugs.openjdk.org/browse/JDK-8366659). The fix just brings >> the failure rate back to where it used to be. >> >> In short, [8366659](https://bugs.openjdk.org/browse/JDK-8366659) introduced >> a code path where there was no handling of a suspension request until the >> thread goes back to Java, which caused test failures slightly more often. >> >> If `should_post_monitor_waited()` returns false, then a thread, which >> timed-out waiting and got a suspension request, does not have any suspension >> check. >> >> Now the extra condition is removed and a thread in the `TS_RUN` state will >> get its suspension request handled at the right place. >> >> A separate test which triggers the condition is added. >> >> Tested in tiers 1 - 7. >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Anton Artemov has updated the pull request incrementally with two additional > commits since the last revision: > > - 8382088: Addressed reviewer's comments. > - 8382088: Addressed reviewer's comments. Thanks, a few extra comments below. src/hotspot/share/runtime/objectMonitor.cpp line 1902: > 1900: > 1901: // The re-enter path will not have a suspension point if there is no > 1902: // contention, so if a thread is not on the _entry_list, we need to > check for suspension now. Suggestion: `When the thread is not on the _entry_list, the re-enter path does not have a suspension point if there is no contention, so we need ...`, otherwise it reads as both re-enter paths don’t check for suspension, but that is not the case for the `TS_ENTER` case. src/hotspot/share/runtime/objectMonitor.cpp line 1906: > 1904: if (interruptible && node.TState != ObjectWaiter::TS_ENTER) { > 1905: > 1906: // Process suspend requests now if any, before posting the event. This comment can be removed now since we already mentioned it above. test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java line 45: > 43: static final Object lock = new Object(); > 44: static long timeout = 10; // milliseconds > 45: static int maxRetries = 2000; I would also tune this down to a few hundred iterations. With the current value the test takes about 40s to run (~20ms per iteration due to the sleep) which seems too much. We are already reproducing the desired scenario in pretty much every loop iteration. test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java line 46: > 44: static long timeout = 10; // milliseconds > 45: static int maxRetries = 2000; > 46: static long waitForTimedWaitingMills = 5000; This should be reduced to something like `20`. The target waits for `10ms` so there is no point in waiting much more than that. Or better, we could use `timeout` times some factor and remove this variable. test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java line 71: > 69: System.out.println("Timeout = " + timeout + " msc."); > 70: > 71: Phaser phaser = new Phaser(1); You can initialize this to `2` and remove the `register()` call in the constructor. test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java line 76: > 74: > 75: targetThread.start(); > 76: phaser.arriveAndAwaitAdvance(); We already have a sync point in the first loop iteration so this is not needed. test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java line 87: > 85: long deadlineNanos = System.nanoTime() + > waitForTimedWaitingMills * 1_000_000L; > 86: if (!waitUntilTimedWaiting(targetThread, deadlineNanos)) { > 87: throw new RuntimeException("Timed out waiting to reach > TIMED_WAITING state at retry " + n); This should be a `continue`. The target might have already finished waiting for this iteration. test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java line 157: > 155: grabbedMonitor = > 156: > Arrays.stream(threadInfo[0].getLockedMonitors()).anyMatch(m -> > m.getIdentityHashCode() == System.identityHashCode(lock)); > 157: return grabbedMonitor; Suggestion: return Arrays.stream(threadInfo[0].getLockedMonitors()).anyMatch(m -> m.getIdentityHashCode() == System.identityHashCode(lock)); test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java line 172: > 170: public void run() { > 171: synchronized (lock) { > 172: phaser.arriveAndAwaitAdvance(); This can be removed (matching the suggestion above). test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java line 175: > 173: boolean done = false; > 174: int retryNumber = 0; > 175: while (!done) { You can have the same loop as the main thread (`for (int n = 0; n < maxRetries; ++n)`), and remove the extra `retryNumber` and `done` logic. ------------- Changes requested by pchilanomate (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/30709#pullrequestreview-4165410958 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3133377250 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3133380520 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3133473855 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3133420549 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3133422807 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3133425088 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3133427292 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3133432532 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3133438318 PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3133439709
