8343177: JFR: Remove critical section for thread id assignment

Reviewed-by: dholmes

Author: Markus Grönlund

src/hotspot/share/jfr/jfr.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2019, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2019, 2024, 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
@@ -100,6 +100,10 @@ void Jfr::on_set_current_thread(JavaThread* jt, oop thread) {
   JfrThreadLocal::on_set_current_thread(jt, thread);
 }
 
+void Jfr::initialize_main_thread(JavaThread* jt) {
+  JfrThreadLocal::initialize_main_thread(jt);
+}
+
 void Jfr::on_resolution(const CallInfo& info, TRAPS) {
   JfrResolution::on_runtime_resolution(info, THREAD);
 }

src/hotspot/share/jfr/jfr.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2018, 2024, 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
@@ -71,6 +71,7 @@ class Jfr : AllStatic {
   static bool on_flight_recorder_option(const JavaVMOption** option, char* delimiter);
   static bool on_start_flight_recording_option(const JavaVMOption** option, char* delimiter);
   static void on_backpatching(const Method* callee_method, JavaThread* jt);
+  static void initialize_main_thread(JavaThread* jt);
 };
 
 #endif // SHARE_JFR_JFR_HPP

src/hotspot/share/jfr/recorder/checkpoint/types/jfrType.cpp

@@ -114,7 +114,7 @@ void JfrCheckpointThreadClosure::do_thread(Thread* t) {
 
 void JfrThreadConstantSet::serialize(JfrCheckpointWriter& writer) {
   JfrCheckpointThreadClosure tc(writer);
-  JfrJavaThreadIterator javathreads(false); // include not yet live threads (_thread_new)
+  JfrJavaThreadIterator javathreads;
   while (javathreads.has_next()) {
     tc.do_thread(javathreads.next());
   }

src/hotspot/share/jfr/recorder/jfrRecorder.cpp

@@ -210,7 +210,7 @@ bool JfrRecorder::on_create_vm_2() {
     return true;
   }
   JavaThread* const thread = JavaThread::current();
-  JfrThreadLocal::assign_thread_id(thread, thread->jfr_thread_local());
+  assert(JfrThreadLocal::jvm_thread_id(thread) != 0, "invariant");
 
   if (!JfrOptionSet::initialize(thread)) {
     return false;

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

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2012, 2024, 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
@@ -71,7 +71,6 @@ JfrThreadLocal::JfrThreadLocal() :
   _wallclock_time(os::javaTimeNanos()),
   _stackdepth(0),
   _entering_suspend_flag(0),
-  _critical_section(0),
   _vthread_epoch(0),
   _vthread_excluded(false),
   _jvm_thread_excluded(false),
@@ -100,6 +99,14 @@ const JfrBlobHandle& JfrThreadLocal::thread_blob() const {
   return _thread;
 }
 
+void JfrThreadLocal::initialize_main_thread(JavaThread* jt) {
+  assert(jt != nullptr, "invariant");
+  assert(Thread::is_starting_thread(jt), "invariant");
+  assert(jt->threadObj() == nullptr, "invariant");
+  assert(jt->jfr_thread_local()->_jvm_thread_id == 0, "invariant");
+  jt->jfr_thread_local()->_jvm_thread_id = 1;
+}
+
 static void send_java_thread_start_event(JavaThread* jt) {
   assert(jt != nullptr, "invariant");
   assert(Thread::current() == jt, "invariant");
@@ -394,13 +401,13 @@ traceid JfrThreadLocal::thread_id(const Thread* t) {
   if (is_impersonating(t)) {
     return t->jfr_thread_local()->_thread_id_alias;
   }
-  JfrThreadLocal* const tl = t->jfr_thread_local();
+  const JfrThreadLocal* const tl = t->jfr_thread_local();
   if (!t->is_Java_thread()) {
-    return jvm_thread_id(t, tl);
+    return jvm_thread_id(tl);
   }
   const JavaThread* jt = JavaThread::cast(t);
   if (!is_vthread(jt)) {
-    return jvm_thread_id(t, tl);
+    return jvm_thread_id(tl);
   }
   // virtual thread
   const traceid tid = vthread_id(jt);
@@ -421,19 +428,30 @@ traceid JfrThreadLocal::external_thread_id(const Thread* t) {
   return JfrRecorder::is_recording() ? thread_id(t) : jvm_thread_id(t);
 }
 
-inline traceid load_java_thread_id(const Thread* t) {
+static inline traceid load_java_thread_id(const Thread* t) {
   assert(t != nullptr, "invariant");
   assert(t->is_Java_thread(), "invariant");
   oop threadObj = JavaThread::cast(t)->threadObj();
   return threadObj != nullptr ? AccessThreadTraceId::id(threadObj) : 0;
 }
 
+#ifdef ASSERT
+static bool can_assign(const Thread* t) {
+  assert(t != nullptr, "invariant");
+  if (!t->is_Java_thread()) {
+    return true;
+  }
+  const JavaThread* jt = JavaThread::cast(t);
+  return jt->thread_state() == _thread_new || jt->is_attaching_via_jni();
+}
+#endif
+
 traceid JfrThreadLocal::assign_thread_id(const Thread* t, JfrThreadLocal* tl) {
   assert(t != nullptr, "invariant");
   assert(tl != nullptr, "invariant");
-  JfrSpinlockHelper spinlock(&tl->_critical_section);
   traceid tid = tl->_jvm_thread_id;
   if (tid == 0) {
+    assert(can_assign(t), "invariant");
     if (t->is_Java_thread()) {
       tid = load_java_thread_id(t);
       tl->_jvm_thread_id = tid;
@@ -446,15 +464,14 @@ traceid JfrThreadLocal::assign_thread_id(const Thread* t, JfrThreadLocal* tl) {
   return tid;
 }
 
-traceid JfrThreadLocal::jvm_thread_id(const Thread* t, JfrThreadLocal* tl) {
-  assert(t != nullptr, "invariant");
+traceid JfrThreadLocal::jvm_thread_id(const JfrThreadLocal* tl) {
   assert(tl != nullptr, "invariant");
-  return tl->_jvm_thread_id != 0 ? tl->_jvm_thread_id : JfrThreadLocal::assign_thread_id(t, tl);
+  return tl->_jvm_thread_id;
 }
 
 traceid JfrThreadLocal::jvm_thread_id(const Thread* t) {
   assert(t != nullptr, "invariant");
-  return jvm_thread_id(t, t->jfr_thread_local());
+  return jvm_thread_id(t->jfr_thread_local());
 }
 
 bool JfrThreadLocal::is_vthread(const JavaThread* jt) {

src/hotspot/share/jfr/support/jfrThreadLocal.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2012, 2024, 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,7 +38,6 @@ class JfrThreadLocal {
   friend class Jfr;
   friend class JfrIntrinsicSupport;
   friend class JfrJavaSupport;
-  friend class JfrRecorder;
   friend class JVMCIVMStructs;
  private:
   jobject _java_event_writer;
@@ -65,7 +64,6 @@ class JfrThreadLocal {
   jlong _wallclock_time;
   mutable u4 _stackdepth;
   volatile jint _entering_suspend_flag;
-  mutable volatile int _critical_section;
   u2 _vthread_epoch;
   bool _vthread_excluded;
   bool _jvm_thread_excluded;
@@ -78,11 +76,13 @@ class JfrThreadLocal {
   JfrStackFrame* install_stackframes() const;
   void release(Thread* t);
   static void release(JfrThreadLocal* tl, Thread* t);
+  static void initialize_main_thread(JavaThread* jt);
 
   static void set(bool* excluded_field, bool state);
   static traceid assign_thread_id(const Thread* t, JfrThreadLocal* tl);
   static traceid vthread_id(const Thread* t);
   static void set_vthread_epoch(const JavaThread* jt, traceid id, u2 epoch);
+  static traceid jvm_thread_id(const JfrThreadLocal* tl);
   bool is_vthread_excluded() const;
   static void exclude_vthread(const JavaThread* jt);
   static void include_vthread(const JavaThread* jt);
@@ -175,7 +175,6 @@ class JfrThreadLocal {
 
   // Non-volatile thread id, for Java carrier threads and non-java threads.
   static traceid jvm_thread_id(const Thread* t);
-  static traceid jvm_thread_id(const Thread* t, JfrThreadLocal* tl);
 
   // To impersonate is to temporarily masquerade as another thread.
   // For example, when writing an event that should be attributed to some other thread.

src/hotspot/share/prims/jni.cpp

@@ -3849,6 +3849,9 @@ static jint attach_current_thread(JavaVM *vm, void **penv, void *_args, bool dae
     return JNI_ERR;
   }
 
+  // Want this inside 'attaching via jni'.
+  JFR_ONLY(Jfr::on_thread_start(thread);)
+
   // mark the thread as no longer attaching
   // this uses a fence to push the change through so we don't have
   // to regrab the threads_lock
@@ -3863,8 +3866,6 @@ static jint attach_current_thread(JavaVM *vm, void **penv, void *_args, bool dae
     JvmtiExport::post_thread_start(thread);
   }
 
-  JFR_ONLY(Jfr::on_thread_start(thread);)
-
   *(JNIEnv**)penv = thread->jni_environment();
 
   // Now leaving the VM, so change thread_state. This is normally automatically taken care

src/hotspot/share/runtime/thread.cpp

@@ -62,8 +62,6 @@ THREAD_LOCAL Thread* Thread::_thr_current = nullptr;
 // Base class for all threads: VMThread, WatcherThread, ConcurrentMarkSweepThread,
 // JavaThread
 
-DEBUG_ONLY(Thread* Thread::_starting_thread = nullptr;)
-
 Thread::Thread(MemTag mem_tag) {
 
   DEBUG_ONLY(_run_state = PRE_CALL_RUN;)
@@ -538,14 +536,22 @@ void Thread::print_owned_locks_on(outputStream* st) const {
     }
   }
 }
+
+Thread* Thread::_starting_thread = nullptr;
+
+bool Thread::is_starting_thread(const Thread* t) {
+  assert(_starting_thread != nullptr, "invariant");
+  return t == _starting_thread;
+}
 #endif // ASSERT
 
-bool Thread::set_as_starting_thread() {
+bool Thread::set_as_starting_thread(JavaThread* jt) {
+  assert(jt != nullptr, "invariant");
   assert(_starting_thread == nullptr, "already initialized: "
          "_starting_thread=" INTPTR_FORMAT, p2i(_starting_thread));
-  // NOTE: this must be called inside the main thread.
-  DEBUG_ONLY(_starting_thread = this;)
-  return os::create_main_thread(JavaThread::cast(this));
+  // NOTE: this must be called from Threads::create_vm().
+  DEBUG_ONLY(_starting_thread = jt;)
+  return os::create_main_thread(jt);
 }
 
 // Ad-hoc mutual exclusion primitives: SpinLock

src/hotspot/share/runtime/thread.hpp

@@ -163,9 +163,6 @@ class Thread: public ThreadShadow {
   // const char* _exception_file;                   // file information for exception (debugging only)
   // int         _exception_line;                   // line information for exception (debugging only)
  protected:
-
-  DEBUG_ONLY(static Thread* _starting_thread;)
-
   // JavaThread lifecycle support:
   friend class SafeThreadsListPtr;  // for _threads_list_ptr, cmpxchg_threads_hazard_ptr(), {dec_,inc_,}nested_threads_hazard_ptr_cnt(), {g,s}et_threads_hazard_ptr(), inc_nested_handle_cnt(), tag_hazard_ptr() access
   friend class ScanHazardPtrGatherProtectedThreadsClosure;  // for cmpxchg_threads_hazard_ptr(), get_threads_hazard_ptr(), is_hazard_ptr_tagged() access
@@ -211,12 +208,15 @@ class Thread: public ThreadShadow {
   static bool is_JavaThread_protected_by_TLH(const JavaThread* target);
 
  private:
+  DEBUG_ONLY(static Thread* _starting_thread;)
   DEBUG_ONLY(bool _suspendible_thread;)
   DEBUG_ONLY(bool _indirectly_suspendible_thread;)
   DEBUG_ONLY(bool _indirectly_safepoint_thread;)
 
  public:
 #ifdef ASSERT
+  static bool is_starting_thread(const Thread* t);
+
   void set_suspendible_thread()   { _suspendible_thread = true; }
   void clear_suspendible_thread() { _suspendible_thread = false; }
   bool is_suspendible_thread()    { return _suspendible_thread; }
@@ -500,9 +500,9 @@ class Thread: public ThreadShadow {
     return is_in_stack_range_incl(adr, os::current_stack_pointer());
   }
 
-  // Sets this thread as starting thread. Returns failure if thread
+  // Sets the argument thread as starting thread. Returns failure if thread
   // creation fails due to lack of memory, too many threads etc.
-  bool set_as_starting_thread();
+  static bool set_as_starting_thread(JavaThread* jt);
 
 protected:
   // OS data associated with the thread

src/hotspot/share/runtime/threads.cpp

@@ -166,6 +166,9 @@ static void create_initial_thread(Handle thread_group, JavaThread* thread,
                           string,
                           CHECK);
 
+  JFR_ONLY(assert(JFR_JVM_THREAD_ID(thread) == static_cast<traceid>(java_lang_Thread::thread_id(thread_oop())),
+             "initial tid mismatch");)
+
   // Set thread status to running since main thread has
   // been started and running.
   java_lang_Thread::set_thread_status(thread_oop(),
@@ -532,14 +535,16 @@ jint Threads::create_vm(JavaVMInitArgs* args, bool* canTryAgain) {
   main_thread->set_active_handles(JNIHandleBlock::allocate_block());
   MACOS_AARCH64_ONLY(main_thread->init_wx());
 
-  if (!main_thread->set_as_starting_thread()) {
+  if (!Thread::set_as_starting_thread(main_thread)) {
     vm_shutdown_during_initialization(
                                       "Failed necessary internal allocation. Out of swap space");
     main_thread->smr_delete();
     *canTryAgain = false; // don't let caller call JNI_CreateJavaVM again
     return JNI_ENOMEM;
   }
 
+  JFR_ONLY(Jfr::initialize_main_thread(main_thread);)
+
   // Enable guard page *after* os::create_main_thread(), otherwise it would
   // crash Linux VM, see notes in os_linux.cpp.
   main_thread->stack_overflow_state()->create_stack_guard_pages();