8352066: JVM.commit() and JVM.flush() exhibit race conditions against JFR epochs

Reviewed-by: egahlin

Author: Markus Grönlund

src/hotspot/share/jfr/jni/jfrJniMethod.cpp

@@ -300,13 +300,13 @@ JVM_ENTRY_NO_ENV(jobject, jfr_new_event_writer(JNIEnv* env, jclass jvm))
   return JfrJavaEventWriter::new_event_writer(thread);
 JVM_END
 
-NO_TRANSITION(void, jfr_event_writer_flush(JNIEnv* env, jclass jvm, jobject writer, jint used_size, jint requested_size))
-  JfrJavaEventWriter::flush(writer, used_size, requested_size, JavaThread::current());
-NO_TRANSITION_END
+JVM_ENTRY_NO_ENV(void, jfr_event_writer_flush(JNIEnv* env, jclass jvm, jobject writer, jint used_size, jint requested_size))
+  JfrJavaEventWriter::flush(writer, used_size, requested_size, thread);
+JVM_END
 
-NO_TRANSITION(jlong, jfr_commit(JNIEnv* env, jclass jvm, jlong next_position))
-  return JfrJavaEventWriter::commit(next_position);
-NO_TRANSITION_END
+JVM_ENTRY_NO_ENV(jlong, jfr_commit(JNIEnv* env, jclass jvm, jlong next_position))
+  return JfrJavaEventWriter::commit(next_position, thread);
+JVM_END
 
 JVM_ENTRY_NO_ENV(void, jfr_flush(JNIEnv* env, jclass jvm))
   JfrRepository::flush(thread);

src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointManager.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 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

src/hotspot/share/jfr/recorder/service/jfrRecorderService.cpp

@@ -486,6 +486,7 @@ void JfrRecorderService::safepoint_clear() {
   assert(SafepointSynchronize::is_at_safepoint(), "invariant");
   _checkpoint_manager.begin_epoch_shift();
   _storage.clear();
+  _checkpoint_manager.notify_threads();
   _chunkwriter.set_time_stamp();
   JfrDeprecationManager::on_safepoint_clear();
   JfrStackTraceRepository::clear();
@@ -650,9 +651,7 @@ size_t JfrRecorderService::flush() {
   if (_string_pool.is_modified()) {
     total_elements += flush_stringpool(_string_pool, _chunkwriter);
   }
-  if (_stack_trace_repository.is_modified()) {
-    total_elements += flush_stacktrace(_stack_trace_repository, _chunkwriter);
-  }
+  total_elements += flush_stacktrace(_stack_trace_repository, _chunkwriter);
   return flush_typeset(_checkpoint_manager, _chunkwriter) + total_elements;
 }
 

src/hotspot/share/jfr/recorder/stacktrace/jfrStackTraceRepository.cpp

@@ -92,13 +92,9 @@ void JfrStackTraceRepository::destroy() {
   _leak_profiler_instance = nullptr;
 }
 
-bool JfrStackTraceRepository::is_modified() const {
-  return _last_entries != _entries;
-}
-
 size_t JfrStackTraceRepository::write(JfrChunkWriter& sw, bool clear) {
   MutexLocker lock(JfrStacktrace_lock, Mutex::_no_safepoint_check_flag);
-  if (_entries == 0) {
+  if ((_entries == _last_entries) && !clear) {
     return 0;
   }
   int count = 0;

src/hotspot/share/jfr/recorder/stacktrace/jfrStackTraceRepository.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2011, 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
@@ -56,7 +56,6 @@ class JfrStackTraceRepository : public JfrCHeapObj {
   static void destroy();
   bool initialize();
 
-  bool is_modified() const;
   static size_t clear();
   static size_t clear(JfrStackTraceRepository& repo);
   size_t write(JfrChunkWriter& cw, bool clear);

src/hotspot/share/jfr/support/jfrIntrinsics.cpp

@@ -37,35 +37,14 @@ static void assert_precondition(JavaThread* jt) {
   DEBUG_ONLY(JfrJavaSupport::check_java_thread_in_java(jt);)
   assert(jt->has_last_Java_frame(), "invariant");
 }
-
-static void assert_epoch_identity(JavaThread* jt, u2 current_epoch) {
-  assert_precondition(jt);
-  // Verify the epoch updates got written through also to the vthread object.
-  const u2 epoch_raw = ThreadIdAccess::epoch(jt->vthread());
-  const bool excluded = epoch_raw & excluded_bit;
-  assert(!excluded, "invariant");
-  assert(!JfrThreadLocal::is_excluded(jt), "invariant");
-  const u2 vthread_epoch = epoch_raw & epoch_mask;
-  assert(vthread_epoch == current_epoch, "invariant");
-}
-#endif // ASSERT
+#endif
 
 void* JfrIntrinsicSupport::write_checkpoint(JavaThread* jt) {
   DEBUG_ONLY(assert_precondition(jt);)
   assert(JfrThreadLocal::is_vthread(jt), "invariant");
-  const u2 vthread_thread_local_epoch = JfrThreadLocal::vthread_epoch(jt);
-  const u2 current_epoch = ThreadIdAccess::current_epoch();
-  MACOS_AARCH64_ONLY(ThreadWXEnable __wx(WXWrite, jt));
-  if (vthread_thread_local_epoch == current_epoch) {
-    // After the epoch test in the intrinsic, the thread sampler interleaved
-    // and suspended the thread. As part of taking a sample, it updated
-    // the vthread object and the thread local "for us". We are good.
-    DEBUG_ONLY(assert_epoch_identity(jt, current_epoch);)
-    ThreadInVMfromJava transition(jt);
-    return JfrJavaEventWriter::event_writer(jt);
-  }
   const traceid vthread_tid = JfrThreadLocal::vthread_id(jt);
-  // Transition before reading the epoch generation anew, now as _thread_in_vm. Can safepoint here.
+  // Transition before reading the epoch generation, now as _thread_in_vm.
+  MACOS_AARCH64_ONLY(ThreadWXEnable __wx(WXWrite, jt));
   ThreadInVMfromJava transition(jt);
   JfrThreadLocal::set_vthread_epoch(jt, vthread_tid, ThreadIdAccess::current_epoch());
   return JfrJavaEventWriter::event_writer(jt);
@@ -74,12 +53,11 @@ void* JfrIntrinsicSupport::write_checkpoint(JavaThread* jt) {
 void* JfrIntrinsicSupport::return_lease(JavaThread* jt) {
   DEBUG_ONLY(assert_precondition(jt);)
   MACOS_AARCH64_ONLY(ThreadWXEnable __wx(WXWrite, jt));
-  ThreadStateTransition::transition_from_java(jt, _thread_in_native);
+  ThreadInVMfromJava transition(jt);
   assert(jt->jfr_thread_local()->has_java_event_writer(), "invariant");
   assert(jt->jfr_thread_local()->shelved_buffer() != nullptr, "invariant");
   JfrJavaEventWriter::flush(jt->jfr_thread_local()->java_event_writer(), 0, 0, jt);
   assert(jt->jfr_thread_local()->shelved_buffer() == nullptr, "invariant");
-  ThreadStateTransition::transition_from_native(jt, _thread_in_Java);
   return nullptr;
 }
 

src/hotspot/share/jfr/writers/jfrJavaEventWriter.cpp

@@ -118,7 +118,7 @@ bool JfrJavaEventWriter::initialize() {
 }
 
 void JfrJavaEventWriter::flush(jobject writer, jint used, jint requested, JavaThread* jt) {
-  DEBUG_ONLY(JfrJavaSupport::check_java_thread_in_native(jt));
+  DEBUG_ONLY(JfrJavaSupport::check_java_thread_in_vm(jt));
   assert(writer != nullptr, "invariant");
   JfrBuffer* const current = jt->jfr_thread_local()->java_buffer();
   assert(current != nullptr, "invariant");
@@ -130,9 +130,6 @@ void JfrJavaEventWriter::flush(jobject writer, jint used, jint requested, JavaTh
   const bool is_valid = buffer->free_size() >= (size_t)(used + requested);
   u1* const new_current_position = is_valid ? buffer->pos() + used : buffer->pos();
   assert(start_pos_offset != invalid_offset, "invariant");
-  // can safepoint here
-  MACOS_AARCH64_ONLY(ThreadWXEnable __wx(WXWrite, jt));
-  ThreadInVMfromNative transition(jt);
   oop const w = JNIHandles::resolve_non_null(writer);
   assert(w != nullptr, "invariant");
   w->long_field_put(start_pos_offset, (jlong)buffer->pos());
@@ -147,11 +144,10 @@ void JfrJavaEventWriter::flush(jobject writer, jint used, jint requested, JavaTh
   }
 }
 
-jlong JfrJavaEventWriter::commit(jlong next_position) {
+jlong JfrJavaEventWriter::commit(jlong next_position, JavaThread* jt) {
   assert(next_position != 0, "invariant");
-  JavaThread* const jt = JavaThread::current();
   assert(jt != nullptr, "invariant");
-  DEBUG_ONLY(JfrJavaSupport::check_java_thread_in_native(jt));
+  DEBUG_ONLY(JfrJavaSupport::check_java_thread_in_vm(jt));
   JfrThreadLocal* const tl = jt->jfr_thread_local();
   assert(tl != nullptr, "invariant");
   assert(tl->has_java_event_writer(), "invariant");
@@ -167,12 +163,11 @@ jlong JfrJavaEventWriter::commit(jlong next_position) {
   }
   // set_pos() has release semantics
   current->set_pos(next);
-  if (!current->lease()) {
-    return next_position;
+  if (current->lease()) {
+    flush(tl->java_event_writer(), 0, 0, jt);
+    return 0; // signals that the buffer lease was returned.
   }
-  assert(current->lease(), "invariant");
-  flush(tl->java_event_writer(), 0, 0, jt);
-  return 0; // signals that the buffer lease was returned.
+  return next_position;
 }
 
 class JfrJavaEventWriterNotificationClosure : public ThreadClosure {

src/hotspot/share/jfr/writers/jfrJavaEventWriter.hpp

@@ -50,7 +50,7 @@ class JfrJavaEventWriter : AllStatic {
   static jobject event_writer(JavaThread* t);
   static jobject new_event_writer(TRAPS);
   static void flush(jobject writer, jint used, jint requested, JavaThread* jt);
-  static jlong commit(jlong next_position);
+  static jlong commit(jlong next_position, JavaThread* jt);
 };
 
 #endif // SHARE_JFR_WRITERS_JFRJAVAEVENTWRITER_HPP