8362239: Reconcile enter_internal and reenter_internal in the ObjectMonitor code

Reviewed-by: dholmes, fbredberg, coleenp, pchilanomate, dcubed

Author: toxaart

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<ExitOnSuspend> 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,18 +1050,23 @@ void ObjectMonitor::enter_internal(JavaThread* current) {
       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.
     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;
+    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;
+      }
     }
 
     // We can find that we were unpark()ed and redesignated _succ while
@@ -1051,15 +1075,22 @@ void ObjectMonitor::enter_internal(JavaThread* current) {
     // just spin again.  This pattern can repeat, leaving _succ to simply
     // spin on a CPU.
 
-    if (has_successor(current)) clear_successor();
+    if (has_successor(current)) {
+      clear_successor();
+    }
 
     // Invariant: after clearing _succ a thread *must* retry _owner before parking.
     OrderAccess::fence();
+
+    // Will only potentially change on the reenter path - see comment in notify_internal.
+    do_timed_park |= current_node->_do_timed_park;
   }
 
+  assert(has_owner(current), "invariant");
+
   // Egress :
   // Current has acquired the lock -- Unlink current from the _entry_list.
-  unlink_after_acquire(current, &node);
+  unlink_after_acquire(current, current_node);
   if (has_successor(current)) {
     clear_successor();
     // Note that we don't need to do OrderAccess::fence() after clearing
@@ -1069,17 +1100,17 @@ void ObjectMonitor::enter_internal(JavaThread* current) {
   // 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
+  // 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.
+  // 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
+  // "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
+  // 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.
@@ -1088,98 +1119,10 @@ void ObjectMonitor::enter_internal(JavaThread* current) {
   // the ST of null into _owner in the *subsequent* (following) corresponding
   // monitorexit.
 
+  current_node->TState = ObjectWaiter::TS_RUN;
   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.
-    if (try_lock(current) == TryLockResult::Success) {
-      break;
-    }
-
-    // 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();
-
-    // Invariant: after clearing _succ a contending thread
-    // *must* retry  _owner before parking.
-    OrderAccess::fence();
-
-    // See comment in notify_internal
-    do_timed_parked |= currentNode->_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()
-}
-
 // This method is called from two places:
 // - On monitorenter contention with a null waiter.
 // - After Object.wait() times out or the target is interrupted to reenter the
@@ -1971,7 +1914,9 @@ void ObjectMonitor::wait(jlong millis, bool interruptible, TRAPS) {
       ExitOnSuspend eos(this);
       {
         ThreadBlockInVMPreprocess<ExitOnSuspend> 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();

src/hotspot/share/runtime/objectMonitor.hpp

@@ -394,8 +394,8 @@ class ObjectMonitor : public CHeapObj<mtObjectMonitor> {
   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);