On Tue, 20 Jan 2026 09:56:09 GMT, Anton Artemov <[email protected]> wrote:
>> Hi, please consider the following changes: >> >> If suspension is allowed when a thread is re-entering an object monitor >> (OM), then a following liveness issues can happen in the >> `ObjectMonitor::wait()` method. >> >> The waiting thread is made to be a successor and is unparked. Upon a >> suspension request, the thread will suspend itself whilst clearing the >> successor. The OM will be left unlocked (not grabbed by any thread), while >> the other threads are parked until a thread grabs the OM and the exits it. >> The suspended thread is on the entry-list and can be selected as a successor >> again. None of other threads can be woken up to grab the OM until the >> suspended thread has been resumed and successfully releases the OM. >> >> This can happen in three places where the successor could be suspended: >> >> 1: >> https://github.com/openjdk/jdk/blob/6322aaba63b235cb6c73d23a932210af318404ec/src/hotspot/share/runtime/objectMonitor.cpp#L1897 >> >> 2: >> https://github.com/openjdk/jdk/blob/6322aaba63b235cb6c73d23a932210af318404ec/src/hotspot/share/runtime/objectMonitor.cpp#L1149 >> >> 3: >> https://github.com/openjdk/jdk/blob/6322aaba63b235cb6c73d23a932210af318404ec/src/hotspot/share/runtime/objectMonitor.cpp#L1951 >> >> The issues are addressed by not allowing suspension in case 1, and by >> handling the suspension request at a later stage, after the thread has >> grabbed the OM in `reenter_internal()` in case 2. In case of a suspension >> request, the thread exits the OM and enters it again once resumed. >> >> Case 3 is handled by not transferring a thread to the `entry_list` in >> `notify_internal()` in case the corresponding JVMTI event is allowed. >> Instead, a tread is unparked and let run. Since it is not on the >> `entry_list`, it will not be chosen as a successor and it is no harm to >> suspend it if needed when posting the event. >> >> Possible issue of posting a `waited` event while still be suspended is >> addressed by adding a suspension check just before the posting of event. >> >> Tests are added. >> >> Tested in tiers 1 - 7. > > Anton Artemov has updated the pull request incrementally with one additional > commit since the last revision: > > 8366659: Fixed whitespace. Looks good overall, just a few comments. Thanks. src/hotspot/share/runtime/objectMonitor.cpp line 558: > 556: bool is_virtual = ce != nullptr && ce->is_virtual_thread(); > 557: if (is_virtual) { > 558: notify_contended_enter(current, post_jvmti_events); We should also add `post_jvmti_event &&` to L567 below. src/hotspot/share/runtime/objectMonitor.cpp line 2065: > 2063: did_notify = true; > 2064: > 2065: if (!JvmtiExport::should_post_monitor_waited()) { For the vthread case this value could be different from the one read above. src/hotspot/share/runtime/objectMonitor.cpp line 2079: > 2077: iterator->TState = ObjectWaiter::TS_RUN; > 2078: > 2079: OrderAccess::fence(); Is the fence needed? We already synchronize with `_wait_set_lock`. src/hotspot/share/runtime/objectMonitor.cpp line 2085: > 2083: if (!iterator->is_vthread()) { > 2084: iterator->wait_reenter_begin(this); > 2085: if (has_unmounted_vthreads()) { The thread is going to call `enter` for this case so this is not needed. Same with `wait_reenter_begin` above. src/hotspot/share/runtime/objectMonitor.cpp line 2105: > 2103: } > 2104: return did_notify; > 2105: } Given that there are more differences than common code, I think it's probably better to just separate the vthread and platform thread paths to make it easier to read. Something like this: https://github.com/openjdk/jdk/compare/pr/27040...pchilano:jdk:altnotify You could add a comment on top of the method about the behavior when monitor_waited event is enabled. src/hotspot/share/runtime/objectMonitor.cpp line 2291: > 2289: // because, if monitor_waited JVMTI event is allowed, there can be a > vthead which > 2290: // is not on the entry_list, but has been notified in the sense that > it has been > 2291: // unparked directly in notify_internal(). Its state is then TS_RUN. Suggestion for shorter comment: // We check the state rather than was_notified because, when JVMTI // monitor_waited event is enabled, the notifier only unparks the waiter // without adding it to the entry_list. src/hotspot/share/runtime/objectMonitor.hpp line 367: > 365: bool enter_is_async_deflating(); > 366: void notify_contended_enter(JavaThread *current, bool > post_jvmti_events = true); > 367: void post_waited_event(JavaThread* current, EventJavaMonitorWait* > wait_event, bool was_notified, ObjectWaiter* node, jlong millis, int ret); Leftover? ------------- PR Review: https://git.openjdk.org/jdk/pull/27040#pullrequestreview-3685329768 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2710948914 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2710964239 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2710957840 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2710958952 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2710969804 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2710956795 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2710954338
