Code review feedback.

oracle · Nov 5, 2021 · 59ca2d7 · 59ca2d7
1 parent 6c96e2f
commit 59ca2d7
Show file tree

Hide file tree

Showing 6 changed files with 64 additions and 51 deletions.
diff --git a/substratevm/src/com.oracle.svm.jfr/src/com/oracle/svm/jfr/JfrChunkWriter.java b/substratevm/src/com.oracle.svm.jfr/src/com/oracle/svm/jfr/JfrChunkWriter.java
@@ -406,7 +406,7 @@ private void changeEpoch() {
             JfrTraceIdEpoch.getInstance().changeEpoch();
 
             // Now that the epoch changed, re-register all running threads for the new epoch.
-            JfrThreadRepository.registerRunningThreads();
+            SubstrateJVM.getThreadRepo().registerRunningThreads();
         }
     }
 

diff --git a/substratevm/src/com.oracle.svm.jfr/src/com/oracle/svm/jfr/JfrManager.java b/substratevm/src/com.oracle.svm.jfr/src/com/oracle/svm/jfr/JfrManager.java
@@ -44,8 +44,8 @@
 import com.oracle.svm.core.SubstrateOptions;
 import com.oracle.svm.core.util.UserError.UserException;
 import com.oracle.svm.core.util.VMError;
-import com.oracle.svm.jfr.events.EndChunkPeriodicEvents;
-import com.oracle.svm.jfr.events.EveryChunkPeriodicEvents;
+import com.oracle.svm.jfr.events.EndChunkNativePeriodicEvents;
+import com.oracle.svm.jfr.events.EveryChunkNativePeriodicEvents;
 
 import jdk.jfr.FlightRecorder;
 import jdk.jfr.Recording;
@@ -89,8 +89,8 @@ Runnable shutdownHook() {
                 // Everything should already have been torn down by JVM.destroyJFR(), which is
                 // called in a shutdown hook. So in this method we should only unregister periodic
                 // events.
-                FlightRecorder.removePeriodicEvent(EveryChunkPeriodicEvents::emit);
-                FlightRecorder.removePeriodicEvent(EndChunkPeriodicEvents::emit);
+                FlightRecorder.removePeriodicEvent(EveryChunkNativePeriodicEvents::emit);
+                FlightRecorder.removePeriodicEvent(EndChunkNativePeriodicEvents::emit);
             }
         };
     }
@@ -102,8 +102,8 @@ private static void parseFlightRecorderLogging(String option) {
     private static void periodicEventSetup() throws SecurityException {
         // The callbacks that are registered below, are invoked regularly to emit periodic native
         // events such as OSInformation or JVMInformation.
-        FlightRecorder.addPeriodicEvent(EveryChunkPeriodicEvents.class, EveryChunkPeriodicEvents::emit);
-        FlightRecorder.addPeriodicEvent(EndChunkPeriodicEvents.class, EndChunkPeriodicEvents::emit);
+        FlightRecorder.addPeriodicEvent(EveryChunkNativePeriodicEvents.class, EveryChunkNativePeriodicEvents::emit);
+        FlightRecorder.addPeriodicEvent(EndChunkNativePeriodicEvents.class, EndChunkNativePeriodicEvents::emit);
     }
 
     private static void initRecording() {

diff --git a/substratevm/src/com.oracle.svm.jfr/src/com/oracle/svm/jfr/JfrThreadRepository.java b/substratevm/src/com.oracle.svm.jfr/src/com/oracle/svm/jfr/JfrThreadRepository.java
@@ -24,6 +24,7 @@
  */
 package com.oracle.svm.jfr;
 
+import com.oracle.svm.core.thread.VMOperation;
 import org.graalvm.nativeimage.ImageSingletons;
 import org.graalvm.nativeimage.IsolateThread;
 import org.graalvm.nativeimage.Platform;
@@ -66,14 +67,20 @@ public final class JfrThreadRepository implements JfrConstantPool {
     }
 
     @Uninterruptible(reason = "Prevent any JFR events from triggering.")
-    public static void registerRunningThreads() {
-        for (IsolateThread isolateThread = VMThreads.firstThread(); isolateThread.isNonNull(); isolateThread = VMThreads.nextThread(isolateThread)) {
-            // IsolateThreads without a Java thread just started executing and will register
-            // themselves later on.
-            Thread thread = JavaThreads.fromVMThread(isolateThread);
-            if (thread != null) {
-                SubstrateJVM.getThreadRepo().registerThread(thread);
+    public void registerRunningThreads() {
+        assert VMOperation.isInProgressAtSafepoint();
+        mutex.lockNoTransition();
+        try {
+            for (IsolateThread isolateThread = VMThreads.firstThread(); isolateThread.isNonNull(); isolateThread = VMThreads.nextThread(isolateThread)) {
+                // IsolateThreads without a Java thread just started executing and will register
+                // themselves later on.
+                Thread thread = JavaThreads.fromVMThread(isolateThread);
+                if (thread != null) {
+                    registerThread0(thread);
+                }
             }
+        } finally {
+            mutex.unlock();
         }
     }
 
@@ -85,44 +92,50 @@ public void registerThread(Thread thread) {
 
         mutex.lockNoTransition();
         try {
-            JfrThreadEpochData epochData = getEpochData(false);
-            if (epochData.threadBuffer.isNull()) {
-                // This will happen only on the first call.
-                epochData.threadBuffer = JfrBufferAccess.allocate(WordFactory.unsigned(INITIAL_BUFFER_SIZE), JfrBufferType.C_HEAP);
-            }
+            registerThread0(thread);
+        } finally {
+            mutex.unlock();
+        }
+    }
 
-            JfrVisited visitedThread = StackValue.get(JfrVisited.class);
-            visitedThread.setId(thread.getId());
-            visitedThread.setHash((int) thread.getId());
-            if (!epochData.visitedThreads.putIfAbsent(visitedThread)) {
-                return;
-            }
+    @Uninterruptible(reason = "Epoch must not change while in this method.")
+    private void registerThread0(Thread thread) {
+        assert SubstrateJVM.isRecording();
+        JfrThreadEpochData epochData = getEpochData(false);
+        if (epochData.threadBuffer.isNull()) {
+            // This will happen only on the first call.
+            epochData.threadBuffer = JfrBufferAccess.allocate(WordFactory.unsigned(INITIAL_BUFFER_SIZE), JfrBufferType.C_HEAP);
+        }
 
-            JfrNativeEventWriterData data = StackValue.get(JfrNativeEventWriterData.class);
-            JfrNativeEventWriterDataAccess.initialize(data, epochData.threadBuffer);
-
-            JfrNativeEventWriter.putLong(data, thread.getId());
-            JfrNativeEventWriter.putString(data, thread.getName());
-            JfrNativeEventWriter.putLong(data, thread.getId());
-            JfrNativeEventWriter.putString(data, thread.getName());
-            JfrNativeEventWriter.putLong(data, thread.getId());
-
-            ThreadGroup threadGroup = thread.getThreadGroup();
-            if (threadGroup != null) {
-                long threadGroupId = JavaLangThreadGroupSubstitutions.getThreadGroupId(threadGroup);
-                JfrNativeEventWriter.putLong(data, threadGroupId);
-                registerThreadGroup(threadGroupId, threadGroup);
-            } else {
-                JfrNativeEventWriter.putLong(data, 0);
-            }
-            JfrNativeEventWriter.commit(data);
+        JfrVisited visitedThread = StackValue.get(JfrVisited.class);
+        visitedThread.setId(thread.getId());
+        visitedThread.setHash((int) thread.getId());
+        if (!epochData.visitedThreads.putIfAbsent(visitedThread)) {
+            return;
+        }
 
-            // Maybe during writing, the thread buffer was replaced with a new (larger) one, so we
-            // need to update the repository pointer as well.
-            epochData.threadBuffer = data.getJfrBuffer();
-        } finally {
-            mutex.unlock();
+        JfrNativeEventWriterData data = StackValue.get(JfrNativeEventWriterData.class);
+        JfrNativeEventWriterDataAccess.initialize(data, epochData.threadBuffer);
+
+        JfrNativeEventWriter.putLong(data, thread.getId()); // JFR trace id
+        JfrNativeEventWriter.putString(data, thread.getName()); // Java or native thread name
+        JfrNativeEventWriter.putLong(data, thread.getId()); // OS thread id
+        JfrNativeEventWriter.putString(data, thread.getName()); // Java thread name
+        JfrNativeEventWriter.putLong(data, thread.getId()); // Java thread id
+
+        ThreadGroup threadGroup = thread.getThreadGroup();
+        if (threadGroup != null) {
+            long threadGroupId = JavaLangThreadGroupSubstitutions.getThreadGroupId(threadGroup);
+            JfrNativeEventWriter.putLong(data, threadGroupId);
+            registerThreadGroup(threadGroupId, threadGroup);
+        } else {
+            JfrNativeEventWriter.putLong(data, 0);
         }
+        JfrNativeEventWriter.commit(data);
+
+        // Maybe during writing, the thread buffer was replaced with a new (larger) one, so we
+        // need to update the repository pointer as well.
+        epochData.threadBuffer = data.getJfrBuffer();
     }
 
     @Uninterruptible(reason = "Epoch must not change while in this method.")

diff --git a/substratevm/src/com.oracle.svm.jfr/src/com/oracle/svm/jfr/SubstrateJVM.java b/substratevm/src/com.oracle.svm.jfr/src/com/oracle/svm/jfr/SubstrateJVM.java
@@ -275,7 +275,7 @@ public void beginRecording() {
 
         JavaVMOperation.enqueueBlockingSafepoint("Jfr begin recording", () -> {
             recording = true;
-            JfrThreadRepository.registerRunningThreads();
+            SubstrateJVM.getThreadRepo().registerRunningThreads();
             // After changing the value of recording to true, JFR events can be triggered at any
             // time.
         });

diff --git a/...vm/jfr/events/EndChunkPeriodicEvents.java → .../events/EndChunkNativePeriodicEvents.java b/...vm/jfr/events/EndChunkPeriodicEvents.java → .../events/EndChunkNativePeriodicEvents.java
@@ -44,7 +44,7 @@
 
 @Name("EndChunkPeriodEvents")
 @Period(value = "endChunk")
-public class EndChunkPeriodicEvents extends Event {
+public class EndChunkNativePeriodicEvents extends Event {
 
     private static String formatOSInformation() {
         String name = System.getProperty("os.name");

diff --git a/.../jfr/events/EveryChunkPeriodicEvents.java → ...vents/EveryChunkNativePeriodicEvents.java b/.../jfr/events/EveryChunkPeriodicEvents.java → ...vents/EveryChunkNativePeriodicEvents.java
@@ -43,7 +43,7 @@
 
 @Name("EveryChunkPeriodEvents")
 @Period(value = "everyChunk")
-public class EveryChunkPeriodicEvents extends Event {
+public class EveryChunkNativePeriodicEvents extends Event {
 
     public static void emit() {
         ThreadMXBean threadMXBean = ManagementFactory.getThreadMXBean();