8358429: JFR: minimize the time the Threads_lock is held for sampling

Reviewed-by: egahlin

src/hotspot/share/jfr/periodic/sampling/jfrSampleMonitor.hpp

@@ -0,0 +1,74 @@
+/*
+ * Copyright (c) 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ *
+ */
+
+#ifndef SHARE_JFR_PERIODIC_SAMPLING_JFRSAMPLEMONITOR_HPP
+#define SHARE_JFR_PERIODIC_SAMPLING_JFRSAMPLEMONITOR_HPP
+
+#include "jfr/periodic/sampling/jfrSampleRequest.hpp"
+#include "jfr/utilities/jfrTime.hpp"
+#include "memory/allocation.hpp"
+#include "runtime/javaThread.hpp"
+#include "runtime/mutex.hpp"
+
+class JfrSampleMonitor : public StackObj {
+ private:
+  JfrThreadLocal* const _tl;
+  Monitor* const _sample_monitor;
+  mutable bool _waiting;
+ public:
+  JfrSampleMonitor(JfrThreadLocal* tl) :
+    _tl(tl), _sample_monitor(tl->sample_monitor()), _waiting(false) {
+    assert(tl != nullptr, "invariant");
+    assert(_sample_monitor != nullptr, "invariant");
+    _sample_monitor->lock_without_safepoint_check();
+  }
+
+  bool is_waiting() const {
+    assert_lock_strong(_sample_monitor);
+    _waiting = _tl->sample_state() == WAITING_FOR_NATIVE_SAMPLE;
+    return _waiting;
+  }
+
+  void install_java_sample_request() {
+    assert_lock_strong(_sample_monitor);
+    assert(_waiting, "invariant");
+    assert(_tl->sample_state() == WAITING_FOR_NATIVE_SAMPLE, "invariant");
+    JfrSampleRequest request;
+    request._sample_ticks = JfrTicks::now();
+    _tl->set_sample_request(request);
+    _tl->set_sample_state(JAVA_SAMPLE);
+    _sample_monitor->notify_all();
+  }
+
+  ~JfrSampleMonitor() {
+    assert_lock_strong(_sample_monitor);
+    if (!_waiting) {
+      _tl->set_sample_state(NO_SAMPLE);
+      _sample_monitor->notify_all();
+    }
+    _sample_monitor->unlock();
+  }
+};
+
+#endif // SHARE_JFR_PERIODIC_SAMPLING_JFRSAMPLEMONITOR_HPP

src/hotspot/share/jfr/periodic/sampling/jfrSampleRequest.hpp

@@ -48,10 +48,11 @@ enum JfrSampleResult {
 };
 
 enum JfrSampleRequestType {
-  NO_SAMPLE = 0,
-  NATIVE_SAMPLE = 1,
-  JAVA_SAMPLE = 2,
-  NOF_SAMPLE_TYPES
+  NO_SAMPLE,
+  JAVA_SAMPLE,
+  NATIVE_SAMPLE,
+  WAITING_FOR_NATIVE_SAMPLE,
+  NOF_SAMPLE_STATES
 };
 
 struct JfrSampleRequest {

src/hotspot/share/jfr/periodic/sampling/jfrThreadSampler.cpp

@@ -24,6 +24,7 @@
 
 #include "jfr/metadata/jfrSerializer.hpp"
 #include "jfr/recorder/service/jfrOptionSet.hpp"
+#include "jfr/periodic/sampling/jfrSampleMonitor.hpp"
 #include "jfr/periodic/sampling/jfrSampleRequest.hpp"
 #include "jfr/periodic/sampling/jfrThreadSampling.hpp"
 #include "jfr/periodic/sampling/jfrThreadSampler.hpp"
@@ -230,38 +231,41 @@ void JfrSamplerThread::task_stacktrace(JfrSampleRequestType type, JavaThread** l
   JavaThread* start = nullptr;
   elapsedTimer sample_time;
   sample_time.start();
-  {
-    MutexLocker tlock(Threads_lock);
-    ThreadsListHandle tlh;
-    // Resolve a sample session relative start position index into the thread list array.
-    // In cases where the last sampled thread is null or not-null but stale, find_index() returns -1.
-    _cur_index = tlh.list()->find_index_of_JavaThread(*last_thread);
-    JavaThread* current = _cur_index != -1 ? *last_thread : nullptr;
+  ThreadsListHandle tlh;
+  // Resolve a sample session relative start position index into the thread list array.
+  // In cases where the last sampled thread is null or not-null but stale, find_index() returns -1.
+  _cur_index = tlh.list()->find_index_of_JavaThread(*last_thread);
+  JavaThread* current = _cur_index != -1 ? *last_thread : nullptr;
 
-    while (num_samples < sample_limit) {
-      current = next_thread(tlh.list(), start, current);
-      if (current == nullptr) {
-        break;
-      }
-      if (is_excluded(current)) {
-        continue;
-      }
-      if (start == nullptr) {
-        start = current; // remember the thread where we started to attempt sampling
-      }
-      bool success;
-      if (JAVA_SAMPLE == type) {
-        success = sample_java_thread(current);
-      } else {
-        assert(type == NATIVE_SAMPLE, "invariant");
-        success = sample_native_thread(current);
-      }
-      if (success) {
-        num_samples++;
-      }
+  while (num_samples < sample_limit) {
+    current = next_thread(tlh.list(), start, current);
+    if (current == nullptr) {
+      break;
+    }
+    if (is_excluded(current)) {
+      continue;
+    }
+    if (start == nullptr) {
+      start = current; // remember the thread where we started to attempt sampling
+    }
+    bool success;
+    if (JAVA_SAMPLE == type) {
+      success = sample_java_thread(current);
+    } else {
+      assert(type == NATIVE_SAMPLE, "invariant");
+      success = sample_native_thread(current);
+    }
+    if (success) {
+      num_samples++;
+    }
+    if (SafepointSynchronize::is_at_safepoint()) {
+      // For _thread_in_native, we cannot get the Threads_lock.
+      // For _thread_in_Java, well, there are none.
+      break;
     }
-    *last_thread = current; // remember the thread we last attempted to sample
   }
+
+  *last_thread = current; // remember the thread we last attempted to sample
   sample_time.stop();
   log_trace(jfr)("JFR thread sampling done in %3.7f secs with %d java %d native samples",
     sample_time.seconds(), type == JAVA_SAMPLE ? num_samples : 0, type == NATIVE_SAMPLE ? num_samples : 0);
@@ -338,17 +342,32 @@ bool JfrSamplerThread::sample_native_thread(JavaThread* jt) {
 
   SafepointMechanism::arm_local_poll_release(jt);
 
-  // Barriers needed to keep the next read of thread state from floating up.
-  if (UseSystemMemoryBarrier) {
-    SystemMemoryBarrier::emit();
-  } else {
-    OrderAccess::storeload();
+  // Take the Threads_lock for two purposes:
+  // 1) Avoid sampling through a safepoint which could result
+  //    in touching oops in case of virtual threads.
+  // 2) Prevent JFR from issuing an epoch rotation while the sampler thread
+  //    is actively processing a thread in native, as both threads are now
+  //    outside the safepoint protocol.
+
+  // OrderAccess::fence() as part of acquiring the lock prevents loads from floating up.
+  JfrMutexTryLock threads_lock(Threads_lock);
+
+  if (!threads_lock.acquired() || !jt->has_last_Java_frame()) {
+    // Remove the native sample request and release the potentially waiting thread.
+    JfrSampleMonitor jsm(tl);
+    return false;
   }
 
-  if (jt->thread_state() != _thread_in_native || !jt->has_last_Java_frame()) {
-    MonitorLocker lock(tl->sample_monitor(), Monitor::_no_safepoint_check_flag);
-    tl->set_sample_state(NO_SAMPLE);
-    lock.notify_all();
+  if (jt->thread_state() != _thread_in_native) {
+    assert_lock_strong(Threads_lock);
+    JfrSampleMonitor jsm(tl);
+    if (jsm.is_waiting()) {
+      // The thread has already returned from native,
+      // now in _thread_in_vm and is waiting to be sampled.
+      // Convert the native sample request into a java sample request
+      // and let the thread process the ljf on its own.
+      jsm.install_java_sample_request();
+    }
     return false;
   }
 

src/hotspot/share/jfr/periodic/sampling/jfrThreadSampling.cpp

@@ -28,6 +28,7 @@
 #include "code/nmethod.hpp"
 #include "interpreter/interpreter.hpp"
 #include "jfr/jfrEvents.hpp"
+#include "jfr/periodic/sampling/jfrSampleMonitor.hpp"
 #include "jfr/periodic/sampling/jfrSampleRequest.hpp"
 #include "jfr/periodic/sampling/jfrThreadSampling.hpp"
 #include "jfr/recorder/stacktrace/jfrStackTrace.hpp"
@@ -307,24 +308,6 @@ static void drain_enqueued_requests(const JfrTicks& now, JfrThreadLocal* tl, Jav
   assert(!tl->has_enqueued_requests(), "invariant");
 }
 
-class SampleMonitor : public StackObj {
- private:
-  JfrThreadLocal* const _tl;
-  Monitor* const _sample_monitor;
- public:
-  SampleMonitor(JfrThreadLocal* tl) : _tl(tl), _sample_monitor(tl->sample_monitor()) {
-    assert(tl != nullptr, "invariant");
-    assert(_sample_monitor != nullptr, "invariant");
-    _sample_monitor->lock_without_safepoint_check();
-  }
-  ~SampleMonitor() {
-    assert_lock_strong(_sample_monitor);
-    _tl->set_sample_state(NO_SAMPLE);
-    _sample_monitor->notify_all();
-    _sample_monitor->unlock();
-  }
-};
-
 // Only entered by the JfrSampler thread.
 bool JfrThreadSampling::process_native_sample_request(JfrThreadLocal* tl, JavaThread* jt, Thread* sampler_thread) {
   assert(tl != nullptr, "invairant");
@@ -334,7 +317,9 @@ bool JfrThreadSampling::process_native_sample_request(JfrThreadLocal* tl, JavaTh
   assert(tl == jt->jfr_thread_local(), "invariant");
   assert(jt != sampler_thread, "only asynchronous processing of native samples");
   assert(jt->has_last_Java_frame(), "invariant");
-  assert(tl->sample_state() == NATIVE_SAMPLE, "invariant");
+  assert(tl->sample_state() >= NATIVE_SAMPLE, "invariant");
+
+  assert_lock_strong(Threads_lock);
 
   const JfrTicks start_time = JfrTicks::now();
 
@@ -342,7 +327,7 @@ bool JfrThreadSampling::process_native_sample_request(JfrThreadLocal* tl, JavaTh
   traceid sid;
 
   {
-    SampleMonitor sm(tl);
+    JfrSampleMonitor sm(tl);
 
     // Because the thread was in native, it is in a walkable state, because
     // it will hit a safepoint poll on the way back from native. To ensure timely
@@ -384,10 +369,14 @@ void JfrThreadSampling::process_sample_request(JavaThread* jt) {
   for (;;) {
     const int sample_state = tl->sample_state();
     if (sample_state == NATIVE_SAMPLE) {
+      tl->set_sample_state(WAITING_FOR_NATIVE_SAMPLE);
       // Wait until stack trace is processed.
       ml.wait();
     } else if (sample_state == JAVA_SAMPLE) {
       tl->enqueue_request();
+    } else if (sample_state == WAITING_FOR_NATIVE_SAMPLE) {
+      // Handle spurious wakeups. Again wait until stack trace is processed.
+      ml.wait();
     } else {
       // State has been processed.
       break;