8338417: Explicitly pin a virtual thread before acquiring the JFR string pool monitor

Reviewed-by: alanb, egahlin, dholmes

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

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 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
@@ -36,6 +36,7 @@
 #include "oops/instanceKlass.hpp"
 #include "oops/oop.inline.hpp"
 #include "runtime/fieldDescriptor.inline.hpp"
+#include "runtime/globals.hpp"
 #include "runtime/handles.inline.hpp"
 #include "runtime/interfaceSupport.inline.hpp"
 #include "runtime/jniHandles.inline.hpp"
@@ -49,6 +50,7 @@ static int max_pos_offset = invalid_offset;
 static int excluded_offset = invalid_offset;
 static int thread_id_offset = invalid_offset;
 static int valid_offset = invalid_offset;
+static int pin_offset = invalid_offset;
 
 static bool setup_event_writer_offsets(TRAPS) {
   const char class_name[] = "jdk/jfr/internal/event/EventWriter";
@@ -98,6 +100,13 @@ static bool setup_event_writer_offsets(TRAPS) {
   assert(invalid_offset == valid_offset, "invariant");
   JfrJavaSupport::compute_field_offset(valid_offset, klass, valid_sym, vmSymbols::bool_signature());
   assert(valid_offset != invalid_offset, "invariant");
+
+  const char pin_name[] = "pinVirtualThread";
+  Symbol* const pin_sym = SymbolTable::new_symbol(valid_name);
+  assert(pin_sym != nullptr, "invariant");
+  assert(invalid_offset == pin_offset, "invariant");
+  JfrJavaSupport::compute_field_offset(pin_offset, klass, pin_sym, vmSymbols::bool_signature());
+  assert(pin_offset != invalid_offset, "invariant");
   return true;
 }
 
@@ -219,13 +228,18 @@ void JfrJavaEventWriter::notify(JavaThread* jt) {
   }
 }
 
+static inline bool pin_virtual(const JavaThread* jt) {
+  assert(jt != nullptr, "invariant");
+  return JfrThreadLocal::is_vthread(jt) && VMContinuations;
+}
+
 static jobject create_new_event_writer(JfrBuffer* buffer, JfrThreadLocal* tl, TRAPS) {
   assert(buffer != nullptr, "invariant");
   DEBUG_ONLY(JfrJavaSupport::check_java_thread_in_vm(THREAD));
   HandleMark hm(THREAD);
   static const char klass[] = "jdk/jfr/internal/event/EventWriter";
   static const char method[] = "<init>";
-  static const char signature[] = "(JJJZZ)V";
+  static const char signature[] = "(JJJZZZ)V";
   JavaValue result(T_OBJECT);
   JfrJavaArguments args(&result, klass, method, signature, CHECK_NULL);
 
@@ -234,6 +248,7 @@ static jobject create_new_event_writer(JfrBuffer* buffer, JfrThreadLocal* tl, TR
   args.push_long((jlong)buffer->end());
   args.push_long((jlong)JfrThreadLocal::thread_id(THREAD));
   args.push_int((jint)JNI_TRUE); // valid
+  args.push_int(pin_virtual(THREAD) ? (jint)JNI_TRUE : (jint)JNI_FALSE);
   args.push_int(tl->is_excluded() ? (jint)JNI_TRUE : (jint)JNI_FALSE); // excluded
   JfrJavaSupport::new_object_global_ref(&args, CHECK_NULL);
   return result.get_jobject();
@@ -249,9 +264,12 @@ jobject JfrJavaEventWriter::event_writer(JavaThread* jt) {
     const jlong event_writer_tid = writer->long_field(thread_id_offset);
     const jlong current_tid = static_cast<jlong>(JfrThreadLocal::thread_id(jt));
     if (event_writer_tid != current_tid) {
+      writer->long_field_put(thread_id_offset, current_tid);
       const bool excluded = tl->is_excluded();
       writer->bool_field_put(excluded_offset, excluded);
-      writer->long_field_put(thread_id_offset, current_tid);
+      if (!excluded) {
+        writer->bool_field_put(pin_offset, pin_virtual(jt));
+      }
     }
   }
   return h_writer;

src/hotspot/share/opto/library_call.cpp

@@ -3233,10 +3233,12 @@ bool LibraryCallKit::inline_native_jvm_commit() {
  * oop threadObj = Thread::threadObj();
  * oop vthread = java_lang_Thread::vthread(threadObj);
  * traceid tid;
+ * bool pinVirtualThread;
  * bool excluded;
  * if (vthread != threadObj) {  // i.e. current thread is virtual
  *   tid = java_lang_Thread::tid(vthread);
  *   u2 vthread_epoch_raw = java_lang_Thread::jfr_epoch(vthread);
+ *   pinVirtualThread = VMContinuations;
  *   excluded = vthread_epoch_raw & excluded_mask;
  *   if (!excluded) {
  *     traceid current_epoch = JfrTraceIdEpoch::current_generation();
@@ -3248,13 +3250,15 @@ bool LibraryCallKit::inline_native_jvm_commit() {
  * } else {
  *   tid = java_lang_Thread::tid(threadObj);
  *   u2 thread_epoch_raw = java_lang_Thread::jfr_epoch(threadObj);
+ *   pinVirtualThread = false;
  *   excluded = thread_epoch_raw & excluded_mask;
  * }
  * oop event_writer = JNIHandles::resolve_non_null(h_event_writer);
  * traceid tid_in_event_writer = getField(event_writer, "threadID");
  * if (tid_in_event_writer != tid) {
- *   setField(event_writer, "threadID", tid);
+ *   setField(event_writer, "pinVirtualThread", pinVirtualThread);
  *   setField(event_writer, "excluded", excluded);
+ *   setField(event_writer, "threadID", tid);
  * }
  * return event_writer
  */
@@ -3325,6 +3329,10 @@ bool LibraryCallKit::inline_native_getEventWriter() {
   // Load the tid field from the vthread object.
   Node* vthread_tid = load_field_from_object(vthread, "tid", "J");
 
+  // Continuation support determines if a virtual thread should be pinned.
+  Node* global_addr = makecon(TypeRawPtr::make((address)&VMContinuations));
+  Node* continuation_support = make_load(control(), global_addr, TypeInt::BOOL, T_BOOLEAN, MemNode::unordered);
+
   // Load the raw epoch value from the vthread.
   Node* vthread_epoch_offset = basic_plus_adr(vthread, java_lang_Thread::jfr_epoch_offset());
   Node* vthread_epoch_raw = access_load_at(vthread, vthread_epoch_offset, TypeRawPtr::BOTTOM, TypeInt::CHAR, T_CHAR,
@@ -3415,6 +3423,8 @@ bool LibraryCallKit::inline_native_getEventWriter() {
   record_for_igvn(tid);
   PhiNode* exclusion = new PhiNode(vthread_compare_rgn, TypeInt::BOOL);
   record_for_igvn(exclusion);
+  PhiNode* pinVirtualThread = new PhiNode(vthread_compare_rgn, TypeInt::BOOL);
+  record_for_igvn(pinVirtualThread);
 
   // Update control and phi nodes.
   vthread_compare_rgn->init_req(_true_path, _gvn.transform(exclude_compare_rgn));
@@ -3427,6 +3437,8 @@ bool LibraryCallKit::inline_native_getEventWriter() {
   tid->init_req(_false_path, _gvn.transform(thread_obj_tid));
   exclusion->init_req(_true_path, _gvn.transform(vthread_is_excluded));
   exclusion->init_req(_false_path, _gvn.transform(threadObj_is_excluded));
+  pinVirtualThread->init_req(_true_path, _gvn.transform(continuation_support));
+  pinVirtualThread->init_req(_false_path, _gvn.intcon(0));
 
   // Update branch state.
   set_control(_gvn.transform(vthread_compare_rgn));
@@ -3450,6 +3462,9 @@ bool LibraryCallKit::inline_native_getEventWriter() {
   // Get the field offset to, conditionally, store an updated exclusion value later.
   Node* const event_writer_excluded_field = field_address_from_object(event_writer, "excluded", "Z", false);
   const TypePtr* event_writer_excluded_field_type = _gvn.type(event_writer_excluded_field)->isa_ptr();
+  // Get the field offset to, conditionally, store an updated pinVirtualThread value later.
+  Node* const event_writer_pin_field = field_address_from_object(event_writer, "pinVirtualThread", "Z", false);
+  const TypePtr* event_writer_pin_field_type = _gvn.type(event_writer_pin_field)->isa_ptr();
 
   RegionNode* event_writer_tid_compare_rgn = new RegionNode(PATH_LIMIT);
   record_for_igvn(event_writer_tid_compare_rgn);
@@ -3470,6 +3485,9 @@ bool LibraryCallKit::inline_native_getEventWriter() {
   Node* tid_is_not_equal = _gvn.transform(new IfTrueNode(iff_tid_not_equal));
   record_for_igvn(tid_is_not_equal);
 
+  // Store the pin state to the event writer.
+  store_to_memory(tid_is_not_equal, event_writer_pin_field, _gvn.transform(pinVirtualThread), T_BOOLEAN, event_writer_pin_field_type, MemNode::unordered);
+
   // Store the exclusion state to the event writer.
   store_to_memory(tid_is_not_equal, event_writer_excluded_field, _gvn.transform(exclusion), T_BOOLEAN, event_writer_excluded_field_type, MemNode::unordered);
 

src/java.base/share/classes/module-info.java

@@ -256,7 +256,8 @@ module java.base {
         jdk.internal.jvmstat,
         jdk.management,
         jdk.management.agent,
-        jdk.internal.vm.ci;
+        jdk.internal.vm.ci,
+        jdk.jfr;
     exports jdk.internal.vm.annotation to
         java.instrument,
         jdk.internal.vm.ci,

src/jdk.jfr/share/classes/jdk/jfr/internal/StringPool.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 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
@@ -26,6 +26,7 @@ package jdk.jfr.internal;
 
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.atomic.AtomicLong;
+import jdk.internal.vm.Continuation;
 
 public final class StringPool {
     public static final int MIN_LIMIT = 16;
@@ -71,8 +72,17 @@ public final class StringPool {
         return internalSid >> 16;
     }
 
+    /* Explicitly pin a virtual thread before acquiring the string pool monitor
+     * because migrating the EventWriter onto another carrier thread is impossible.
+     */
+    private static long storeString(String s, boolean pinVirtualThread) {
+        if (pinVirtualThread) {
+            assert(Thread.currentThread().isVirtual());
+            Continuation.pin();
+        }
+        try {
             /* synchronized because of writing the string to the JVM. */
-    private static synchronized long storeString(String s) {
+            synchronized (StringPool.class) {
                 Long lsid = cache.get(s);
                 long internalSid;
                 if (lsid != null) {
@@ -93,11 +103,18 @@ public final class StringPool {
                 cache.put(s, internalSid);
                 return extSid;
             }
+        } finally {
+            if (pinVirtualThread) {
+                assert(Thread.currentThread().isVirtual());
+                Continuation.unpin();
+            }
+        }
+    }
 
     /* a string fetched from the string pool must be of the current generation */
-    private static long ensureCurrentGeneration(String s, Long lsid) {
+    private static long ensureCurrentGeneration(String s, Long lsid, boolean pinVirtualThread) {
         long internalSid = lsid.longValue();
-        return isCurrentGeneration(internalSid) ? externalSid(internalSid) : storeString(s);
+        return isCurrentGeneration(internalSid) ? externalSid(internalSid) : storeString(s, pinVirtualThread);
     }
 
     /*
@@ -109,10 +126,10 @@ public final class StringPool {
      * effectively invalidating the fetched string id. The event restart mechanism
      * of the EventWriter ensures that committed strings are in the correct generation.
      */
-    public static long addString(String s) {
+    public static long addString(String s, boolean pinVirtualThread) {
         Long lsid = cache.get(s);
         if (lsid != null) {
-            return ensureCurrentGeneration(s, lsid);
+            return ensureCurrentGeneration(s, lsid, pinVirtualThread);
         }
         if (!preCache(s)) {
             /* we should not pool this string */
@@ -120,9 +137,9 @@ public final class StringPool {
         }
         if (cache.size() > MAX_SIZE || currentSizeUTF16 > MAX_SIZE_UTF16) {
             /* pool was full */
-            reset();
+            reset(pinVirtualThread);
         }
-        return storeString(s);
+        return storeString(s, pinVirtualThread);
     }
 
     private static boolean preCache(String s) {
@@ -143,8 +160,21 @@ public final class StringPool {
         return false;
     }
 
-    private static synchronized void reset() {
+    private static void reset(boolean pinVirtualThread) {
+        if (pinVirtualThread) {
+            assert(Thread.currentThread().isVirtual());
+            Continuation.pin();
+        }
+        try {
+            synchronized (StringPool.class) {
                 cache.clear();
                 currentSizeUTF16 = 0;
             }
+        } finally {
+            if (pinVirtualThread) {
+                assert(Thread.currentThread().isVirtual());
+                Continuation.unpin();
+            }
+        }
+    }
 }

src/jdk.jfr/share/classes/jdk/jfr/internal/event/EventWriter.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 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
@@ -65,6 +65,7 @@ public final class EventWriter {
     private long currentPosition;
     private long maxPosition;
     private boolean valid;
+    private boolean pinVirtualThread;
     boolean excluded;
 
     private PlatformEventType eventType;
@@ -144,7 +145,7 @@ public final class EventWriter {
             return;
         }
         if (length > StringPool.MIN_LIMIT && length < StringPool.MAX_LIMIT) {
-            long l = StringPool.addString(s);
+            long l = StringPool.addString(s, pinVirtualThread);
             if (l > 0) {
                 putByte(StringParser.Encoding.CONSTANT_POOL.byteValue());
                 putLong(l);
@@ -296,11 +297,12 @@ public final class EventWriter {
         return false;
     }
 
-    private EventWriter(long startPos, long maxPos, long threadID, boolean valid, boolean excluded) {
+    private EventWriter(long startPos, long maxPos, long threadID, boolean valid, boolean pinVirtualThread, boolean excluded) {
         startPosition = currentPosition = startPos;
         maxPosition = maxPos;
         this.threadID = threadID;
         this.valid = valid;
+        this.pinVirtualThread = pinVirtualThread;
         this.excluded = excluded;
     }
 

test/jdk/jdk/jfr/threading/TestStringPoolVirtualThreadPinning.java

@@ -0,0 +1,110 @@
+/*
+ * Copyright (c) 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
+ * 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.
+ */
+package jdk.jfr.threading;
+
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ThreadFactory;
+
+import jdk.jfr.Event;
+import jdk.jfr.Name;
+import jdk.jfr.Recording;
+import jdk.jfr.consumer.RecordedEvent;
+import jdk.jfr.consumer.RecordedThread;
+import jdk.jfr.consumer.RecordingFile;
+import jdk.test.lib.Asserts;
+import jdk.test.lib.Utils;
+
+/**
+ * @test
+ * @bug 8338417
+ * @summary Tests pinning of virtual threads when the JFR string pool monitor is contended.
+ * @key jfr
+ * @requires vm.hasJFR & vm.continuations
+ * @library /test/lib /test/jdk
+ * @run main/othervm jdk.jfr.threading.TestStringPoolVirtualThreadPinning
+ */
+public class TestStringPoolVirtualThreadPinning {
+
+    private static final int VIRTUAL_THREAD_COUNT = 100_000;
+    private static final int STARTER_THREADS = 10;
+
+    @Name("test.Tester")
+    private static class TestEvent extends Event {
+        private String eventString = Thread.currentThread().getName();
+    }
+
+    /*
+     * During event commit, the thread is in a critical section because it has loaded a carrier thread local event writer object.
+     * For virtual threads, a contended monitor, such as a synchronized block, is a point where a thread could become unmounted.
+     * A monitor guards the JFR string pool, but because of the event writer, remounting a virtual thread onto another carrier is impossible.
+     *
+     * The test provokes JFR string pool monitor contention to exercise explicit pin constructs to ensure the pinning of virtual threads.
+    */
+    public static void main(String... args) throws Exception {
+        try (Recording r = new Recording()) {
+            r.start();
+
+            ThreadFactory factory = Thread.ofVirtual().factory();
+            CompletableFuture<?>[] c = new CompletableFuture[STARTER_THREADS];
+            for (int j = 0; j < STARTER_THREADS; j++) {
+                c[j] = CompletableFuture.runAsync(() -> {
+                    for (int i = 0; i < VIRTUAL_THREAD_COUNT / STARTER_THREADS; i++) {
+                        try {
+                            Thread vt = factory.newThread(TestStringPoolVirtualThreadPinning::emitEvent);
+                            // For an event field string to be placed in the JFR string pool, it must exceed 16 characters.
+                            // We use the virtual thread name as the event field string so we can verify the result as a 1-1 mapping.
+                            vt.setName("VirtualTestThread-" + i);
+                            vt.start();
+                            vt.join();
+                        } catch (InterruptedException ie) {
+                            ie.printStackTrace();
+                        }
+                    }
+                });
+            }
+            for (int j = 0; j < STARTER_THREADS; j++) {
+                c[j].get();
+            }
+
+            r.stop();
+            Path p = Utils.createTempFile("test", ".jfr");
+            r.dump(p);
+            List<RecordedEvent> events = RecordingFile.readAllEvents(p);
+            Asserts.assertEquals(events.size(), VIRTUAL_THREAD_COUNT, "Expected " + VIRTUAL_THREAD_COUNT + " events");
+            for (RecordedEvent e : events) {
+                RecordedThread t = e.getThread();
+                Asserts.assertNotNull(t);
+                Asserts.assertTrue(t.isVirtual());
+                Asserts.assertEquals(e.getString("eventString"), t.getJavaName());
+            }
+        }
+    }
+
+    private static void emitEvent() {
+        TestEvent t = new TestEvent();
+        t.commit();
+    }
+}