[Backport] [Oracle GraalVM] [GR-65067] Backport to 23.1: CompilationAlarm shouldn't interfere with snippet creation.

[elkorchi]

What would you like to backport?

Oracle GraalVM backported commits:

(oracle/graal@24ac40a)

Describe the motivation for the backport.

Backported to Oracle GraalVM for JDK 21, fix version: graalvm-23.1.8

[bulasevich]

It seems that cherry-picking the single commit CompilationAlarm shouldn't interfere with snippet creation does not work for CompilationAlarm.java. The history of modifications to the CompilationAlarm class is much longer on the mainline and includes:

• implement GuestGraal
• Do not keep Graph/LIR objects alive in CompilationAlarm
• reduced runtime of CompilationAlarmTest from ~165 secs to ~3 secs
• improved CompilationAlarm logging and made conversion to milliseconds more eager to reduce mixing of doubles and longs
• fix timeout reported in bailout exception thrown by CompilationAlarm
• improved Graal options
• rename jdk.compiler.graal to jdk.graal.compiler
• [GR-48937] Reset progress-dependet CompilationAlarm for jmh-whitebox microbenchmarks due to false alarms.
• address review comments
• compilation alarm: improve logging and fix tests to work properly with the delay
• compilation alarm: start detection after 10s
• compilation alarm: refactorings
• address review comments
• compilation alarm: initially dont collect stack traces, only time stamps, later also stack traces
• compilation alarm: also log counter if requested
• disable compilation progress detection for svm
• compilation alarm: disable progress detection if the user sets it to 0
• compilation alarm: also reset counter
• compilation alarm: progress detection bind to counter
• rename compilation alarm check to checkProgress
• switch to copmilation alarm progress
• endless loops: require termination argument
• compilation alarm: check in prominent methods that can inidicate endless iterations

The usage of CompilationAlarm is spread across more than forty files in the mainline. The change CompilationAlarm shouldn't interfere with snippet creation does not seem applicable without all that context.

git cherry-pick results in several conflicts in SnippetTemplate.java and ReentrantBlockIterator.java:

<<<<<<< HEAD:compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/phases/graph/ReentrantBlockIterator.java
        StructuredGraph graph = start.getBeginNode().graph();
        CompilationAlarm compilationAlarm = CompilationAlarm.current();
        while (true) {
            if (compilationAlarm.hasExpired()) {
                double period = CompilationAlarm.Options.CompilationExpirationPeriod.getValue(graph.getOptions());
                throw new PermanentBailoutException("Compilation exceeded %f seconds during CFG traversal", period);
            }
=======
        while (true) { // TERMINATION ARGUMENT: processing all blocks reverse post order until end
                       // of cfg or until a bailout is triggered because of a long compile
            CompilationAlarm.checkProgress(start.getCfg().graph);
>>>>>>> 24ac40a9474 (CompilationAlarm shouldn't interfere with snippet creation):compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/graph/ReentrantBlockIterator.java

<<<<<<< HEAD:compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/replacements/SnippetTemplate.java
                new PartialEscapePhase(true, true, canonicalizer, null, options, SchedulingStrategy.LATEST).apply(snippetCopy, providers);
            }
            if (needsCE) {
                new IterativeConditionalEliminationPhase(canonicalizer, false).apply(snippetCopy, providers);
            }
            // (2)
            try (DebugContext.Scope s = debug.scope("LoweringSnippetTemplate_HIGH_TIER", snippetCopy)) {
                new HighTierLoweringPhase(canonicalizer).apply(snippetCopy, providers);
            } catch (Throwable e) {
                throw debug.handle(e);
            }
            if (loweringStage != LoweringTool.StandardLoweringStage.HIGH_TIER) {
                // (3)
                assert !guardsStage.allowsFloatingGuards() : guardsStage;
                // only create memory map nodes if we need the memory graph
                new FloatingReadPhase(true, canonicalizer).apply(snippetCopy, providers);
                if (!snippetCopy.getGraphState().isExplicitExceptionsNoDeopt()) {
                    new GuardLoweringPhase().apply(snippetCopy, providers);
=======
                // (1)
                boolean needsPEA = false;
                boolean needsCE = false;
                LoweringTool.LoweringStage loweringStage = args.cacheKey.loweringStage;
                for (Node n : snippetCopy.getNodes()) {
                    if (!needsPEA && (n instanceof AbstractNewObjectNode || n instanceof AbstractBoxingNode)) {
                        needsPEA = true;
                    } else if (!needsCE && n instanceof LogicNode) {
                        needsCE = true;
                    }
>>>>>>> 24ac40a9474 (CompilationAlarm shouldn't interfere with snippet creation):compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/replacements/SnippetTemplate.java

<<<<<<< HEAD:compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/replacements/SnippetTemplate.java
                    if (frameState != null) {
                        stateSplit.setStateAfter(null);
=======
                    if (info.type == SnippetType.INLINED_SNIPPET) {
                        FrameState frameState = stateSplit.stateAfter();
                        if (frameState != null) {
                            stateSplit.setStateAfter(null);
                        }
                    } else {
                        // preserve the state for the late inlined snippets, they might be needed
                        // still (if they express java semantic)
>>>>>>> 24ac40a9474 (CompilationAlarm shouldn't interfere with snippet creation):compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/replacements/SnippetTemplate.java

In addition, git cherry-pick

• fails to match new CompilationAlarm.java location (it requires a manual update):
  • graal-jdk21u/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/core/common/util/CompilationAlarm.java
  • graal-oracle/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/core/common/util/CompilationAlarm.java
• and cannot apply changes to CompilationAlarmTest.java, which does not exist in the graal-jdk21u repo (simply copying it results in compilation errors):
  • graal-oracle/compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/debug/test/CompilationAlarmTest.java

If we decide to backport the change anyway, it might look like this — although I do not feel confident proposing it as a PR in this form:

commit 0d5966ad45892c3ff579d6d56e7ae68e6d9800db
Author: Tom Rodriguez <tom.rodriguez@oracle.com>
Date:   Mon Apr 10 17:16:20 2023 -0700

    CompilationAlarm shouldn't interfere with snippet creation

    (cherry picked from commit 24ac40a947461fbc26ec30b9b4c56e84da7b6137)

diff --git a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/core/common/util/CompilationAlarm.java b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/core/common/util/CompilationAlarm.java
index 2e3c01fcbaa..202c5949b44 100644
--- a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/core/common/util/CompilationAlarm.java
+++ b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/core/common/util/CompilationAlarm.java
@@ -46,6 +46,7 @@ public final class CompilationAlarm implements AutoCloseable {

     private CompilationAlarm(long expiration) {
         this.expiration = expiration;
+        this.previous = currentAlarm.get();
     }

     /**
@@ -73,14 +74,12 @@ public final class CompilationAlarm implements AutoCloseable {
      *         otherwise
      */
     public boolean hasExpired() {
-        return this != NEVER_EXPIRES && System.currentTimeMillis() > expiration;
+        return expiration != 0 && System.currentTimeMillis() > expiration;
     }

     @Override
     public void close() {
-        if (this != NEVER_EXPIRES) {
-            currentAlarm.set(null);
-        }
+        currentAlarm.set(previous);
     }

     /**
@@ -88,10 +87,15 @@ public final class CompilationAlarm implements AutoCloseable {
      */
     private final long expiration;

+    /**
+     * The previously installed alarm.
+     */
+    private final CompilationAlarm previous;
+
     /**
      * Starts an alarm for setting a time limit on a compilation if there isn't already an active
      * alarm and {@link CompilationAlarm.Options#CompilationExpirationPeriod}{@code > 0}. The
-     * returned value can be used in a try-with-resource statement to disable the alarm once the
+     * returned value should be used in a try-with-resource statement to disable the alarm once the
      * compilation is finished.
      *
      * @return a {@link CompilationAlarm} if there was no current alarm for the calling thread
@@ -106,15 +110,21 @@ public final class CompilationAlarm implements AutoCloseable {
             if (Assertions.detailedAssertionsEnabled(options)) {
                 period *= 2;
             }
-            CompilationAlarm current = currentAlarm.get();
-            if (current == null) {
-                long expiration = System.currentTimeMillis() + (long) (period * 1000);
-                current = new CompilationAlarm(expiration);
-                currentAlarm.set(current);
-                return current;
-            }
+            long expiration = System.currentTimeMillis() + (long) (period * 1000);
+            CompilationAlarm current = new CompilationAlarm(expiration);
+            currentAlarm.set(current);
+            return current;
         }
         return null;
     }

+    /**
+     * Disable the compilation alarm. The returned value should be used in a try-with-resource
+     * statement to restore the previous alarm state.
+     */
+    public static CompilationAlarm disable() {
+        CompilationAlarm current = new CompilationAlarm(0);
+        currentAlarm.set(current);
+        return current;
+    }
 }
diff --git a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/phases/graph/ReentrantBlockIterator.java b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/phases/graph/ReentrantBlockIterator.java
index fa3e87a3196..d30da479d11 100644
--- a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/phases/graph/ReentrantBlockIterator.java
+++ b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/phases/graph/ReentrantBlockIterator.java
@@ -139,10 +139,7 @@ public final class ReentrantBlockIterator {
         StructuredGraph graph = start.getBeginNode().graph();
         CompilationAlarm compilationAlarm = CompilationAlarm.current();
         while (true) {
-            if (compilationAlarm.hasExpired()) {
-                double period = CompilationAlarm.Options.CompilationExpirationPeriod.getValue(graph.getOptions());
-                throw new PermanentBailoutException("Compilation exceeded %f seconds during CFG traversal", period);
-            }
+            //CompilationAlarm.checkProgress(start.getCfg().graph);
             HIRBlock next = null;
             if (stopAtBlock != null && stopAtBlock.test(current)) {
                 states.put(current.getBeginNode(), state);
diff --git a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/replacements/SnippetTemplate.java b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/replacements/SnippetTemplate.java
index d7bfaefe2b0..7fd8b2d8203 100644
--- a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/replacements/SnippetTemplate.java
+++ b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/replacements/SnippetTemplate.java
@@ -65,6 +65,7 @@ import org.graalvm.compiler.core.common.type.Stamp;
 import org.graalvm.compiler.core.common.type.StampFactory;
 import org.graalvm.compiler.core.common.type.StampPair;
 import org.graalvm.compiler.core.common.type.TypeReference;
+import org.graalvm.compiler.core.common.util.CompilationAlarm;
 import org.graalvm.compiler.debug.Assertions;
 import org.graalvm.compiler.debug.CounterKey;
 import org.graalvm.compiler.debug.DebugCloseable;
@@ -519,6 +520,15 @@ public class SnippetTemplate {
             this.receiver = receiver;
         }

+        public boolean isPrivateLocation(LocationIdentity identity) {
+            for (LocationIdentity i : privateLocations) {
+                if (i.equals(identity)) {
+                    return true;
+                }
+            }
+            return false;
+        }
+
         public ResolvedJavaMethod getMethod() {
             return method;
         }
@@ -945,7 +955,8 @@ public class SnippetTemplate {
                 try (DebugContext debug = context.getReplacements().openSnippetDebugContext("SnippetTemplate_", args.cacheKey.method, outer, options)) {
                     try (DebugCloseable a = SnippetTemplateCreationTime.start(outer);
                                     DebugCloseable a2 = args.info.creationTimer.start(outer);
-                                    DebugContext.Scope s = debug.scope("SnippetSpecialization", args.info.method)) {
+                                    DebugContext.Scope s = debug.scope("SnippetSpecialization", args.info.method);
+                                    CompilationAlarm alarm = CompilationAlarm.disable()) {
                         SnippetTemplates.increment(outer);
                         args.info.creationCounter.increment(outer);
                         OptionValues snippetOptions = new OptionValues(options, GraalOptions.TraceInlining, GraalOptions.TraceInliningForStubsAndSnippets.getValue(options),
@@ -1023,7 +1034,7 @@ public class SnippetTemplate {
                     SnippetReflectionProvider snippetReflection,
                     Arguments args,
                     boolean trackNodeSourcePosition,
-                    Node replacee,
+                    ValueNode replacee,
                     PhaseSuite<CoreProviders> midTierPreLoweringPhases,
                     PhaseSuite<CoreProviders> midTierPostLoweringPhases) {
         this.snippetReflection = snippetReflection;
@@ -1196,10 +1207,10 @@ public class SnippetTemplate {
             boolean needsCE = false;
             LoweringTool.LoweringStage loweringStage = args.cacheKey.loweringStage;
             for (Node n : snippetCopy.getNodes()) {
-                if (n instanceof AbstractNewObjectNode || n instanceof AbstractBoxingNode) {
+                if (!needsPEA && (n instanceof AbstractNewObjectNode || n instanceof AbstractBoxingNode)) {
                     needsPEA = true;
                     break;
-                } else if (n instanceof LogicNode) {
+                } else if (!needsCE && n instanceof LogicNode) {
                     needsCE = true;
                 }
             }
@@ -1279,10 +1290,17 @@ public class SnippetTemplate {

                 if (node instanceof StateSplit) {
                     StateSplit stateSplit = (StateSplit) node;
-                    FrameState frameState = stateSplit.stateAfter();
-                    if (stateSplit.hasSideEffect()) {
+
+                    // The normal side effect check doesn't understand private locations, so
+                    // explicitly filter those accesses from the side effects.
+                    boolean hasSideEffect = stateSplit.hasSideEffect();
+                    if (hasSideEffect && stateSplit instanceof MemoryAccess && MemoryKill.isSingleMemoryKill(stateSplit.asNode())) {
+                        hasSideEffect = !info.isPrivateLocation(((SingleMemoryKill) stateSplit).getKilledLocationIdentity());
+                    }
+                    if (hasSideEffect) {
                         curSideEffectNodes.add((StateSplit) node);
                     }
+                    FrameState frameState = stateSplit.stateAfter();
                     if (frameState != null) {
                         stateSplit.setStateAfter(null);
                     }
@@ -1441,6 +1459,10 @@ public class SnippetTemplate {
                 }
             }

+            if (!curSideEffectNodes.isEmpty()) {
+                checkSideEffects(replacee);
+            }
+
             debug.dump(DebugContext.INFO_LEVEL, snippet, "SnippetTemplate final state");
             assert snippet.verify();
             this.snippet.freeze();
@@ -2044,21 +2066,7 @@ public class SnippetTemplate {
             FixedNode firstCFGNodeDuplicate = (FixedNode) duplicates.get(firstCFGNode);
             replacee.replaceAtPredecessor(firstCFGNodeDuplicate);

-            if (replacee.graph().getGuardsStage().areFrameStatesAtSideEffects()) {
-                boolean replaceeHasSideEffect = replacee instanceof StateSplit && ((StateSplit) replacee).hasSideEffect();
-                boolean replacementHasSideEffect = !sideEffectNodes.isEmpty();
-
-                /*
-                 * Following cases are allowed: Either the replacee and replacement don't have
-                 * side-effects or the replacee has and the replacement hasn't (lowered to something
-                 * without a side-effect which is fine regarding correctness) or both have
-                 * side-effects, under which conditions also merges should have states.
-                 */
-
-                if (replacementHasSideEffect) {
-                    GraalError.guarantee(replaceeHasSideEffect, "Lowering node %s without side-effect to snippet %s with sideeffects=%s", replacee, info, this.sideEffectNodes);
-                }
-            }
+            checkSideEffects(replacee);

             updateStamps(replacee, duplicates);

@@ -2191,6 +2199,22 @@ public class SnippetTemplate {
         }
     }

+    /**
+     * Check that either the replacee and replacement don't have side effects or the replacee does
+     * and the replacement doesn't (lowered to something without a side effect which is fine
+     * regarding correctness) or both have side effects, under which conditions also merges should
+     * have states.
+     */
+    private void checkSideEffects(ValueNode replacee) {
+        if (replacee.graph().getGuardsStage().areFrameStatesAtSideEffects()) {
+            boolean replaceeHasSideEffect = replacee instanceof StateSplit && ((StateSplit) replacee).hasSideEffect();
+            boolean replacementHasSideEffect = !sideEffectNodes.isEmpty();
+            if (replacementHasSideEffect) {
+                GraalError.guarantee(replaceeHasSideEffect, "Lowering node %s without side effect to snippet %s with side effects=%s", replacee, info, this.sideEffectNodes);
+            }
+        }
+    }
+
     /**
      * Searches for {@link WithExceptionNode} reachable from the {@link UnwindNode} and marks them
      * as {@linkplain WithExceptionNode#replaceWithNonThrowing() non-throwing}.
diff --git a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/virtual/phases/ea/EffectsPhase.java b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/virtual/phases/ea/EffectsPhase.java
index 4cb2d4ecff8..409a58a964b 100644
--- a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/virtual/phases/ea/EffectsPhase.java
+++ b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/virtual/phases/ea/EffectsPhase.java
@@ -31,8 +31,8 @@ import java.util.Optional;
 import org.graalvm.collections.EconomicSet;
 import org.graalvm.compiler.core.common.util.CompilationAlarm;
 import org.graalvm.compiler.debug.DebugContext;
-import org.graalvm.compiler.graph.Graph.NodeEventScope;
 import org.graalvm.compiler.graph.Graph;
+import org.graalvm.compiler.graph.Graph.NodeEventScope;
 import org.graalvm.compiler.graph.Node;
 import org.graalvm.compiler.nodes.GraphState;
 import org.graalvm.compiler.nodes.StructuredGraph;
@@ -94,9 +94,9 @@ public abstract class EffectsPhase<CoreProvidersT extends CoreProviders> extends
         LoopUtility.removeObsoleteProxies(graph, context, canonicalizer);
         assert unscheduled || strategy != null;
         boolean changed = false;
-        CompilationAlarm compilationAlarm = CompilationAlarm.current();
         DebugContext debug = graph.getDebug();
-        for (int iteration = 0; iteration < maxIterations && !compilationAlarm.hasExpired(); iteration++) {
+        for (int iteration = 0; iteration < maxIterations; iteration++) {
+            //CompilationAlarm.checkProgress(graph);
             try (DebugContext.Scope s = debug.scope(debug.areScopesEnabled() ? "iteration " + iteration : null)) {
                 ScheduleResult schedule;
                 ControlFlowGraph cfg;