Skip to content

Commit

Permalink
CompilationAlarm shouldn't interfere with snippet creation
Browse files Browse the repository at this point in the history
  • Loading branch information
@tkrodriguez
tkrodriguez committed Aug 13, 2024
1 parent 1ff744c commit f289f1b
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 36 deletions.
34 changes: 22 additions & 12 deletions ...iler/src/jdk.graal.compiler/src/jdk/graal/compiler/core/common/util/CompilationAlarm.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ public static class Options {
private CompilationAlarm(double period) {
this.period = period;
this.expiration = period == 0.0D ? 0L : System.currentTimeMillis() + (long) (period * 1000);
this.previous = currentAlarm.get();
}

/**
Expand Down Expand Up @@ -98,7 +99,7 @@ public double getPeriod() {
* period} seconds, {@code false} otherwise
*/
public boolean hasExpired() {
return this != NEVER_EXPIRES && System.currentTimeMillis() > expiration;
return expiration != 0 && System.currentTimeMillis() > expiration;
}

/**
Expand All @@ -112,10 +113,7 @@ public void checkExpiration() {

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

/**
Expand All @@ -128,10 +126,15 @@ public void close() {
*/
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
Expand All @@ -146,16 +149,23 @@ public static CompilationAlarm trackCompilationPeriod(OptionValues options) {
if (Assertions.detailedAssertionsEnabled(options)) {
period *= 2;
}
CompilationAlarm current = currentAlarm.get();
if (current == null) {
current = new CompilationAlarm(period);
currentAlarm.set(current);
return current;
}
CompilationAlarm current = new CompilationAlarm(period);
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;
}

/**
* Number of graph events (iterating inputs, usages, etc) before triggering a check on the
* compilation alarm.
Expand Down
67 changes: 45 additions & 22 deletions compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/replacements/SnippetTemplate.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@
import jdk.graal.compiler.core.common.type.StampFactory;
import jdk.graal.compiler.core.common.type.StampPair;
import jdk.graal.compiler.core.common.type.TypeReference;
import jdk.graal.compiler.core.common.util.CompilationAlarm;
import jdk.graal.compiler.debug.Assertions;
import jdk.graal.compiler.debug.CounterKey;
import jdk.graal.compiler.debug.DebugCloseable;
Expand Down Expand Up @@ -523,6 +524,15 @@ protected SnippetInfo(ResolvedJavaMethod method, ResolvedJavaMethod original, Lo
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;
}
Expand Down Expand Up @@ -949,7 +959,8 @@ public SnippetTemplate template(CoreProviders context, ValueNode replacee, final
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),
Expand Down Expand Up @@ -1028,7 +1039,7 @@ protected SnippetTemplate(OptionValues options,
SnippetReflectionProvider snippetReflection,
Arguments args,
boolean trackNodeSourcePosition,
Node replacee,
ValueNode replacee,
PhaseSuite<CoreProviders> midTierPreLoweringPhases,
PhaseSuite<CoreProviders> midTierPostLoweringPhases) {
this.snippetReflection = snippetReflection;
Expand Down Expand Up @@ -1201,10 +1212,9 @@ protected SnippetTemplate(OptionValues options,
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;
}
}
Expand Down Expand Up @@ -1284,10 +1294,17 @@ protected SnippetTemplate(OptionValues options,

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);
}
Expand Down Expand Up @@ -1446,6 +1463,10 @@ protected SnippetTemplate(OptionValues options,
}
}

if (!curSideEffectNodes.isEmpty()) {
checkSideEffects(replacee);
}

debug.dump(DebugContext.INFO_LEVEL, snippet, "SnippetTemplate final state");
assert snippet.verify();
this.snippet.freeze();
Expand Down Expand Up @@ -2076,21 +2097,7 @@ public UnmodifiableEconomicMap<Node, Node> instantiate(MetaAccessProvider metaAc
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);

Expand Down Expand Up @@ -2263,6 +2270,22 @@ public static FixedNode walkBackToExceptionEdgeStart(FixedNode start) {
return null;
}

/**
* 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}.
Expand Down
3 changes: 1 addition & 2 deletions compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/virtual/phases/ea/EffectsPhase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,8 @@ public boolean runAnalysis(StructuredGraph graph, CoreProvidersT context) {
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++) {
try (DebugContext.Scope s = debug.scope(debug.areScopesEnabled() ? "iteration " + iteration : null)) {
ScheduleResult schedule;
ControlFlowGraph cfg;
Expand Down

0 comments on commit f289f1b

Please sign in to comment.