[GR-67578] Reference handler and VM operation thread fixes.

PullRequest: graal/21556

Author: christianhaeubl

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/SubstrateOptions.java

@@ -24,7 +24,6 @@
  */
 package com.oracle.svm.core;
 
-import static com.oracle.svm.core.SubstrateOptions.DeprecatedOptions.TearDownFailureNanos;
 import static com.oracle.svm.core.option.RuntimeOptionKey.RuntimeOptionKeyFlag.Immutable;
 import static com.oracle.svm.core.option.RuntimeOptionKey.RuntimeOptionKeyFlag.RegisterForIsolateArgumentParser;
 import static com.oracle.svm.core.option.RuntimeOptionKey.RuntimeOptionKeyFlag.RelevantForCompilationIsolates;
@@ -618,17 +617,13 @@ protected void onValueUpdate(EconomicMap<OptionKey<?>, Object> values, Boolean o
             }
         };
 
-        public static final String TEAR_DOWN_WARNING_NANOS_ERROR = "Can't set both TearDownWarningSeconds and TearDownWarningNanos at the same time. Use TearDownWarningSeconds.";
-        @Option(help = "The number of nanoseconds before and between which tearing down an isolate gives a warning message. 0 implies no warning.", //
+        @Option(help = "The number of nanoseconds that the isolate teardown can take before warnings are printed. Disabled if less or equal to 0.", //
                         deprecated = true, deprecationMessage = "Use -XX:TearDownWarningSeconds=<secs> instead")//
-        public static final RuntimeOptionKey<Long> TearDownWarningNanos = new RuntimeOptionKey<>(0L,
-                        (key) -> UserError.guarantee(!(key.hasBeenSet() && TearDownWarningSeconds.hasBeenSet()), TEAR_DOWN_WARNING_NANOS_ERROR),
-                        RelevantForCompilationIsolates);
+        public static final RuntimeOptionKey<Long> TearDownWarningNanos = new RuntimeOptionKey<>(0L, RelevantForCompilationIsolates);
 
-        @Option(help = "The number of nanoseconds before tearing down an isolate gives a failure message and returns from a tear-down call. 0 implies no message.", //
-                        deprecated = true, deprecationMessage = "This call leaks resources. Instead, terminate java threads cooperatively, or use System#exit")//
+        @Option(help = "The number of nanoseconds that the isolate teardown can take before a fatal error is thrown. Disabled if less or equal to 0.", //
+                        deprecated = true, deprecationMessage = "Use -XX:TearDownFailureSeconds=<secs> instead")//
         public static final RuntimeOptionKey<Long> TearDownFailureNanos = new RuntimeOptionKey<>(0L, RelevantForCompilationIsolates);
-
     }
 
     @LayerVerifiedOption(kind = Kind.Changed, severity = Severity.Error)//
@@ -870,24 +865,18 @@ private static void validateZapNativeMemory(HostedOptionKey<Boolean> optionKey)
         }
     }
 
-    /*
-     * Isolate tear down options.
-     */
-    @Option(help = "The number of seconds before and between which tearing down an isolate gives a warning message. 0 implies no warning.")//
-    public static final RuntimeOptionKey<Long> TearDownWarningSeconds = new RuntimeOptionKey<>(0L, RelevantForCompilationIsolates);
-
     public static long getTearDownWarningNanos() {
-        if (TearDownWarningSeconds.hasBeenSet() && DeprecatedOptions.TearDownWarningNanos.hasBeenSet()) {
-            throw new IllegalArgumentException(DeprecatedOptions.TEAR_DOWN_WARNING_NANOS_ERROR);
-        }
-        if (DeprecatedOptions.TearDownWarningNanos.hasBeenSet()) {
-            return DeprecatedOptions.TearDownWarningNanos.getValue();
+        if (ConcealedOptions.TearDownWarningSeconds.getValue() != 0) {
+            return TimeUtils.secondsToNanos(ConcealedOptions.TearDownWarningSeconds.getValue());
         }
-        return TearDownWarningSeconds.getValue() * TimeUtils.nanosPerSecond;
+        return DeprecatedOptions.TearDownWarningNanos.getValue();
     }
 
     public static long getTearDownFailureNanos() {
-        return TearDownFailureNanos.getValue();
+        if (ConcealedOptions.TearDownFailureSeconds.getValue() != 0) {
+            return TimeUtils.secondsToNanos(ConcealedOptions.TearDownFailureSeconds.getValue());
+        }
+        return DeprecatedOptions.TearDownFailureNanos.getValue();
     }
 
     @Option(help = "Define the maximum number of stores for which the loop that zeroes out objects is unrolled.")//
@@ -1264,6 +1253,15 @@ protected void onValueUpdate(EconomicMap<OptionKey<?>, Object> values, Integer o
         @Option(help = "Avoid linker relocations for code and instead emit address computations.", type = OptionType.Expert) //
         @LayerVerifiedOption(severity = Severity.Error, kind = Kind.Changed, positional = false) //
         public static final HostedOptionKey<Boolean> RelativeCodePointers = new HostedOptionKey<>(false, SubstrateOptions::validateRelativeCodePointers);
+
+        /** Use {@link SubstrateOptions#getTearDownWarningNanos()} instead. */
+        @Option(help = "The number of seconds that the isolate teardown can take before warnings are printed. Disabled if less or equal to 0.")//
+        public static final RuntimeOptionKey<Long> TearDownWarningSeconds = new RuntimeOptionKey<>(0L, RelevantForCompilationIsolates);
+
+        /** Use {@link SubstrateOptions#getTearDownFailureNanos()} instead. */
+        @Option(help = "The number of seconds that the isolate teardown can take before a fatal error is thrown. Disabled if less or equal to 0.")//
+        public static final RuntimeOptionKey<Long> TearDownFailureSeconds = new RuntimeOptionKey<>(0L, RelevantForCompilationIsolates);
+
     }
 
     @Option(help = "Overwrites the available number of processors provided by the OS. Any value <= 0 means using the processor count from the OS.")//

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/graal/snippets/CEntryPointSnippets.java

@@ -671,14 +671,17 @@ public static int tearDownIsolateSnippet() {
     private static int tearDownIsolate() {
         try {
             /* Execute interruptible code. */
-            if (!initiateTearDownIsolateInterruptibly()) {
-                return CEntryPointErrors.UNSPECIFIED;
-            }
+            initiateTearDownIsolateInterruptibly();
 
             /* After threadExit(), only uninterruptible code may be executed. */
             RecurringCallbackSupport.suspendCallbackTimer("Execution of arbitrary code is prohibited during the last teardown steps.");
 
-            /* Shut down VM thread. */
+            /* Wait until the reference handler thread detaches (it was already stopped earlier). */
+            if (ReferenceHandler.useDedicatedThread()) {
+                ReferenceHandlerThread.waitUntilDetached();
+            }
+
+            /* Shut down VM operation thread. */
             if (VMOperationControl.useDedicatedVMOperationThread()) {
                 VMOperationControl.shutdownAndDetachVMOperationThread();
             }
@@ -717,15 +720,20 @@ private static int tearDownIsolate() {
         }
     }
 
-    @Uninterruptible(reason = "Tear-down in progress - still safe to execute interruptible Java code.", calleeMustBe = false)
-    private static boolean initiateTearDownIsolateInterruptibly() {
+    @Uninterruptible(reason = "Tear-down in progress - still safe to execute interruptible Java code.", callerMustBe = true, calleeMustBe = false)
+    private static void initiateTearDownIsolateInterruptibly() {
         RuntimeSupport.executeTearDownHooks();
-        if (!PlatformThreads.tearDownOtherThreads()) {
-            return false;
+        PlatformThreads.tearDownOtherThreads();
+        /*
+         * At this point, only the current thread, the VM operation thread, and the reference
+         * handler thread are still running.
+         */
+        if (ReferenceHandler.useDedicatedThread()) {
+            ReferenceHandlerThread.initiateShutdown();
         }
 
         VMThreads.singleton().threadExit();
-        return true;
+        /* After threadExit(), only uninterruptible code may be executed. */
     }
 
     @Snippet(allowMissingProbabilities = true)
@@ -802,7 +810,7 @@ static boolean runtimeAssertionsEnabled() {
     @SubstrateForeignCallTarget(stubCallingConvention = false)
     private static int verifyIsolateThread(IsolateThread thread) {
         VMError.guarantee(CurrentIsolate.getCurrentThread() == thread, "Threads must match for the call below");
-        if (!VMThreads.singleton().verifyIsCurrentThread(thread) || !VMThreads.singleton().verifyThreadIsAttached(thread)) {
+        if (!VMThreads.singleton().verifyIsCurrentThread(thread) || !VMThreads.isAttached(thread)) {
             throw VMError.shouldNotReachHere("A call from native code to Java code provided the wrong JNI environment or the wrong IsolateThread. " +
                             "The JNI environment / IsolateThread is a thread-local data structure and must not be shared between threads.");
 

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/heap/ReferenceHandler.java

@@ -24,18 +24,22 @@
  */
 package com.oracle.svm.core.heap;
 
+import static com.oracle.svm.core.Uninterruptible.CALLED_FROM_UNINTERRUPTIBLE_CODE;
+
 import java.lang.ref.Reference;
 
 import com.oracle.svm.core.IsolateArgumentParser;
 import com.oracle.svm.core.NeverInline;
 import com.oracle.svm.core.SubstrateOptions;
 import com.oracle.svm.core.SubstrateUtil;
+import com.oracle.svm.core.Uninterruptible;
 import com.oracle.svm.core.stack.StackOverflowCheck;
 import com.oracle.svm.core.util.VMError;
 
 import jdk.internal.ref.CleanerFactory;
 
 public final class ReferenceHandler {
+    @Uninterruptible(reason = CALLED_FROM_UNINTERRUPTIBLE_CODE, mayBeInlined = true)
     public static boolean useDedicatedThread() {
         int automaticReferenceHandling = IsolateArgumentParser.getOptionIndex(SubstrateOptions.ConcealedOptions.AutomaticReferenceHandling);
         return ReferenceHandlerThread.isSupported() && IsolateArgumentParser.singleton().getBooleanOptionValue(automaticReferenceHandling);

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/heap/ReferenceHandlerThread.java

@@ -41,10 +41,12 @@
 import com.oracle.svm.core.util.VMError;
 
 import jdk.graal.compiler.api.replacements.Fold;
+import jdk.graal.compiler.word.Word;
 
 public final class ReferenceHandlerThread implements Runnable {
     private final Thread thread;
     private volatile IsolateThread isolateThread;
+    private volatile boolean stopped;
 
     @Platforms(Platform.HOSTED_ONLY.class)
     ReferenceHandlerThread() {
@@ -54,24 +56,48 @@ public final class ReferenceHandlerThread implements Runnable {
     }
 
     public static void start() {
-        if (isSupported()) {
-            singleton().thread.start();
+        if (!isSupported()) {
+            return;
         }
+
+        singleton().thread.start();
+        /* Wait until the isolateThread field is initialized. */
+        while (singleton().isolateThread.isNull()) {
+            Thread.yield();
+        }
+    }
+
+    public static void initiateShutdown() {
+        if (!isSupported()) {
+            return;
+        }
+
+        singleton().stopped = true;
+        Heap.getHeap().wakeUpReferencePendingListWaiters();
+    }
+
+    @Uninterruptible(reason = "Executed during teardown after VMThreads#threadExit")
+    public static void waitUntilDetached() {
+        if (!isSupported()) {
+            return;
+        }
+
+        VMThreads.waitInNativeUntilDetached(singleton().isolateThread);
+        singleton().isolateThread = Word.nullPointer();
     }
 
     @Uninterruptible(reason = CALLED_FROM_UNINTERRUPTIBLE_CODE, mayBeInlined = true)
     public static boolean isReferenceHandlerThread() {
-        if (isSupported()) {
-            return CurrentIsolate.getCurrentThread() == singleton().isolateThread;
-        }
-        return false;
+        return isReferenceHandlerThread(CurrentIsolate.getCurrentThread());
+    }
+
+    @Uninterruptible(reason = CALLED_FROM_UNINTERRUPTIBLE_CODE, mayBeInlined = true)
+    public static boolean isReferenceHandlerThread(IsolateThread other) {
+        return isSupported() && other == singleton().isolateThread;
     }
 
     public static boolean isReferenceHandlerThread(Thread other) {
-        if (isSupported()) {
-            return other == singleton().thread;
-        }
-        return false;
+        return isSupported() && other == singleton().thread;
     }
 
     @Override
@@ -80,19 +106,13 @@ public void run() {
 
         this.isolateThread = CurrentIsolate.getCurrentThread();
         try {
-            while (true) {
+            while (!stopped) {
                 ReferenceInternals.waitForPendingReferences();
                 ReferenceInternals.processPendingReferences();
                 ReferenceHandler.processCleaners();
             }
-        } catch (InterruptedException e) {
-            VMError.guarantee(VMThreads.isTearingDown(), "Reference Handler should only be interrupted during tear-down");
         } catch (Throwable t) {
-            if (t instanceof OutOfMemoryError && VMThreads.isTearingDown()) {
-                // Likely failed to allocate the InterruptedException, ignore either way.
-            } else {
-                throw VMError.shouldNotReachHere("Reference processing and cleaners must handle all potential exceptions", t);
-            }
+            throw VMError.shouldNotReachHere("Reference processing and cleaners must handle all potential exceptions", t);
         }
     }