8210024: JFR calls virtual is_Java_thread from ~Thread()

Reviewed-by: kbarrett, dholmes, dcubed, egahlin

src/share/vm/jfr/jfr.cpp

@@ -63,13 +63,17 @@ void Jfr::on_unloading_classes() {
   }
 }
 
-void Jfr::on_thread_exit(JavaThread* thread) {
-  JfrThreadLocal::on_exit(thread);
+void Jfr::on_thread_start(Thread* t) {
+  JfrThreadLocal::on_start(t);
 }
 
-void Jfr::on_thread_destruct(Thread* thread) {
-  if (JfrRecorder::is_created()) {
-    JfrThreadLocal::on_destruct(thread);
+void Jfr::on_thread_exit(Thread* t) {
+  JfrThreadLocal::on_exit(t);
+}
+
+void Jfr::on_java_thread_dismantle(JavaThread* jt) {
+  if (JfrRecorder::is_recording()) {
+    JfrCheckpointManager::write_thread_checkpoint(jt);
   }
 }
 

src/share/vm/jfr/jfr.hpp

@@ -46,8 +46,9 @@ class Jfr : AllStatic {
   static void on_vm_init();
   static void on_vm_start();
   static void on_unloading_classes();
-  static void on_thread_exit(JavaThread* thread);
-  static void on_thread_destruct(Thread* thread);
+  static void on_thread_start(Thread* thread);
+  static void on_thread_exit(Thread* thread);
+  static void on_java_thread_dismantle(JavaThread* jt);
   static void on_vm_shutdown(bool exception_handler = false);
   static bool on_flight_recorder_option(const JavaVMOption** option, char* delimiter);
   static bool on_start_flight_recording_option(const JavaVMOption** option, char* delimiter);

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

@@ -23,6 +23,7 @@
  */
 
 #include "precompiled.hpp"
+#include "jfr/jfrEvents.hpp"
 #include "jfr/jni/jfrJavaSupport.hpp"
 #include "jfr/periodic/jfrThreadCPULoadEvent.hpp"
 #include "jfr/recorder/jfrRecorder.hpp"
@@ -35,6 +36,7 @@
 #include "memory/allocation.inline.hpp"
 #include "runtime/os.hpp"
 #include "runtime/thread.inline.hpp"
+#include "utilities/sizes.hpp"
 
 /* This data structure is per thread and only accessed by the thread itself, no locking required */
 JfrThreadLocal::JfrThreadLocal() :
@@ -73,53 +75,76 @@ const JfrCheckpointBlobHandle& JfrThreadLocal::thread_checkpoint() const {
   return _thread_cp;
 }
 
-void JfrThreadLocal::set_dead() {
-  assert(!is_dead(), "invariant");
-  _dead = true;
+static void send_java_thread_start_event(JavaThread* jt) {
+  EventThreadStart event;
+  event.set_thread(jt->jfr_thread_local()->thread_id());
+  event.commit();
 }
 
-void JfrThreadLocal::on_exit(JavaThread* thread) {
+void JfrThreadLocal::on_start(Thread* t) {
+  assert(t != NULL, "invariant");
+  assert(Thread::current() == t, "invariant");
   if (JfrRecorder::is_recording()) {
-    JfrCheckpointManager::write_thread_checkpoint(thread);
-    JfrThreadCPULoadEvent::send_event_for_thread(thread);
+    if (t->is_Java_thread()) {
+      send_java_thread_start_event((JavaThread*)t);
+    }
   }
-  thread->jfr_thread_local()->set_dead();
 }
 
-void JfrThreadLocal::on_destruct(Thread* thread) {
-  JfrThreadLocal* const tl = thread->jfr_thread_local();
+static void send_java_thread_end_events(traceid id, JavaThread* jt) {
+  assert(jt != NULL, "invariant");
+  assert(Thread::current() == jt, "invariant");
+  assert(jt->jfr_thread_local()->trace_id() == id, "invariant");
+  EventThreadEnd event;
+  event.set_thread(id);
+  event.commit();
+  JfrThreadCPULoadEvent::send_event_for_thread(jt);
+}
+
+void JfrThreadLocal::release(JfrThreadLocal* tl, Thread* t) {
+  assert(tl != NULL, "invariant");
+  assert(t != NULL, "invariant");
+  assert(Thread::current() == t, "invariant");
+  assert(!tl->is_dead(), "invariant");
+  assert(tl->shelved_buffer() == NULL, "invariant");
   if (tl->has_native_buffer()) {
-    release(tl->native_buffer(), thread);
+    JfrStorage::release_thread_local(tl->native_buffer(), t);
   }
   if (tl->has_java_buffer()) {
-    release(tl->java_buffer(), thread);
+    JfrStorage::release_thread_local(tl->java_buffer(), t);
   }
-  assert(tl->shelved_buffer() == NULL, "invariant");
-  if (thread->jfr_thread_local()->has_java_event_writer()) {
+  if (tl->has_java_event_writer()) {
+    assert(t->is_Java_thread(), "invariant");
     JfrJavaSupport::destroy_global_jni_handle(tl->java_event_writer());
   }
-  destroy_stackframes(thread);
-}
-
-JfrBuffer* JfrThreadLocal::acquire(Thread* thread, size_t size) {
-  return JfrStorage::acquire_thread_local(thread, size);
+  if (tl->_stackframes != NULL) {
+    FREE_C_HEAP_ARRAY(JfrStackFrame, tl->_stackframes, mtTracing);
+  }
+  tl->_dead = true;
 }
 
-void JfrThreadLocal::release(JfrBuffer* buffer, Thread* thread) {
-  assert(buffer != NULL, "invariant");
-  JfrStorage::release_thread_local(buffer, thread);
+void JfrThreadLocal::on_exit(Thread* t) {
+  assert(t != NULL, "invariant");
+  JfrThreadLocal * const tl = t->jfr_thread_local();
+  assert(!tl->is_dead(), "invariant");
+  if (JfrRecorder::is_recording()) {
+    if (t->is_Java_thread()) {
+      send_java_thread_end_events(tl->thread_id(), (JavaThread*)t);
+    }
+  }
+  release(tl, Thread::current()); // because it could be that Thread::current() != t
 }
 
 JfrBuffer* JfrThreadLocal::install_native_buffer() const {
   assert(!has_native_buffer(), "invariant");
-  _native_buffer = acquire(Thread::current());
+  _native_buffer = JfrStorage::acquire_thread_local(Thread::current());
   return _native_buffer;
 }
 
 JfrBuffer* JfrThreadLocal::install_java_buffer() const {
   assert(!has_java_buffer(), "invariant");
   assert(!has_java_event_writer(), "invariant");
-  _java_buffer = acquire(Thread::current());
+  _java_buffer = JfrStorage::acquire_thread_local(Thread::current());
   return _java_buffer;
 }
 
@@ -131,11 +156,10 @@ JfrStackFrame* JfrThreadLocal::install_stackframes() const {
   return _stackframes;
 }
 
-void JfrThreadLocal::destroy_stackframes(Thread* thread) {
-  assert(thread != NULL, "invariant");
-  JfrStackFrame* frames = thread->jfr_thread_local()->stackframes();
-  if (frames != NULL) {
-    FREE_C_HEAP_ARRAY(JfrStackFrame, frames, mtTracing);
-    thread->jfr_thread_local()->set_stackframes(NULL);
-  }
+ByteSize JfrThreadLocal::trace_id_offset() {
+  return in_ByteSize(offset_of(JfrThreadLocal, _trace_id));
+}
+
+ByteSize JfrThreadLocal::java_event_writer_offset() {
+  return in_ByteSize(offset_of(JfrThreadLocal, _java_event_writer));
 }

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

@@ -27,11 +27,11 @@
 
 #include "jfr/recorder/checkpoint/jfrCheckpointBlob.hpp"
 #include "jfr/utilities/jfrTypes.hpp"
-#include "utilities/sizes.hpp"
 
 class JavaThread;
 class JfrBuffer;
 class JfrStackFrame;
+class Thread;
 
 class JfrThreadLocal {
  private:
@@ -56,7 +56,7 @@ class JfrThreadLocal {
   JfrBuffer* install_java_buffer() const;
   JfrStackFrame* install_stackframes() const;
 
-  void set_dead();
+  static void release(JfrThreadLocal* tl, Thread* t);
 
  public:
   JfrThreadLocal();
@@ -213,20 +213,12 @@ class JfrThreadLocal {
   void set_thread_checkpoint(const JfrCheckpointBlobHandle& handle);
   const JfrCheckpointBlobHandle& thread_checkpoint() const;
 
-  static JfrBuffer* acquire(Thread* t, size_t size = 0);
-  static void release(JfrBuffer* buffer, Thread* t);
-  static void destroy_stackframes(Thread* t);
-  static void on_exit(JavaThread* t);
-  static void on_destruct(Thread* t);
+  static void on_start(Thread* t);
+  static void on_exit(Thread* t);
 
   // Code generation
-  static ByteSize trace_id_offset() {
-    return in_ByteSize(offset_of(JfrThreadLocal, _trace_id));
-  }
-
-  static ByteSize java_event_writer_offset() {
-    return in_ByteSize(offset_of(JfrThreadLocal, _java_event_writer));
-  }
+  static ByteSize trace_id_offset();
+  static ByteSize java_event_writer_offset();
 };
 
 #endif // SHARE_VM_JFR_SUPPORT_JFRTHREADLOCAL_HPP

src/share/vm/runtime/thread.cpp

@@ -33,7 +33,6 @@
 #include "interpreter/linkResolver.hpp"
 #include "interpreter/oopMapCache.hpp"
 #include "jfr/jfrEvents.hpp"
-#include "jfr/support/jfrThreadId.hpp"
 #include "jvmtifiles/jvmtiEnv.hpp"
 #include "memory/gcLocker.inline.hpp"
 #include "memory/metaspaceShared.hpp"
@@ -345,8 +344,6 @@ Thread::~Thread() {
   // Reclaim the objectmonitors from the omFreeList of the moribund thread.
   ObjectSynchronizer::omFlush (this) ;
 
-  JFR_ONLY(Jfr::on_thread_destruct(this);)
-
   // stack_base can be NULL if the thread is never started or exited before
   // record_stack_base_and_size called. Although, we would like to ensure
   // that all started threads do call record_stack_base_and_size(), there is
@@ -1215,6 +1212,7 @@ NamedThread::NamedThread() : Thread() {
 }
 
 NamedThread::~NamedThread() {
+  JFR_ONLY(Jfr::on_thread_exit(this);)
   if (_name != NULL) {
     FREE_C_HEAP_ARRAY(char, _name, mtThread);
     _name = NULL;
@@ -1672,11 +1670,7 @@ void JavaThread::run() {
     JvmtiExport::post_thread_start(this);
   }
 
-  EventThreadStart event;
-  if (event.should_commit()) {
-    event.set_thread(JFR_THREAD_ID(this));
-     event.commit();
-  }
+  JFR_ONLY(Jfr::on_thread_start(this);)
 
   // We call another function to do the rest so we are sure that the stack addresses used
   // from there will be lower than the stack base just computed
@@ -1803,17 +1797,7 @@ void JavaThread::exit(bool destroy_vm, ExitType exit_type) {
         }
       }
     }
-
-    // Called before the java thread exit since we want to read info
-    // from java_lang_Thread object
-    EventThreadEnd event;
-    if (event.should_commit()) {
-      event.set_thread(JFR_THREAD_ID(this));
-      event.commit();
-    }
-
-    // Call after last event on thread
-    JFR_ONLY(Jfr::on_thread_exit(this);)
+    JFR_ONLY(Jfr::on_java_thread_dismantle(this);)
 
     // Call Thread.exit(). We try 3 times in case we got another Thread.stop during
     // the execution of the method. If that is not enough, then we don't really care. Thread.stop
@@ -1890,6 +1874,7 @@ void JavaThread::exit(bool destroy_vm, ExitType exit_type) {
   // These things needs to be done while we are still a Java Thread. Make sure that thread
   // is in a consistent state, in case GC happens
   assert(_privileged_stack_top == NULL, "must be NULL when we get here");
+  JFR_ONLY(Jfr::on_thread_exit(this);)
 
   if (active_handles() != NULL) {
     JNIHandleBlock* block = active_handles();