On Mon, 19 Jan 2026 00:27:39 GMT, David Holmes <[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 44 commits: >> >> - 8366659: Fixed year in the copyright. >> - Merge remote-tracking branch 'origin/master' into >> JDK-8366659-OM-wait-suspend-deadlock >> - 8366659: Removed ClearSuccOnSuspend >> - 8366659: Fixed liveness problem. >> - Merge remote-tracking branch 'origin/master' into >> JDK-8366659-OM-wait-suspend-deadlock >> - 8366659: Fixed build problem. >> - 8366659: Fixed build issue. >> - 8366659: Changed the way how notify_internal works if JVMTI monitor >> waited event allowed. >> - Merge remote-tracking branch 'origin/master' into >> JDK-8366659-OM-wait-suspend-deadlock >> - 8366659: Fixed semi-broken tests >> - ... and 34 more: https://git.openjdk.org/jdk/compare/a01283a5...21b83214 > > src/hotspot/share/runtime/objectMonitor.cpp line 1907: > >> 1905: // That is, we fail toward safety. >> 1906: >> 1907: was_notified = true; > > This isn't quite right. If we would reach line 1882 then we were not > notified, so we cannot unconditionally set this to true here. I think you > need to initialize to true at line 1868 and then set to false at line 1882 > (deleting the current comment). Thanks for spotting this! Addressed. > src/hotspot/share/runtime/objectMonitor.cpp line 1917: > >> 1915: } >> 1916: >> 1917: // The thread is now either on off-list (TS_RUN), > > Suggestion: > > // The thread is now either off-list (TS_RUN), Addressed. > src/hotspot/share/runtime/objectMonitor.cpp line 1932: > >> 1930: // (Don't cache naked oops over safepoints, of course). >> 1931: >> 1932: // post monitor waited event. Note that this is past-tense, we are >> done waiting. > > Suggestion: > > // Post monitor waited event. Note that this is past-tense, we are done > waiting. > > You need to expand the comment to explain the significance of checking for > TS_ENTER. Addressed. > src/hotspot/share/runtime/objectMonitor.cpp line 1973: > >> 1971: // and set the OM as pending. >> 1972: } >> 1973: // Done waiting, post the corresponding event > > What event? I expected to see `JvmtiExport::post_monitor_waited` here. ?? I think I overlooked this comment, it is related to one of the previous iterations, where the `JvmtiExport::post_monitor_waited` event was posted further down in the code. Removed. > src/hotspot/share/runtime/objectMonitor.cpp line 2053: > >> 2051: // Adding to _entry_list uses Atomic::cmpxchg() which already >> provides >> 2052: // a fence that prevents reordering of the stores. >> 2053: if (!JvmtiExport::should_post_monitor_waited()) { > > You need to explain why this is now conditional. Added a comment there. > src/hotspot/share/runtime/objectMonitor.cpp line 2063: > >> 2061: if (!JvmtiExport::should_post_monitor_waited()) { >> 2062: add_to_entry_list(current, iterator); >> 2063: } else { > > You need to explain what each of these paths is doing. At this point I'm lost. Added extended comments for both cases. > src/hotspot/share/runtime/objectMonitor.cpp line 2069: > >> 2067: >> 2068: oop vthread = nullptr; >> 2069: ParkEvent* Trigger; > > I assume this is copied from elsewhere but we should use proper style rules > in the new code i.e. local variables don't start with a capital. Thanks. Also > "trigger" doesn't really convet anythng meaningful to me - `evt` for event > seems fine. Right, it was copied from some other file. Fixed. > src/hotspot/share/runtime/objectMonitor.cpp line 2277: > >> 2275: node->_at_reenter = true; >> 2276: >> 2277: if (state == ObjectWaiter::TS_RUN) { > > I'm finding the changes to `was_notified` versus use of explicit state checks > very confusing. I keep asking myself which state means I was notified and > which does not. Yes, it is all because unparking inside of `notify_internal()`, which is needed to fix the problem, leads to threads which have been actually notified (aka woken up in `notify_internal()'), but are not on the `entry_list`. We may need to do a follow-up PR with some refactoring, maybe binary state is not sufficient. > test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait1.java > line 71: > >> 69: public int doWork1(int timeMax, PrintStream out) { >> 70: SuspendWithObjectMonitorWaitWorker waiter; // waiter thread >> 71: SuspendWithObjectMonitorWaitWorker resumer; // resumer thread > > Suggestion: > > SuspendWithObjectMonitorWaitWorker waiter; // waiter thread > SuspendWithObjectMonitorWaitWorker resumer; // resumer thread Fixed in all test cases. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2704328783 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2704329246 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2704329583 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2704330248 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2704330507 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2704330740 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2704330960 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2704331400 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2704331637
