Simplify the safepoint handling and fix assertions.

Author: christianhaeubl

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/Safepoint.java

@@ -116,6 +116,13 @@ boolean isInProgress() {
         return safepointState == AT_SAFEPOINT;
     }
 
+    @Uninterruptible(reason = CALLED_FROM_UNINTERRUPTIBLE_CODE, mayBeInlined = true)
+    public boolean isPendingOrInProgress() {
+        /* Only read the state once. */
+        int state = safepointState;
+        return state == SYNCHRONIZING || state == AT_SAFEPOINT;
+    }
+
     @Uninterruptible(reason = CALLED_FROM_UNINTERRUPTIBLE_CODE, mayBeInlined = true)
     public UnsignedWord getSafepointId() {
         return safepointId;
@@ -126,11 +133,6 @@ public boolean isMasterThread() {
         return requestingThread == CurrentIsolate.getCurrentThread();
     }
 
-    @Uninterruptible(reason = CALLED_FROM_UNINTERRUPTIBLE_CODE, mayBeInlined = true)
-    public boolean isPendingOrInProgress() {
-        return requestingThread.isNonNull();
-    }
-
     /**
      * Initiates a safepoint.
      *
@@ -150,12 +152,13 @@ boolean startSafepoint(String reason) {
             THREAD_MUTEX.lock();
         }
 
+        assert requestingThread.isNull();
         requestingThread = CurrentIsolate.getCurrentThread();
         ImageSingletons.lookup(Heap.class).prepareForSafepoint();
+
         safepointState = SYNCHRONIZING;
+        int numJavaThreads = requestThreadsEnterSafepoint(reason);
 
-        int numJavaThreads = requestThreadsEnterSafepoint();
-        waitUntilThreadsEnterSafepoint(reason);
         safepointState = AT_SAFEPOINT;
         safepointId = safepointId.add(1);
         SafepointBeginEvent.emit(getSafepointId(), numJavaThreads, startTicks);
@@ -167,46 +170,34 @@ boolean startSafepoint(String reason) {
     void endSafepoint(boolean unlock) {
         assert VMOperationControl.mayExecuteVmOperations();
         long startTicks = JfrTicks.elapsedTicks();
+
         safepointState = NOT_AT_SAFEPOINT;
         releaseThreadsFromSafepoint();
         /* Some Java threads can continue execution now. */
 
-        SafepointEndEvent.emit(getSafepointId(), startTicks);
-        ImageSingletons.lookup(Heap.class).endSafepoint();
-
-        requestingThread = Word.nullPointer();
         if (unlock) {
             THREAD_MUTEX.unlock();
         }
 
-        /* This can take a moment, so we do it after letting all other threads proceed. */
-        VMThreads.singleton().cleanupExitedOsThreads();
-    }
-
-    /**
-     * Ask all other threads to come to a safepoint. There is no guarantee that the threads will
-     * comply with the request.
-     */
-    private static int requestThreadsEnterSafepoint() {
-        assert THREAD_MUTEX.isOwner() : "must hold mutex while requesting a safepoint";
-        int numJavaThreads = 0;
+        /*
+         * Now that the safepoint was released, we can treat this thread like a normal VM thread and
+         * don't need the special handling in the safepoint slowpath anymore. Other threads also
+         * can't start a new safepoint yet (this code is still executed in the VM operation thread
+         * and only the VM operation thread may start a safepoint).
+         */
+        requestingThread = Word.nullPointer();
 
-        for (IsolateThread thread = VMThreads.firstThread(); thread.isNonNull(); thread = VMThreads.nextThread(thread)) {
-            numJavaThreads++;
-            if (thread == CurrentIsolate.getCurrentThread()) {
-                continue;
-            }
-            if (SafepointBehavior.ignoresSafepoints(thread)) {
-                /* If safepoints are disabled, do not ask it to stop at safepoints. */
-                continue;
-            }
-            requestEnterSafepoint(thread);
-        }
-        return numJavaThreads;
+        /*
+         * Everything down here can take a moment, so we do it after letting all other threads
+         * proceed.
+         */
+        ImageSingletons.lookup(Heap.class).endSafepoint();
+        SafepointEndEvent.emit(getSafepointId(), startTicks);
+        VMThreads.singleton().cleanupExitedOsThreads();
     }
 
-    /** Wait until all other threads reach a safepoint. Re-requests safepoints if necessary. */
-    private static void waitUntilThreadsEnterSafepoint(String reason) {
+    /** Blocks until all other threads entered the safepoint. */
+    private static int requestThreadsEnterSafepoint(String reason) {
         assert THREAD_MUTEX.isOwner() : "must hold mutex while waiting for safepoints";
 
         long startNanos = System.nanoTime();
@@ -216,12 +207,13 @@ private static void waitUntilThreadsEnterSafepoint(String reason) {
         long failureNanos = -1;
 
         for (int loopCount = 1; /* return */ ; loopCount++) {
+            int numThreads = 0;
             int atSafepoint = 0;
             int ignoreSafepoints = 0;
             int notAtSafepoint = 0;
-            int lostUpdates = 0;
 
             for (IsolateThread thread = VMThreads.firstThread(); thread.isNonNull(); thread = VMThreads.nextThread(thread)) {
+                numThreads++;
                 if (thread == CurrentIsolate.getCurrentThread()) {
                     continue;
                 }
@@ -245,10 +237,9 @@ private static void waitUntilThreadsEnterSafepoint(String reason) {
                     switch (status) {
                         case StatusSupport.STATUS_IN_JAVA:
                         case StatusSupport.STATUS_IN_VM: {
-                            /* Re-request the safepoint in case of a lost update. */
+                            /* Request the safepoint (or re-request in case of a lost update). */
                             if (SafepointCheckCounter.getVolatile(thread) > 0) {
                                 requestEnterSafepoint(thread);
-                                lostUpdates++;
                             }
                             notAtSafepoint++;
                             break;
@@ -274,8 +265,8 @@ private static void waitUntilThreadsEnterSafepoint(String reason) {
             }
 
             if (notAtSafepoint == 0) {
-                /* All threads entered the safepoint. */
-                return;
+                /* All relevant threads entered the safepoint. */
+                return numThreads;
             }
 
             if (warningNanos == -1 || failureNanos == -1) {
@@ -297,7 +288,6 @@ private static void waitUntilThreadsEnterSafepoint(String reason) {
                                     .string("  atSafepoint: ").signed(atSafepoint)
                                     .string("  ignoreSafepoints: ").signed(ignoreSafepoints)
                                     .string("  notAtSafepoint: ").signed(notAtSafepoint)
-                                    .string("  lostUpdates: ").signed(lostUpdates)
                                     .string("]")
                                     .newline();
 
@@ -333,6 +323,7 @@ private static void releaseThreadsFromSafepoint() {
                 continue;
             }
 
+            assert StatusSupport.getStatusVolatile(thread) == StatusSupport.STATUS_IN_SAFEPOINT;
             restoreCounter(thread);
 
             /* Skip suspended threads so that they remain in STATUS_IN_SAFEPOINT. */

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/SafepointCheckCounter.java

@@ -39,13 +39,17 @@
 /**
  * Per-thread counter for safepoint checks. If the counter reaches a value less or equal 0, the
  * {@link SafepointSlowpath} is entered.
- *
+ * <p>
  * Be careful when calling any of the methods that manipulate or set the counter value directly as
  * they have the potential to destroy or skew the data that is needed for scheduling the execution
  * of recurring callbacks (i.e., the number of executed safepoints in a period of time). See class
  * {@link RecurringCallbackSupport} for more details about recurring callbacks.
- *
- * The safepoint check counter can have one of the following values:
+ * <p>
+ * If the recurring callback support is disabled, the safepoint check counter can only be 0
+ * (safepoint requested) or {@link #MAX_VALUE} (normal execution).
+ * <p>
+ * If the recurring callback support is enabled, the safepoint check counter can have one of the
+ * following values:
  * <ul>
  * <li>value > 0: remaining number of safepoint checks before the safepoint slowpath code is
  * executed.</li>
@@ -69,7 +73,7 @@ static boolean compareAndSet(IsolateThread thread, int oldValue, int newValue) {
     @Uninterruptible(reason = CALLED_FROM_UNINTERRUPTIBLE_CODE, mayBeInlined = true)
     static void setVolatile(IsolateThread thread, int value) {
         assert CurrentIsolate.getCurrentThread() == thread || VMThreads.StatusSupport.isStatusCreated(thread) || VMOperationControl.mayExecuteVmOperations();
-        assert value > 0;
+        assert RecurringCallbackSupport.isEnabled() && value > 0 || !RecurringCallbackSupport.isEnabled() && (value == 0 || value == MAX_VALUE);
         valueTL.setVolatile(thread, value);
     }
 

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/SafepointSlowpath.java

@@ -38,6 +38,7 @@
 import com.oracle.svm.core.snippets.SnippetRuntime.SubstrateForeignCallDescriptor;
 import com.oracle.svm.core.snippets.SubstrateForeignCallTarget;
 import com.oracle.svm.core.stack.JavaFrameAnchors;
+import com.oracle.svm.core.thread.RecurringCallbackSupport.RecurringCallbackTimer;
 import com.oracle.svm.core.thread.VMThreads.ActionOnTransitionToJavaSupport;
 import com.oracle.svm.core.thread.VMThreads.StatusSupport;
 import com.oracle.svm.core.util.VMError;
@@ -48,11 +49,11 @@
  * {@link #slowPathSafepointCheck(int, boolean, boolean) slowpath}. See {@link SafepointCheckNode}
  * and {@link SafepointSnippets} for more details.
  * <p>
- * {@link RecurringCallbackSupport} are implemented on top of the safepoint mechanism. If
- * {@linkplain RecurringCallbackSupport#isEnabled supported}, each safepoint check decrements
- * {@link SafepointCheckCounter} before the comparison. So, threads enter the safepoint slowpath
- * periodically, even if no safepoint is pending. At the end of the slowpath, the recurring callback
- * is executed if the timer has expired.
+ * {@link RecurringCallbackSupport Recurring callbacks} are implemented on top of the safepoint
+ * mechanism. If {@linkplain RecurringCallbackSupport#isEnabled supported}, each safepoint check
+ * decrements {@link SafepointCheckCounter} before the comparison. So, threads enter the safepoint
+ * slowpath periodically, even if no safepoint is pending. At the end of the slowpath, the recurring
+ * callback is executed if the timer has expired.
  */
 public class SafepointSlowpath {
     /*
@@ -134,7 +135,7 @@ private static void slowPathSafepointCheck(int newStatus, boolean callerHasJavaF
             slowPathSafepointCheck0(newStatus, callerHasJavaFrameAnchor, popFrameAnchor);
         } catch (RecurringCallbackSupport.SafepointException e) {
             /* This exception is intended to be thrown from safepoint checks, at one's own risk */
-            throw RecurringCallbackSupport.RecurringCallbackTimer.getAndClearPendingException();
+            throw RecurringCallbackTimer.getAndClearPendingException();
         } catch (Throwable ex) {
             /*
              * The foreign call from snippets to this method does not have an exception edge. So we

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/VMThreads.java

@@ -69,6 +69,7 @@
 import jdk.graal.compiler.api.directives.GraalDirectives;
 import jdk.graal.compiler.api.replacements.Fold;
 import jdk.graal.compiler.core.common.SuppressFBWarnings;
+import jdk.graal.compiler.nodes.PauseNode;
 import jdk.graal.compiler.replacements.ReplacementsUtil;
 import jdk.graal.compiler.replacements.nodes.AssertionNode;
 import jdk.graal.compiler.word.Word;
@@ -214,6 +215,7 @@ public static boolean ensureInitialized() {
             /* Already initialized, or some other thread claimed the initialization lock. */
             while (initializationState.get() < STATE_INITIALIZED) {
                 /* Busy wait until the other thread finishes the initialization. */
+                PauseNode.pause();
             }
         }
         return result;