8348402: PerfDataManager stalls shutdown for 1ms

src/hotspot/share/runtime/objectMonitor.cpp

@@ -53,6 +53,7 @@
 #include "runtime/sharedRuntime.hpp"
 #include "services/threadService.hpp"
 #include "utilities/dtrace.hpp"
+#include "utilities/globalCounter.inline.hpp"
 #include "utilities/macros.hpp"
 #include "utilities/preserveException.hpp"
 #if INCLUDE_JFR
@@ -828,9 +829,9 @@ void ObjectMonitor::EnterI(JavaThread* current) {
     // That is by design - we trade "lossy" counters which are exposed to
     // races during updates for a lower probe effect.
 
-    // This PerfData object can be used in parallel with a safepoint.
-    // See the work around in PerfDataManager::destroy().
-    OM_PERFDATA_OP(FutileWakeups, inc());
+    // We are in safepoint safe state, so shutdown can remove the counter
+    // under our feet. Make sure we make this access safely.
+    OM_PERFDATA_SAFE_OP(FutileWakeups, inc());
     ++nWakeups;
 
     // Assuming this is not a spurious wakeup we'll normally find _succ == current.
@@ -971,8 +972,6 @@ void ObjectMonitor::ReenterI(JavaThread* current, ObjectWaiter* currentNode) {
     // *must* retry  _owner before parking.
     OrderAccess::fence();
 
-    // This PerfData object can be used in parallel with a safepoint.
-    // See the work around in PerfDataManager::destroy().
     OM_PERFDATA_OP(FutileWakeups, inc());
   }
 

src/hotspot/share/runtime/objectMonitor.hpp

@@ -197,13 +197,31 @@ class ObjectMonitor : public CHeapObj<mtObjectMonitor> {
 
   // Only perform a PerfData operation if the PerfData object has been
   // allocated and if the PerfDataManager has not freed the PerfData
-  // objects which can happen at normal VM shutdown.
-  //
+  // objects which can happen at normal VM shutdown. This operation is
+  // only safe when thread is not in safepoint-safe code, i.e. PerfDataManager
+  // could not reach the safepoint and free the counter while we are using it.
+  // If this is not guaranteed, use OM_PERFDATA_SAFE_OP instead.
   #define OM_PERFDATA_OP(f, op_str)                 \
     do {                                            \
-      if (ObjectMonitor::_sync_ ## f != nullptr &&  \
-          PerfDataManager::has_PerfData()) {        \
-        ObjectMonitor::_sync_ ## f->op_str;         \
+      if (ObjectMonitor::_sync_ ## f != nullptr) {  \
+        if (PerfDataManager::has_PerfData()) {      \
+          ObjectMonitor::_sync_ ## f->op_str;       \
+        }                                           \
+      }                                             \
+    } while (0)
+
+  // Only perform a PerfData operation if the PerfData object has been
+  // allocated and if the PerfDataManager has not freed the PerfData
+  // objects which can happen at normal VM shutdown. Additionally, we
+  // enter the critical section to resolve the race against PerfDataManager
+  // entering the safepoint and deleting the counter during shutdown.
+  #define OM_PERFDATA_SAFE_OP(f, op_str)            \
+    do {                                            \
+      if (ObjectMonitor::_sync_ ## f != nullptr) {  \
+        GlobalCounter::CriticalSection cs(Thread::current()); \
+        if (PerfDataManager::has_PerfData()) {      \
+          ObjectMonitor::_sync_ ## f->op_str;       \
+        }                                           \
       }                                             \
     } while (0)
 

src/hotspot/share/runtime/perfData.cpp

@@ -35,6 +35,7 @@
 #include "runtime/os.hpp"
 #include "runtime/perfData.inline.hpp"
 #include "utilities/exceptions.hpp"
+#include "utilities/globalCounter.inline.hpp"
 #include "utilities/globalDefinitions.hpp"
 
 PerfDataList*   PerfDataManager::_all = nullptr;
@@ -251,15 +252,13 @@ void PerfDataManager::destroy() {
     // destroy already called, or initialization never happened
     return;
 
-  // Clear the flag before we free the PerfData counters. Thus begins
-  // the race between this thread and another thread that has just
-  // queried PerfDataManager::has_PerfData() and gotten back 'true'.
-  // The hope is that the other thread will finish its PerfData
-  // manipulation before we free the memory. The two alternatives are
-  // 1) leak the PerfData memory or 2) do some form of synchronized
-  // access or check before every PerfData operation.
-  _has_PerfData = false;
-  os::naked_short_sleep(1);  // 1ms sleep to let other thread(s) run
+  // About to delete the counters than might still be accessed by other threads.
+  // The shutdown is performed in two stages: a) clear the flag to notify future
+  // counter users that we are at shutdown; b) sync up with current users, waiting
+  // for them to finish with counters.
+  //
+  Atomic::store(&_has_PerfData, false);
+  GlobalCounter::write_synchronize();
 
   log_debug(perf, datacreation)("Total = %d, Sampled = %d, Constants = %d",
                                 _all->length(), _sampled == nullptr ? 0 : _sampled->length(),
@@ -286,7 +285,7 @@ void PerfDataManager::add_item(PerfData* p, bool sampled) {
   // Default sizes determined using -Xlog:perf+datacreation=debug
   if (_all == nullptr) {
     _all = new PerfDataList(191);
-    _has_PerfData = true;
+    Atomic::release_store(&_has_PerfData, true);
   }
 
   assert(!_all->contains(p->name()), "duplicate name added");

src/hotspot/share/runtime/perfData.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2001, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2001, 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
@@ -26,6 +26,7 @@
 #define SHARE_RUNTIME_PERFDATA_HPP
 
 #include "memory/allocation.hpp"
+#include "runtime/atomic.hpp"
 #include "runtime/perfDataTypes.hpp"
 #include "runtime/perfMemory.hpp"
 #include "runtime/timer.hpp"
@@ -241,6 +242,18 @@ enum CounterNS {
  * UsePerfData, a product flag set to true by default. This flag may
  * be removed from the product in the future.
  *
+ * There are possible shutdown races between counter uses and counter
+ * destruction code. Normal shutdown happens with taking VM_Exit safepoint
+ * operation, so in the vast majority of uses this is not an issue. On the
+ * paths where a concurrent access can still happen when VM is at safepoint,
+ * use the following pattern to coordinate with shutdown:
+ *
+ * {
+ *   GlobalCounter::CriticalSection cs(Thread::current());
+ *   if (PerfDataManager::has_PerfData()) {
+ *     <update-counter>
+ *   }
+ * }
  */
 class PerfData : public CHeapObj<mtInternal> {
 
@@ -785,7 +798,7 @@ class PerfDataManager : AllStatic {
     }
 
     static void destroy();
-    static bool has_PerfData() { return _has_PerfData; }
+    static bool has_PerfData() { return Atomic::load_acquire(&_has_PerfData); }
 };
 
 // Useful macros to create the performance counters

src/hotspot/share/runtime/synchronizer.cpp

@@ -60,6 +60,7 @@
 #include "utilities/align.hpp"
 #include "utilities/dtrace.hpp"
 #include "utilities/events.hpp"
+#include "utilities/globalCounter.inline.hpp"
 #include "utilities/linkedlist.hpp"
 #include "utilities/preserveException.hpp"