Use putThread to avoid races.

roberttoyonaga · roberttoyonaga · commit 22add5c2f8c8 · 2024-09-13T12:27:37.000-04:00
diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/JfrNativeEventWriter.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/JfrNativeEventWriter.java
@@ -250,12 +250,14 @@ public static void putThread(JfrNativeEventWriterData data, Thread thread) {
         if (thread == null) {
             putThread(data, 0L);
         } else {
+            SubstrateJVM.maybeRegisterVirtualThread(thread);
             putThread(data, SubstrateJVM.getThreadId(thread));
         }
     }
 
     @Uninterruptible(reason = "Accesses a native JFR buffer.", callerMustBe = true)
     public static void putThread(JfrNativeEventWriterData data, long threadId) {
+        SubstrateJVM.maybeRegisterVirtualThread(threadId);
         putLong(data, threadId);
     }
 
diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/JfrThreadRepository.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/JfrThreadRepository.java
@@ -24,6 +24,8 @@
  */
 package com.oracle.svm.core.jfr;
 
+import static com.oracle.svm.core.Uninterruptible.CALLED_FROM_UNINTERRUPTIBLE_CODE;
+
 import org.graalvm.nativeimage.IsolateThread;
 import org.graalvm.nativeimage.Platform;
 import org.graalvm.nativeimage.Platforms;
@@ -96,14 +98,33 @@ public void registerRunningThreads() {
         }
     }
 
+    /**
+     * If this method is called on platform threads, nothing will happen since they are already
+     * registered eagerly upon starting and at epoch changes.
+     */
+    @Uninterruptible(reason = "Locking without transition requires that the whole critical section is uninterruptible.")
+    public void registerThread(long threadId) {
+        if (!SubstrateJVM.get().isRecording()) {
+            return;
+        }
+
+        registerThread0(threadId, 0, true, null, null);
+    }
+
     @Uninterruptible(reason = "Locking without transition requires that the whole critical section is uninterruptible.")
     public void registerThread(Thread thread) {
         if (!SubstrateJVM.get().isRecording()) {
             return;
         }
 
         long threadId = JavaThreads.getThreadId(thread);
+        boolean isVirtual = JavaThreads.isVirtual(thread);
+        long osThreadId = isVirtual ? 0 : threadId;
+        registerThread0(threadId, osThreadId, isVirtual, thread, thread.getName());
+    }
 
+    @Uninterruptible(reason = CALLED_FROM_UNINTERRUPTIBLE_CODE, mayBeInlined = true)
+    private void registerThread0(long threadId, long osThreadId, boolean isVirtual, Thread thread, String name) {
         JfrVisited visitedThread = StackValue.get(JfrVisited.class);
         visitedThread.setId(threadId);
         visitedThread.setHash(UninterruptibleUtils.Long.hashCode(threadId));
@@ -124,14 +145,12 @@ public void registerThread(Thread thread) {
             JfrNativeEventWriterDataAccess.initialize(data, epochData.threadBuffer);
 
             /* Similar to JfrThreadConstant::serialize in HotSpot. */
-            boolean isVirtual = JavaThreads.isVirtual(thread);
-            long osThreadId = isVirtual ? 0 : threadId;
             long threadGroupId = registerThreadGroup(thread, isVirtual);
 
             JfrNativeEventWriter.putLong(data, threadId);
-            JfrNativeEventWriter.putString(data, thread.getName()); // OS thread name
+            JfrNativeEventWriter.putString(data, name); // OS thread name
             JfrNativeEventWriter.putLong(data, osThreadId); // OS thread id
-            JfrNativeEventWriter.putString(data, thread.getName()); // Java thread name
+            JfrNativeEventWriter.putString(data, name); // Java thread name
             JfrNativeEventWriter.putLong(data, threadId); // Java thread id
             JfrNativeEventWriter.putLong(data, threadGroupId); // Java thread group
             JfrNativeEventWriter.putBoolean(data, isVirtual);
diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/SubstrateJVM.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/SubstrateJVM.java
@@ -306,7 +306,6 @@ public long getStackTraceId(JfrEvent eventType, int skipCount) {
     @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
     public static long getThreadId(Thread thread) {
         if (HasJfrSupport.get()) {
-            maybeRegisterVirtualThread(thread);
             return JavaThreads.getThreadId(thread);
         }
         return 0;
@@ -315,7 +314,6 @@ public static long getThreadId(Thread thread) {
     @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
     public static long getCurrentThreadId() {
         if (HasJfrSupport.get()) {
-            maybeRegisterVirtualThread(Thread.currentThread());
             return JavaThreads.getCurrentThreadId();
         }
         return 0;
@@ -326,7 +324,7 @@ public static long getCurrentThreadId() {
      * eagerly when started and at chunk rotations.
      */
     @Uninterruptible(reason = "Epoch should not change while checking generation.")
-    private static void maybeRegisterVirtualThread(Thread thread) {
+    public static void maybeRegisterVirtualThread(Thread thread) {
         // Do quick preliminary checks to avoid global locking unless necessary.
         if (JavaThreads.isVirtual(thread)) {
             Target_java_lang_VirtualThread tjlv = SubstrateUtil.cast(thread, Target_java_lang_VirtualThread.class);
@@ -338,6 +336,15 @@ private static void maybeRegisterVirtualThread(Thread thread) {
         }
     }
 
+    /**
+     * {@link SubstrateJVM#maybeRegisterVirtualThread(Thread)} Is preferred over this method since
+     * it can perform preliminary checks to avoid locking the Thread Repository unnecessarily.
+     */
+    @Uninterruptible(reason = "Epoch should not change while checking generation.")
+    public static void maybeRegisterVirtualThread(long tid) {
+        getThreadRepo().registerThread(tid);
+    }
+
     /**
      * See {@link JVM#storeMetadataDescriptor}.
      */
diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/events/ExecutionSampleEvent.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/events/ExecutionSampleEvent.java
@@ -41,7 +41,7 @@ public static void writeExecutionSample(long elapsedTicks, long threadId, long s
 
             JfrNativeEventWriter.beginSmallEvent(data, JfrEvent.ExecutionSample);
             JfrNativeEventWriter.putLong(data, elapsedTicks);
-            JfrNativeEventWriter.putLong(data, threadId);
+            JfrNativeEventWriter.putThread(data, threadId);
             JfrNativeEventWriter.putLong(data, stackTraceId);
             JfrNativeEventWriter.putLong(data, threadState);
             JfrNativeEventWriter.endSmallEvent(data);
diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/events/JavaMonitorWaitEvent.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/events/JavaMonitorWaitEvent.java
@@ -40,15 +40,15 @@
 import jdk.graal.compiler.word.Word;
 
 public class JavaMonitorWaitEvent {
-    public static void emit(long startTicks, Object obj, long notifier, long timeout, boolean timedOut) {
+    public static void emit(long startTicks, Object obj, Thread notifier, long timeout, boolean timedOut) {
         if (HasJfrSupport.get() && obj != null && !Target_jdk_jfr_internal_JVM_ChunkRotationMonitor.class.equals(obj.getClass()) &&
                         !Target_jdk_jfr_internal_management_HiddenWait.class.equals(obj.getClass())) {
             emit0(startTicks, obj, notifier, timeout, timedOut);
         }
     }
 
     @Uninterruptible(reason = "Accesses a JFR buffer.")
-    private static void emit0(long startTicks, Object obj, long notifier, long timeout, boolean timedOut) {
+    private static void emit0(long startTicks, Object obj, Thread notifier, long timeout, boolean timedOut) {
         long duration = JfrTicks.duration(startTicks);
         if (JfrEvent.JavaMonitorWait.shouldEmit(duration)) {
             JfrNativeEventWriterData data = org.graalvm.nativeimage.StackValue.get(JfrNativeEventWriterData.class);
@@ -60,7 +60,7 @@ private static void emit0(long startTicks, Object obj, long notifier, long timeo
             JfrNativeEventWriter.putEventThread(data);
             JfrNativeEventWriter.putLong(data, SubstrateJVM.get().getStackTraceId(JfrEvent.JavaMonitorWait, 0));
             JfrNativeEventWriter.putClass(data, obj.getClass());
-            JfrNativeEventWriter.putLong(data, notifier);
+            JfrNativeEventWriter.putThread(data, notifier);
             JfrNativeEventWriter.putLong(data, timeout);
             JfrNativeEventWriter.putBoolean(data, timedOut);
             JfrNativeEventWriter.putLong(data, Word.objectToUntrackedPointer(obj).rawValue());
diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/events/ThreadStartEvent.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/events/ThreadStartEvent.java
@@ -47,7 +47,7 @@ public static void emit(Thread thread) {
             JfrNativeEventWriter.putEventThread(data);
             JfrNativeEventWriter.putLong(data, SubstrateJVM.get().getStackTraceId(JfrEvent.ThreadStart, 0));
             JfrNativeEventWriter.putThread(data, thread);
-            JfrNativeEventWriter.putLong(data, JavaThreads.getParentThreadId(thread));
+            JfrNativeEventWriter.putThread(data, JavaThreads.getParentThreadId(thread));
             JfrNativeEventWriter.endSmallEvent(data);
         }
     }
diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/monitor/JavaMonitorQueuedSynchronizer.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/monitor/JavaMonitorQueuedSynchronizer.java
@@ -32,7 +32,6 @@
 
 import com.oracle.svm.core.Uninterruptible;
 import com.oracle.svm.core.jfr.JfrTicks;
-import com.oracle.svm.core.jfr.SubstrateJVM;
 import com.oracle.svm.core.jfr.events.JavaMonitorWaitEvent;
 import com.oracle.svm.core.thread.JavaThreads;
 import com.oracle.svm.core.util.BasedOnJDKClass;
@@ -120,7 +119,7 @@ static final class ExclusiveNode extends Node {
     @BasedOnJDKClass(value = AbstractQueuedLongSynchronizer.class, innerClass = "ConditionNode")
     static final class ConditionNode extends Node {
         ConditionNode nextWaiter;
-        long notifierJfrTid;
+        Thread notifierJfr;
 
         // see AbstractQueuedLongSynchronizer.ConditionNode.isReleasable()
         public boolean isReleasable() {
@@ -457,7 +456,7 @@ private void doSignal(ConditionNode first, boolean all) {
                     first.nextWaiter = null; // GC assistance
                 }
                 if ((first.getAndUnsetStatus(COND) & COND) != 0) {
-                    first.notifierJfrTid = SubstrateJVM.getCurrentThreadId();
+                    first.notifierJfr = Thread.currentThread();
                     enqueue(first);
                     if (!all) {
                         break;
@@ -564,7 +563,7 @@ private ConditionNode newConditionNode() {
         public void await(Object obj) throws InterruptedException {
             long startTicks = JfrTicks.elapsedTicks();
             if (Thread.interrupted()) {
-                JavaMonitorWaitEvent.emit(startTicks, obj, 0, 0L, false);
+                JavaMonitorWaitEvent.emit(startTicks, obj, null, 0L, false);
                 throw new InterruptedException();
             }
             ConditionNode node = newConditionNode();
@@ -587,7 +586,7 @@ public void await(Object obj) throws InterruptedException {
             }
             node.clearStatus();
             // waiting is done, emit wait event
-            JavaMonitorWaitEvent.emit(startTicks, obj, node.notifierJfrTid, 0L, false);
+            JavaMonitorWaitEvent.emit(startTicks, obj, node.notifierJfr, 0L, false);
             acquire(node, savedAcquisitions);
             if (interrupted) {
                 if (cancelled) {
@@ -604,7 +603,7 @@ public boolean await(Object obj, long time, TimeUnit unit) throws InterruptedExc
             long startTicks = JfrTicks.elapsedTicks();
             long nanosTimeout = unit.toNanos(time);
             if (Thread.interrupted()) {
-                JavaMonitorWaitEvent.emit(startTicks, obj, 0, 0L, false);
+                JavaMonitorWaitEvent.emit(startTicks, obj, null, 0L, false);
                 throw new InterruptedException();
             }
             ConditionNode node = newConditionNode();
@@ -627,7 +626,7 @@ public boolean await(Object obj, long time, TimeUnit unit) throws InterruptedExc
             }
             node.clearStatus();
             // waiting is done, emit wait event
-            JavaMonitorWaitEvent.emit(startTicks, obj, node.notifierJfrTid, time, cancelled);
+            JavaMonitorWaitEvent.emit(startTicks, obj, node.notifierJfr, time, cancelled);
             acquire(node, savedAcquisitions);
             if (cancelled) {
                 unlinkCancelledWaiters(node);

Original file line number	Diff line number	Diff line change
`@@ -250,12 +250,14 @@ public static void putThread(JfrNativeEventWriterData data, Thread thread) {`
`250`	`250`	`if (thread == null) {`
`251`	`251`	`putThread(data, 0L);`
`252`	`252`	`} else {`
	`253`	`+ SubstrateJVM.maybeRegisterVirtualThread(thread);`
`253`	`254`	`putThread(data, SubstrateJVM.getThreadId(thread));`
`254`	`255`	`}`
`255`	`256`	`}`
`256`	`257`
`257`	`258`	`@Uninterruptible(reason = "Accesses a native JFR buffer.", callerMustBe = true)`
`258`	`259`	`public static void putThread(JfrNativeEventWriterData data, long threadId) {`
	`260`	`+ SubstrateJVM.maybeRegisterVirtualThread(threadId);`
`259`	`261`	`putLong(data, threadId);`
`260`	`262`	`}`
`261`	`263`
Original file line number	Diff line number	Diff line change
`@@ -47,7 +47,7 @@ public static void emit(Thread thread) {`
`47`	`47`	`JfrNativeEventWriter.putEventThread(data);`
`48`	`48`	`JfrNativeEventWriter.putLong(data, SubstrateJVM.get().getStackTraceId(JfrEvent.ThreadStart, 0));`
`49`	`49`	`JfrNativeEventWriter.putThread(data, thread);`
`50`		`- JfrNativeEventWriter.putLong(data, JavaThreads.getParentThreadId(thread));`
	`50`	`+ JfrNativeEventWriter.putThread(data, JavaThreads.getParentThreadId(thread));`
`51`	`51`	`JfrNativeEventWriter.endSmallEvent(data);`
`52`	`52`	`}`
`53`	`53`	`}`