8354180: Clean up uses of ObjectMonitor caches

Co-authored-by: Axel Boldt-Christmas <[email protected]>
Reviewed-by: aboldtch, fbredberg

Author: coleenp

src/hotspot/share/runtime/basicLock.cpp

@@ -27,7 +27,7 @@
 #include "runtime/objectMonitor.hpp"
 #include "runtime/synchronizer.hpp"
 
-void BasicLock::print_on(outputStream* st, oop owner) {
+void BasicLock::print_on(outputStream* st, oop owner) const {
   st->print("monitor");
   if (UseObjectMonitorTable) {
     ObjectMonitor* mon = object_monitor_cache();

src/hotspot/share/runtime/basicLock.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2024, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2025, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -61,12 +61,12 @@ class BasicLock {
   static int displaced_header_offset_in_bytes() { return metadata_offset_in_bytes(); }
 
   // LM_LIGHTWEIGHT
-  inline ObjectMonitor* object_monitor_cache();
+  inline ObjectMonitor* object_monitor_cache() const;
   inline void clear_object_monitor_cache();
   inline void set_object_monitor_cache(ObjectMonitor* mon);
   static int object_monitor_cache_offset_in_bytes() { return metadata_offset_in_bytes(); }
 
-  void print_on(outputStream* st, oop owner);
+  void print_on(outputStream* st, oop owner) const;
 
   // move a basic lock (used during deoptimization)
   void move_to(oop obj, BasicLock* dest);

src/hotspot/share/runtime/basicLock.inline.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2024, 2025, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -38,15 +38,10 @@ inline void BasicLock::set_displaced_header(markWord header) {
   Atomic::store(&_metadata, header.value());
 }
 
-inline ObjectMonitor* BasicLock::object_monitor_cache() {
+inline ObjectMonitor* BasicLock::object_monitor_cache() const {
   assert(UseObjectMonitorTable, "must be");
 #if !defined(ZERO) && (defined(X86) || defined(AARCH64) || defined(RISCV64) || defined(PPC64) || defined(S390))
-  ObjectMonitor* monitor = reinterpret_cast<ObjectMonitor*>(get_metadata());
-  if (monitor != nullptr && monitor->is_being_async_deflated()) {
-    clear_object_monitor_cache();
-    return nullptr;
-  }
-  return monitor;
+  return reinterpret_cast<ObjectMonitor*>(get_metadata());
 #else
   // Other platforms do not make use of the cache yet,
   // and are not as careful with maintaining the invariant

src/hotspot/share/runtime/deoptimization.cpp

@@ -1666,6 +1666,9 @@ bool Deoptimization::relock_objects(JavaThread* thread, GrowableArray<MonitorInf
           // Entering may create an invalid lock stack. Inflate the lock if it
           // was fast_locked to restore the valid lock stack.
           if (UseObjectMonitorTable) {
+            // UseObjectMonitorTable expects the BasicLock cache to be either a
+            // valid ObjectMonitor* or nullptr. Right now it is garbage, set it
+            // to nullptr.
             lock->clear_object_monitor_cache();
           }
           ObjectSynchronizer::enter_for(obj, lock, deoptee_thread);

src/hotspot/share/runtime/lightweightSynchronizer.cpp

@@ -519,8 +519,12 @@ class LightweightSynchronizer::CacheSetter : StackObj {
     // Only use the cache if using the table.
     if (UseObjectMonitorTable) {
       if (_monitor != nullptr) {
-        _thread->om_set_monitor_cache(_monitor);
-        _lock->set_object_monitor_cache(_monitor);
+        // If the monitor is already in the BasicLock cache then it is most
+        // likely in the thread cache, do not set it again to avoid reordering.
+        if (_monitor != _lock->object_monitor_cache()) {
+          _thread->om_set_monitor_cache(_monitor);
+          _lock->set_object_monitor_cache(_monitor);
+        }
       } else {
         _lock->clear_object_monitor_cache();
       }
@@ -534,6 +538,16 @@ class LightweightSynchronizer::CacheSetter : StackObj {
 
 };
 
+// Reads first from the BasicLock cache then from the OMCache in the current thread.
+// C2 fast-path may have put the monitor in the cache in the BasicLock.
+inline static ObjectMonitor* read_caches(JavaThread* current, BasicLock* lock, oop object) {
+  ObjectMonitor* monitor = lock->object_monitor_cache();
+  if (monitor == nullptr) {
+    monitor = current->om_get_from_monitor_cache(object);
+  }
+  return monitor;
+}
+
 class LightweightSynchronizer::VerifyThreadState {
   bool _no_safepoint;
 
@@ -615,15 +629,14 @@ bool LightweightSynchronizer::fast_lock_spin_enter(oop obj, LockStack& lock_stac
 
 void LightweightSynchronizer::enter_for(Handle obj, BasicLock* lock, JavaThread* locking_thread) {
   assert(LockingMode == LM_LIGHTWEIGHT, "must be");
+  assert(!UseObjectMonitorTable || lock->object_monitor_cache() == nullptr, "must be cleared");
   JavaThread* current = JavaThread::current();
   VerifyThreadState vts(locking_thread, current);
 
   if (obj->klass()->is_value_based()) {
     ObjectSynchronizer::handle_sync_on_value_based_class(obj, locking_thread);
   }
 
-  CacheSetter cache_setter(locking_thread, lock);
-
   LockStack& lock_stack = locking_thread->lock_stack();
 
   ObjectMonitor* monitor = nullptr;
@@ -640,7 +653,7 @@ void LightweightSynchronizer::enter_for(Handle obj, BasicLock* lock, JavaThread*
   }
 
   assert(monitor != nullptr, "LightweightSynchronizer::enter_for must succeed");
-  cache_setter.set_monitor(monitor);
+  assert(!UseObjectMonitorTable || lock->object_monitor_cache() == nullptr, "unused. already cleared");
 }
 
 void LightweightSynchronizer::enter(Handle obj, BasicLock* lock, JavaThread* current) {
@@ -741,12 +754,9 @@ void LightweightSynchronizer::exit(oop object, BasicLock* lock, JavaThread* curr
   // The monitor exists
   ObjectMonitor* monitor;
   if (UseObjectMonitorTable) {
-    monitor = lock->object_monitor_cache();
+    monitor = read_caches(current, lock, object);
     if (monitor == nullptr) {
-      monitor = current->om_get_from_monitor_cache(object);
-      if (monitor == nullptr) {
-        monitor = get_monitor_from_table(current, object);
-      }
+      monitor = get_monitor_from_table(current, object);
     }
   } else {
     monitor = ObjectSynchronizer::read_monitor(mark);
@@ -1019,10 +1029,7 @@ ObjectMonitor* LightweightSynchronizer::inflate_and_enter(oop object, BasicLock*
   // There's no need to use the cache if we are locking
   // on behalf of another thread.
   if (current == locking_thread) {
-    monitor = lock->object_monitor_cache();
-    if (monitor == nullptr) {
-      monitor = current->om_get_from_monitor_cache(object);
-    }
+    monitor = read_caches(current, lock, object);
   }
 
   // Get or create the monitor
@@ -1044,6 +1051,9 @@ ObjectMonitor* LightweightSynchronizer::inflate_and_enter(oop object, BasicLock*
     // The MonitorDeflation thread is deflating the monitor. The locking thread
     // must spin until further progress has been made.
 
+    // Clear the BasicLock cache as it may contain this monitor.
+    lock->clear_object_monitor_cache();
+
     const markWord mark = object->mark_acquire();
 
     if (mark.has_monitor()) {
@@ -1204,12 +1214,7 @@ bool LightweightSynchronizer::quick_enter(oop obj, BasicLock* lock, JavaThread*
   if (mark.has_monitor()) {
     ObjectMonitor* monitor;
     if (UseObjectMonitorTable) {
-      // C2 fast-path may have put the monitor in the cache in the BasicLock.
-      monitor = lock->object_monitor_cache();
-      if (monitor == nullptr) {
-        // Otherwise look up the monitor in the thread's OMCache.
-        monitor = current->om_get_from_monitor_cache(obj);
-      }
+      monitor = read_caches(current, lock, obj);
     } else {
       monitor = ObjectSynchronizer::read_monitor(mark);
     }
@@ -1220,6 +1225,11 @@ bool LightweightSynchronizer::quick_enter(oop obj, BasicLock* lock, JavaThread*
     }
 
     if (UseObjectMonitorTable) {
+      // Set the monitor regardless of success.
+      // Either we successfully lock on the monitor, or we retry with the
+      // monitor in the slow path. If the monitor gets deflated, it will be
+      // cleared, either by the CacheSetter if we fast lock in enter or in
+      // inflate_and_enter when we see that the monitor is deflated.
       lock->set_object_monitor_cache(monitor);
     }
 

src/hotspot/share/runtime/objectMonitor.cpp

@@ -2571,7 +2571,7 @@ void ObjectMonitor::print_on(outputStream* st) const {
   st->print("{contentions=0x%08x,waiters=0x%08x"
             ",recursions=%zd,owner=" INT64_FORMAT "}",
             contentions(), waiters(), recursions(),
-            owner());
+            owner_raw());
 }
 void ObjectMonitor::print() const { print_on(tty); }