From 619fe63b5f112b9c7f3d2755504c70fd4bebade3 Mon Sep 17 00:00:00 2001 From: Anton Artemov Date: Wed, 11 Mar 2026 08:39:41 +0000 Subject: [PATCH] 8362239: Reconcile enter_internal and reenter_internal in the ObjectMonitor code Reviewed-by: dholmes, fbredberg, coleenp, pchilanomate, dcubed --- src/hotspot/share/runtime/objectMonitor.cpp | 255 ++++++++------------ src/hotspot/share/runtime/objectMonitor.hpp | 4 +- 2 files changed, 102 insertions(+), 157 deletions(-) diff --git a/src/hotspot/share/runtime/objectMonitor.cpp b/src/hotspot/share/runtime/objectMonitor.cpp index cb69b82d767..94399088464 100644 --- a/src/hotspot/share/runtime/objectMonitor.cpp +++ b/src/hotspot/share/runtime/objectMonitor.cpp @@ -593,12 +593,15 @@ void ObjectMonitor::enter_with_contention_mark(JavaThread* current, ObjectMonito OSThreadContendState osts(current->osthread()); assert(current->thread_state() == _thread_in_vm, "invariant"); + ObjectWaiter node(current); for (;;) { ExitOnSuspend eos(this); { ThreadBlockInVMPreprocess tbivs(current, eos, true /* allow_suspend */); - enter_internal(current); + if (!try_enter_fast(current, &node)) { + enter_internal(current, &node, false /* reenter_path */); + } current->set_current_pending_monitor(nullptr); // We can go to a safepoint at the end of this block. If we // do a thread dump during that safepoint, then this thread will show @@ -942,14 +945,17 @@ const char* ObjectMonitor::is_busy_to_string(stringStream* ss) { return ss->base(); } -void ObjectMonitor::enter_internal(JavaThread* current) { +bool ObjectMonitor::try_enter_fast(JavaThread* current, ObjectWaiter* current_node) { + assert(current != nullptr, "invariant"); assert(current->thread_state() == _thread_blocked, "invariant"); + assert(current_node != nullptr, "invariant"); + assert(current_node->_thread == current, "invariant"); // Try the lock - TATAS if (try_lock(current) == TryLockResult::Success) { assert(!has_successor(current), "invariant"); assert(has_owner(current), "invariant"); - return; + return true; } assert(InitDone, "Unexpectedly not initialized"); @@ -964,7 +970,7 @@ void ObjectMonitor::enter_internal(JavaThread* current) { if (try_spin(current)) { assert(has_owner(current), "invariant"); assert(!has_successor(current), "invariant"); - return; + return true; } // The Spin failed -- Enqueue and park the thread ... @@ -973,54 +979,67 @@ void ObjectMonitor::enter_internal(JavaThread* current) { // Enqueue "current" on ObjectMonitor's _entry_list. // - // Node acts as a proxy for current. + // current_node acts as a proxy for current. // As an aside, if were to ever rewrite the synchronization code mostly // in Java, WaitNodes, ObjectMonitors, and Events would become 1st-class // Java objects. This would avoid awkward lifecycle and liveness issues, // as well as eliminate a subset of ABA issues. // TODO: eliminate ObjectWaiter and enqueue either Threads or Events. - ObjectWaiter node(current); current->_ParkEvent->reset(); - if (try_lock_or_add_to_entry_list(current, &node)) { - return; // We got the lock. + if (try_lock_or_add_to_entry_list(current, current_node)) { + return true; // We got the lock. } + // This thread is now added to the _entry_list. // The lock might have been released while this thread was occupied queueing // itself onto _entry_list. To close the race and avoid "stranding" and - // progress-liveness failure we must resample-retry _owner before parking. + // progress-liveness failure the caller must resample-retry _owner before parking. // Note the Dekker/Lamport duality: ST _entry_list; MEMBAR; LD Owner. - // In this case the ST-MEMBAR is accomplished with CAS(). - // - // TODO: Defer all thread state transitions until park-time. - // Since state transitions are heavy and inefficient we'd like - // to defer the state transitions until absolutely necessary, - // and in doing so avoid some transitions ... + // In this case the ST-MEMBAR is accomplished with CAS() in try_lock_or_add_to_entry_list. + return false; +} + +void ObjectMonitor::enter_internal(JavaThread* current, ObjectWaiter* current_node, bool reenter_path) { + assert(current != nullptr, "invariant"); + assert(current->thread_state() == _thread_blocked, "invariant"); + assert(current_node != nullptr, "invariant"); + assert(current_node->_thread == current, "invariant"); // If there are unmounted virtual threads ahead in the _entry_list we want // to do a timed-park instead to alleviate some deadlock cases where one // of them is picked as the successor but cannot run due to having run out // of carriers. This can happen, for example, if this is a pinned virtual - // thread currently loading or initializining a class, and all other carriers + // thread currently loading or initializing a class, and all other carriers // have a pinned vthread waiting for said class to be loaded/initialized. // Read counter *after* adding this thread to the _entry_list. Adding to // _entry_list uses Atomic::cmpxchg() which already provides a fence that - // prevents this load from floating up previous store. + // prevents this load from floating up past a previous store. // Note that we can have false positives where timed-park is not necessary. - bool do_timed_parked = has_unmounted_vthreads(); + bool do_timed_park = has_unmounted_vthreads(); jlong recheck_interval = 1; for (;;) { + ObjectWaiter::TStates v = current_node->TState; + guarantee(v == ObjectWaiter::TS_ENTER, "invariant"); if (try_lock(current) == TryLockResult::Success) { break; } assert(!has_owner(current), "invariant"); + if (reenter_path) { + // If try_lock failed, spin again - we expect the notifier to release the monitor quickly. + // Note that spin count may be zero so the above try_lock is necessary. + if (try_spin(current)) { + break; + } + } + // park self - if (do_timed_parked) { + if (do_timed_park) { current->_ParkEvent->park(recheck_interval); // Increase the recheck_interval, but clamp the value. recheck_interval *= 8; @@ -1031,125 +1050,6 @@ void ObjectMonitor::enter_internal(JavaThread* current) { current->_ParkEvent->park(); } - if (try_lock(current) == TryLockResult::Success) { - break; - } - - // The lock is still contested. - - // Assuming this is not a spurious wakeup we'll normally find _succ == current. - // We can defer clearing _succ until after the spin completes - // try_spin() must tolerate being called with _succ == current. - // Try yet another round of adaptive spinning. - if (try_spin(current)) { - break; - } - - // We can find that we were unpark()ed and redesignated _succ while - // we were spinning. That's harmless. If we iterate and call park(), - // park() will consume the event and return immediately and we'll - // just spin again. This pattern can repeat, leaving _succ to simply - // spin on a CPU. - - if (has_successor(current)) clear_successor(); - - // Invariant: after clearing _succ a thread *must* retry _owner before parking. - OrderAccess::fence(); - } - - // Egress : - // Current has acquired the lock -- Unlink current from the _entry_list. - unlink_after_acquire(current, &node); - if (has_successor(current)) { - clear_successor(); - // Note that we don't need to do OrderAccess::fence() after clearing - // _succ here, since we own the lock. - } - - // We've acquired ownership with CAS(). - // CAS is serializing -- it has MEMBAR/FENCE-equivalent semantics. - // But since the CAS() this thread may have also stored into _succ - // or entry_list. These meta-data updates must be visible __before - // this thread subsequently drops the lock. - // Consider what could occur if we didn't enforce this constraint -- - // STs to monitor meta-data and user-data could reorder with (become - // visible after) the ST in exit that drops ownership of the lock. - // Some other thread could then acquire the lock, but observe inconsistent - // or old monitor meta-data and heap data. That violates the JMM. - // To that end, the exit() operation must have at least STST|LDST - // "release" barrier semantics. Specifically, there must be at least a - // STST|LDST barrier in exit() before the ST of null into _owner that drops - // the lock. The barrier ensures that changes to monitor meta-data and data - // protected by the lock will be visible before we release the lock, and - // therefore before some other thread (CPU) has a chance to acquire the lock. - // See also: http://gee.cs.oswego.edu/dl/jmm/cookbook.html. - // - // Critically, any prior STs to _succ or entry_list must be visible before - // the ST of null into _owner in the *subsequent* (following) corresponding - // monitorexit. - - return; -} - -// reenter_internal() is a specialized inline form of the latter half of the -// contended slow-path from enter_internal(). We use reenter_internal() only for -// monitor reentry in wait(). -// -// In the future we should reconcile enter_internal() and reenter_internal(). - -void ObjectMonitor::reenter_internal(JavaThread* current, ObjectWaiter* currentNode) { - assert(current != nullptr, "invariant"); - assert(current->thread_state() == _thread_blocked, "invariant"); - assert(currentNode != nullptr, "invariant"); - assert(currentNode->_thread == current, "invariant"); - assert(_waiters > 0, "invariant"); - - // If there are unmounted virtual threads ahead in the _entry_list we want - // to do a timed-park instead to alleviate some deadlock cases where one - // of them is picked as the successor but cannot run due to having run out - // of carriers. This can happen, for example, if a mixed of unmounted and - // pinned vthreads taking up all the carriers are waiting for a class to be - // initialized, and the selected successor is one of the unmounted vthreads. - // Although this method is used for the "notification" case, it could be - // that this thread reached here without been added to the _entry_list yet. - // This can happen if it was interrupted or the wait timed-out at the same - // time. In that case we rely on currentNode->_do_timed_park, which will be - // read on the next loop iteration, after consuming the park permit set by - // the notifier in notify_internal. - // Note that we can have false positives where timed-park is not necessary. - bool do_timed_parked = has_unmounted_vthreads(); - jlong recheck_interval = 1; - - for (;;) { - ObjectWaiter::TStates v = currentNode->TState; - guarantee(v == ObjectWaiter::TS_ENTER, "invariant"); - assert(!has_owner(current), "invariant"); - - // This thread has been notified so try to reacquire the lock. - if (try_lock(current) == TryLockResult::Success) { - break; - } - - // If that fails, spin again. Note that spin count may be zero so the above TryLock - // is necessary. - if (try_spin(current)) { - break; - } - - { - OSThreadContendState osts(current->osthread()); - if (do_timed_parked) { - current->_ParkEvent->park(recheck_interval); - // Increase the recheck_interval, but clamp the value. - recheck_interval *= 8; - if (recheck_interval > MAX_RECHECK_INTERVAL) { - recheck_interval = MAX_RECHECK_INTERVAL; - } - } else { - current->_ParkEvent->park(); - } - } - // Try again, but just so we distinguish between futile wakeups and // successful wakeups. The following test isn't algorithmically // necessary, but it helps us maintain sensible statistics. @@ -1159,25 +1059,68 @@ void ObjectMonitor::reenter_internal(JavaThread* current, ObjectWaiter* currentN // The lock is still contested. - // Assuming this is not a spurious wakeup we'll normally - // find that _succ == current. - if (has_successor(current)) clear_successor(); + if (!reenter_path) { + // Assuming this is not a spurious wakeup we'll normally find _succ == current. + // We can defer clearing _succ until after the spin completes and + // try_spin() must tolerate being called with _succ == current. + // Try yet another round of adaptive spinning. + if (try_spin(current)) { + break; + } + } - // Invariant: after clearing _succ a contending thread - // *must* retry _owner before parking. + // We can find that we were unpark()ed and redesignated _succ while + // we were spinning. That's harmless. If we iterate and call park(), + // park() will consume the event and return immediately and we'll + // just spin again. This pattern can repeat, leaving _succ to simply + // spin on a CPU. + + if (has_successor(current)) { + clear_successor(); + } + + // Invariant: after clearing _succ a thread *must* retry _owner before parking. OrderAccess::fence(); - // See comment in notify_internal - do_timed_parked |= currentNode->_do_timed_park; + // Will only potentially change on the reenter path - see comment in notify_internal. + do_timed_park |= current_node->_do_timed_park; } - // Current has acquired the lock -- Unlink current from the _entry_list. assert(has_owner(current), "invariant"); - unlink_after_acquire(current, currentNode); - if (has_successor(current)) clear_successor(); - assert(!has_successor(current), "invariant"); - currentNode->TState = ObjectWaiter::TS_RUN; - OrderAccess::fence(); // see comments at the end of enter_internal() + + // Egress : + // Current has acquired the lock -- Unlink current from the _entry_list. + unlink_after_acquire(current, current_node); + if (has_successor(current)) { + clear_successor(); + // Note that we don't need to do OrderAccess::fence() after clearing + // _succ here, since we own the lock. + } + + // We've acquired ownership with CAS(). + // CAS is serializing -- it has MEMBAR/FENCE-equivalent semantics. + // But since the CAS() this thread may have also stored into _succ + // or entry_list. These meta-data updates must be visible __before + // this thread subsequently drops the lock. + // Consider what could occur if we didn't enforce this constraint -- + // STs to monitor meta-data and user-data could reorder with (become + // visible after) the ST in exit that drops ownership of the lock. + // Some other thread could then acquire the lock, but observe inconsistent + // or old monitor meta-data and heap data. That violates the JMM. + // To that end, the exit() operation must have at least STST|LDST + // "release" barrier semantics. Specifically, there must be at least a + // STST|LDST barrier in exit() before the ST of null into _owner that drops + // the lock. The barrier ensures that changes to monitor meta-data and data + // protected by the lock will be visible before we release the lock, and + // therefore before some other thread (CPU) has a chance to acquire the lock. + // See also: http://gee.cs.oswego.edu/dl/jmm/cookbook.html. + // + // Critically, any prior STs to _succ or entry_list must be visible before + // the ST of null into _owner in the *subsequent* (following) corresponding + // monitorexit. + + current_node->TState = ObjectWaiter::TS_RUN; + return; } // This method is called from two places: @@ -1971,7 +1914,9 @@ void ObjectMonitor::wait(jlong millis, bool interruptible, TRAPS) { ExitOnSuspend eos(this); { ThreadBlockInVMPreprocess tbivs(current, eos, true /* allow_suspend */); - reenter_internal(current, &node); + assert( _waiters > 0, "invariant"); + OSThreadContendState osts(current->osthread()); + enter_internal(current, &node, true /* reenter_path */); // We can go to a safepoint at the end of this block. If we // do a thread dump during that safepoint, then this thread will show // as having "-locked" the monitor, but the OS and java.lang.Thread @@ -2082,12 +2027,12 @@ bool ObjectMonitor::notify_internal(JavaThread* current) { // Wake up the thread to alleviate some deadlock cases where the successor // that will be picked up when this thread releases the monitor is an unmounted // virtual thread that cannot run due to having run out of carriers. Upon waking - // up, the thread will call reenter_internal() which will use timed-park in case + // up, the thread will call enter_internal(..., true) which will use timed-park in case // there is contention and there are still vthreads in the _entry_list. // If the target was interrupted or the wait timed-out at the same time, it could - // have reached reenter_internal and read a false value of has_unmounted_vthreads() + // have reached enter_internal and read a false value of has_unmounted_vthreads() // before we added it to the _entry_list above. To deal with that case, we set _do_timed_park - // which will be read by the target on the next loop iteration in reenter_internal. + // which will be read by the target on the next loop iteration in enter_internal. iterator->_do_timed_park = true; JavaThread* t = iterator->thread(); t->_ParkEvent->unpark(); diff --git a/src/hotspot/share/runtime/objectMonitor.hpp b/src/hotspot/share/runtime/objectMonitor.hpp index 8d9a481bf37..842aa1b374e 100644 --- a/src/hotspot/share/runtime/objectMonitor.hpp +++ b/src/hotspot/share/runtime/objectMonitor.hpp @@ -394,8 +394,8 @@ class ObjectMonitor : public CHeapObj { bool notify_internal(JavaThread* current); ObjectWaiter* dequeue_waiter(); void dequeue_specific_waiter(ObjectWaiter* waiter); - void enter_internal(JavaThread* current); - void reenter_internal(JavaThread* current, ObjectWaiter* current_node); + void enter_internal(JavaThread* current, ObjectWaiter* current_node, bool reenter_path); + bool try_enter_fast(JavaThread* current, ObjectWaiter* current_node); void entry_list_build_dll(JavaThread* current); void unlink_after_acquire(JavaThread* current, ObjectWaiter* current_node); ObjectWaiter* entry_list_tail(JavaThread* current);