[GR-68917] Fix engine shutdown to also cancel currently active compilations; Avoid waiting for compilations on native-image isolate tear down whenever possible.

PullRequest: graal/21919

Author: chumer

compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/truffle/test/PolyglotEngineOptionsTest.java

@@ -24,7 +24,8 @@
  */
 package jdk.graal.compiler.truffle.test;
 
-import com.oracle.truffle.sl.runtime.SLStrings;
+import java.util.ArrayList;
+import java.util.List;
 
 import org.graalvm.options.OptionDescriptor;
 import org.graalvm.options.OptionStability;
@@ -38,6 +39,7 @@
 import com.oracle.truffle.runtime.OptimizedRuntimeOptions;
 import com.oracle.truffle.sl.runtime.SLContext;
 import com.oracle.truffle.sl.runtime.SLFunction;
+import com.oracle.truffle.sl.runtime.SLStrings;
 
 public class PolyglotEngineOptionsTest extends TestWithSynchronousCompiling {
 
@@ -51,6 +53,9 @@ public void testVisibleOptions() {
 
     @Test
     public void testCompilationThreshold() {
+        // cleanup gets in the way with recursive contexts
+        this.automaticCleanup = false;
+
         // does not work with a different inline cache size.
         Assert.assertEquals(2, SLFunction.INLINE_CACHE_SIZE);
 
@@ -96,9 +101,16 @@ public void testParseUnknownMode() {
     }
 
     private void testCompilationThreshold(int iterations, String compilationThresholdOption, Runnable doWhile) {
-        Context ctx = setupContext(compilationThresholdOption == null ? new String[]{"engine.MultiTier", "false"}
-                        : new String[]{"engine.SingleTierCompilationThreshold", compilationThresholdOption, "engine.MultiTier", "false"});
+        List<String> args = new ArrayList<>();
+        args.add("engine.MultiTier");
+        args.add("false");
+        if (compilationThresholdOption != null) {
+            args.add("engine.SingleTierCompilationThreshold");
+            args.add(compilationThresholdOption);
+        }
+        Context ctx = setupContext(args.toArray(String[]::new));
         ctx.eval("sl", "function test() {}");
+
         SLFunction test = SLContext.get(null).getFunctionRegistry().getFunction(SLStrings.fromJavaString("test"));
 
         Assert.assertFalse(isExecuteCompiled(test));
@@ -114,6 +126,7 @@ private void testCompilationThreshold(int iterations, String compilationThreshol
         Assert.assertTrue(isExecuteCompiled(test));
         test.getCallTarget().call();
         Assert.assertTrue(isExecuteCompiled(test));
+        cleanup();
     }
 
     private static boolean isExecuteCompiled(SLFunction value) {

compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/truffle/test/RewriteDuringCompilationTest.java

@@ -195,7 +195,7 @@ private void testCompilation(FrameDescriptor.Builder builder, BaseNode testedCod
                     throws IOException, InterruptedException, ExecutionException {
         // DetectInvalidCodeNode.invalidTwice does not work with multi-tier
         // code can remain active of another tier with local invalidation.
-        setupEnv(Context.newBuilder().allowExperimentalOptions(true).option("engine.MultiTier", "false"), new ProxyLanguage() {
+        setupEnv(Context.newBuilder().allowExperimentalOptions(true).option("engine.MultiTier", "false").option("engine.MaximumCompilations", "-1"), new ProxyLanguage() {
             private CallTarget target;
 
             @Override

compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/truffle/test/TestWithPolyglotOptions.java

@@ -37,6 +37,7 @@
 public abstract class TestWithPolyglotOptions {
 
     private Context activeContext;
+    protected boolean automaticCleanup = true;
 
     @After
     public final void cleanup() {
@@ -70,7 +71,9 @@ protected final Context setupContext(String... keyValuePairs) {
     }
 
     protected final Context setupContext(Context.Builder builder) {
-        cleanup();
+        if (automaticCleanup) {
+            cleanup();
+        }
         Context newContext = builder.build();
         newContext.enter();
         activeContext = newContext;

substratevm/src/com.oracle.svm.truffle/src/com/oracle/svm/truffle/api/SubstrateTruffleCompiler.java

@@ -41,6 +41,9 @@ public interface SubstrateTruffleCompiler extends TruffleCompiler {
 
     /**
      * Called on tear-down of the current isolate.
+     *
+     * @param shutdownCompilationsAndWaitAction action to perform before a compiler isolate would
+     *            shut down to cancel work that can delay the shutdown.
      */
-    void teardown();
+    void teardown(Runnable shutdownCompilationsAndWaitAction);
 }

substratevm/src/com.oracle.svm.truffle/src/com/oracle/svm/truffle/api/SubstrateTruffleCompilerImpl.java

@@ -110,7 +110,7 @@ protected void parseGraalOptions(String[] options, EconomicMap<OptionKey<?>, Obj
     }
 
     @Override
-    public void teardown() {
+    public void teardown(Runnable shutdownCompilationsAndWaitAction) {
     }
 
     @Override

substratevm/src/com.oracle.svm.truffle/src/com/oracle/svm/truffle/api/SubstrateTruffleRuntime.java

@@ -103,6 +103,7 @@ static boolean isMultiThreaded() {
          */
         return SubstrateTruffleOptions.TruffleMultiThreaded.getValue();
     }
+
 }
 
 public final class SubstrateTruffleRuntime extends OptimizedTruffleRuntime {
@@ -166,7 +167,7 @@ private void initializeAtRuntime(OptimizedCallTarget callTarget) {
             Deoptimizer.Options.TraceDeoptimization.update(true);
         }
         installDefaultListeners();
-        RuntimeSupport.getRuntimeSupport().addTearDownHook(isFirstIsolate -> teardown());
+        RuntimeSupport.getRuntimeSupport().addTearDownHook(_ -> teardownCompilerIsolate());
     }
 
     @Override
@@ -198,16 +199,16 @@ public ConstantFieldInfo getConstantFieldInfo(ResolvedJavaField field) {
         return super.getConstantFieldInfo(field);
     }
 
-    private void teardown() {
-        long timeout = SubstrateUtil.assertionsEnabled() ? DEBUG_TEAR_DOWN_TIMEOUT : PRODUCTION_TEAR_DOWN_TIMEOUT;
-        BackgroundCompileQueue queue = getCompileQueue();
-        if (queue != null) {
-            queue.shutdownAndAwaitTermination(timeout);
-        }
-
+    private void teardownCompilerIsolate() {
         TruffleCompiler tcp = truffleCompiler;
         if (tcp != null) {
-            ((SubstrateTruffleCompiler) tcp).teardown();
+            ((SubstrateTruffleCompiler) tcp).teardown(() -> {
+                long timeout = SubstrateUtil.assertionsEnabled() ? DEBUG_TEAR_DOWN_TIMEOUT : PRODUCTION_TEAR_DOWN_TIMEOUT;
+                BackgroundCompileQueue queue = getCompileQueue();
+                if (queue != null) {
+                    queue.shutdownAndAwaitTermination(timeout);
+                }
+            });
         }
     }
 

substratevm/src/com.oracle.svm.truffle/src/com/oracle/svm/truffle/isolated/IsolateAwareTruffleCompiler.java

@@ -229,9 +229,31 @@ private static void copyEncodedOptions(@SuppressWarnings("unused") @CEntryPoint.
     }
 
     @Override
-    public void teardown() {
+    public void teardown(Runnable shutdownCompilationsAndWaitAction) {
         if (SubstrateOptions.shouldCompileInIsolates()) {
-            tearDownIsolateOnShutdown();
+            /*
+             * Start a separate thread for shutting down any ongoing compilations. If we are in a
+             * process with a single Truffle runtime (e.g. a language launcher), it can {@linkplain
+             * System#exit exit} early without waiting for this thread to finish. If there are
+             * potentially multiple isolates in the current process, the thread will delay the
+             * shutdown of the runtime isolate until the compilation isolate has been fully torn
+             * down.
+             */
+            Thread t = new Thread(() -> {
+                shutdownCompilationsAndWaitAction.run();
+                tearDownIsolateOnShutdown();
+            });
+            // we ignore uncaught exceptions here. For example
+            // if the waiting thread was interrupted.
+            t.setUncaughtExceptionHandler((_, _) -> {
+            });
+            /*
+             * Technically this does not need to be a daemon thread, as either way this thread is
+             * still being waited on when tearing down the isolate. However it seems more correct to
+             * mark this thread as daemon as it should not prevent the regular JVM shutdown.
+             */
+            t.setDaemon(true);
+            t.start();
         }
     }
 

truffle/src/com.oracle.truffle.api/src/com/oracle/truffle/api/impl/Accessor.java

@@ -831,6 +831,7 @@ public abstract <T, G> Iterator<T> mergeHostGuestFrames(Object polyglotEngine, S
         public abstract DispatchOutputStream getEngineOut(Object engine);
 
         public abstract InputStream getEngineIn(Object engine);
+
     }
 
     public abstract static class LanguageSupport extends Support {
@@ -1285,7 +1286,7 @@ public final FrameExtensions getFrameExtensionsUnsafe() {
         @SuppressWarnings({"unchecked"})
         public abstract <T> T unsafeCast(Object value, Class<T> type, boolean condition, boolean nonNull, boolean exact);
 
-        public abstract void flushCompileQueue(Object runtimeData);
+        public abstract void shutdownCompilationForEngine(Object engineData);
 
         public abstract Object createRuntimeData(Object engine, OptionValues engineOptions, Function<String, TruffleLogger> loggerFactory, SandboxPolicy sandboxPolicy);
 

truffle/src/com.oracle.truffle.api/src/com/oracle/truffle/api/impl/DefaultRuntimeAccessor.java

@@ -212,7 +212,7 @@ public Object[] castArrayFixedLength(Object[] args, int length) {
         }
 
         @Override
-        public void flushCompileQueue(Object runtimeData) {
+        public void shutdownCompilationForEngine(Object runtimeData) {
             // default runtime has no compile queue.
         }
 

truffle/src/com.oracle.truffle.polyglot/src/com/oracle/truffle/polyglot/PolyglotEngineImpl.java

@@ -1366,6 +1366,11 @@ void ensureClosed(boolean force, boolean initiatedByContext) {
 
         synchronized (this.lock) {
             closed = true;
+            // We want to flush as early as possible to make sure we start cancelling fast.
+            // this also makes sure we are no longer accepting any compilations from this engine
+            if (runtimeData != null) {
+                EngineAccessor.RUNTIME.shutdownCompilationForEngine(runtimeData);
+            }
             closingThread = null;
             this.lock.notifyAll();
             if (!activeSystemThreads.isEmpty()) {
@@ -1409,10 +1414,6 @@ void ensureClosed(boolean force, boolean initiatedByContext) {
             }
 
             polyglotHostService.notifyEngineClosed(this, force);
-
-            if (runtimeData != null) {
-                EngineAccessor.RUNTIME.flushCompileQueue(runtimeData);
-            }
             getAPIAccess().engineClosed(weakAPI);
         }
     }