On Thu, 23 Apr 2026 19:45:09 GMT, Patricio Chilano Mateo
<[email protected]> wrote:
>> 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.
>
> 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.
Addressed.
> 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.
Fixed.
> 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.
Okay, brought down to 200 repeats.
> 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.
Changed to `2 * timeout`.
> 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.
Changed as suggested.
> 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.
Yes, indeed. Removed this one and the one in the target thread.
> 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.
Addressed.
> 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));
Addressed.
> 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).
Done.
> 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.
Yes, looks like a leftover from some previous attempt, fixed now.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3136302981
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3136302756
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3136300377
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3136302526
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3136302351
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3136302129
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3136301927
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3136300996
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3136300773
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3136300552
