Skip to content

Commit d7db489

Browse files
teshullteshull
committed
Track extra metadata about DeoptEntry/DeoptProxyAnchor nodes and more aggressively remove them.
1 parent 9f016aa commit d7db489

File tree

9 files changed

+266
-116
lines changed

9 files changed

+266
-116
lines changed

‎compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/java/BciBlockMapping.java‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -624,6 +624,12 @@ public boolean isOutOfBounds() {
624624
}
625625
}
626626

627+
/**
628+
* Represents an entry into an exception handler routine. This block is needed to ensure there
629+
* is a place to put checks that determine whether the handler should be entered (i.e., the
630+
* exception kind is compatible) or execution should continue to check the next handler (or
631+
* unwind).
632+
*/
627633
public static class ExceptionDispatchBlock extends BciBlock {
628634
public final ExceptionHandler handler;
629635
public final int deoptBci;
@@ -1351,7 +1357,7 @@ private void addSuccessor(int predBci, BciBlock sux) {
13511357
}
13521358

13531359
/**
1354-
* Logic for adding an the "normal" invoke successor link.
1360+
* Logic for adding the "normal" invoke successor link.
13551361
*/
13561362
protected void addInvokeNormalSuccessor(int invokeBci, BciBlock sux) {
13571363
addSuccessor(invokeBci, sux);

‎substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/graal/nodes/DeoptEntryNode.java‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,8 @@
4343

4444
import com.oracle.svm.core.graal.lir.DeoptEntryOp;
4545

46+
import jdk.vm.ci.code.BytecodeFrame;
47+
4648
/**
4749
* A landing-pad for deoptimization. This node is generated in deoptimization target methods for all
4850
* deoptimization entry points.
@@ -53,8 +55,20 @@ public final class DeoptEntryNode extends WithExceptionNode implements DeoptEntr
5355

5456
@OptionalInput(InputType.State) protected FrameState stateAfter;
5557

56-
public DeoptEntryNode() {
58+
private final int proxifiedInvokeBci;
59+
60+
protected DeoptEntryNode(int proxifiedInvokeBci) {
5761
super(TYPE, StampFactory.forVoid());
62+
this.proxifiedInvokeBci = proxifiedInvokeBci;
63+
}
64+
65+
public static DeoptEntryNode create(int proxifiedInvokeBci) {
66+
assert proxifiedInvokeBci != BytecodeFrame.UNKNOWN_BCI;
67+
return new DeoptEntryNode(proxifiedInvokeBci);
68+
}
69+
70+
public static DeoptEntryNode create() {
71+
return new DeoptEntryNode(BytecodeFrame.UNKNOWN_BCI);
5872
}
5973

6074
@Override
@@ -99,4 +113,9 @@ public void setStateAfter(FrameState x) {
99113
public boolean hasSideEffect() {
100114
return true;
101115
}
116+
117+
@Override
118+
public int getProxifiedInvokeBci() {
119+
return proxifiedInvokeBci;
120+
}
102121
}

‎substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/graal/nodes/DeoptEntrySupport.java‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,4 +30,12 @@
3030
import org.graalvm.compiler.nodes.spi.LIRLowerable;
3131

3232
public interface DeoptEntrySupport extends LIRLowerable, ControlFlowAnchored, FixedNodeInterface, StateSplit {
33+
34+
/**
35+
* Returns the invoke bci this deopt is serving as a proxy anchor for or
36+
* {@link jdk.vm.ci.code.BytecodeFrame#UNKNOWN_BCI} if it is not serving as a proxy anchor for
37+
* an invoke. Note this will be different than the entry's stateafter bci since this node will
38+
* be after the invoke it is "proxifying".
39+
*/
40+
int getProxifiedInvokeBci();
3341
}

‎substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/graal/nodes/DeoptProxyAnchorNode.java‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -41,16 +41,22 @@
4141
public class DeoptProxyAnchorNode extends AbstractStateSplit implements DeoptEntrySupport {
4242
public static final NodeClass<DeoptProxyAnchorNode> TYPE = NodeClass.create(DeoptProxyAnchorNode.class);
4343

44-
public DeoptProxyAnchorNode() {
45-
this(TYPE);
46-
}
44+
private final int proxifiedInvokeBci;
45+
46+
public DeoptProxyAnchorNode(int proxifiedInvokeBci) {
47+
super(TYPE, StampFactory.forVoid());
48+
assert proxifiedInvokeBci >= 0 : "DeoptProxyAnchorNode should be proxing an invoke";
4749

48-
protected DeoptProxyAnchorNode(NodeClass<? extends DeoptProxyAnchorNode> c) {
49-
super(c, StampFactory.forVoid());
50+
this.proxifiedInvokeBci = proxifiedInvokeBci;
5051
}
5152

5253
@Override
5354
public void generate(NodeLIRBuilderTool gen) {
5455
/* No-op */
5556
}
57+
58+
@Override
59+
public int getProxifiedInvokeBci() {
60+
return proxifiedInvokeBci;
61+
}
5662
}

‎substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/graal/replacements/SubstrateGraphKit.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,7 @@ public ValueNode createCFunctionCall(ValueNode targetAddress, List<ValueNode> ar
218218
* exception handlers and directly unwind.
219219
*/
220220
int bci = invoke.stateAfter().bci;
221-
appendWithUnwind(new DeoptEntryNode(), bci);
221+
appendWithUnwind(DeoptEntryNode.create(invoke.bci()), bci);
222222
}
223223

224224
ValueNode result = invoke;

‎substratevm/src/com.oracle.svm.graal/src/com/oracle/svm/graal/hosted/ParseOnceRuntimeCompilationFeature.java‎

Lines changed: 67 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@
6767
import org.graalvm.compiler.nodes.graphbuilderconf.InlineInvokePlugin;
6868
import org.graalvm.compiler.nodes.graphbuilderconf.IntrinsicContext;
6969
import org.graalvm.compiler.nodes.graphbuilderconf.NodePlugin;
70+
import org.graalvm.compiler.nodes.java.ExceptionObjectNode;
7071
import org.graalvm.compiler.options.Option;
7172
import org.graalvm.compiler.options.OptionValues;
7273
import org.graalvm.compiler.phases.OptimisticOptimizations;
@@ -101,6 +102,8 @@
101102
import com.oracle.svm.core.ParsingReason;
102103
import com.oracle.svm.core.config.ConfigurationValues;
103104
import com.oracle.svm.core.graal.nodes.DeoptEntryNode;
105+
import com.oracle.svm.core.graal.nodes.DeoptEntrySupport;
106+
import com.oracle.svm.core.graal.nodes.DeoptProxyAnchorNode;
104107
import com.oracle.svm.core.graal.nodes.InlinedInvokeArgumentsNode;
105108
import com.oracle.svm.core.graal.stackvalue.StackValueNode;
106109
import com.oracle.svm.core.option.HostedOptionKey;
@@ -126,6 +129,7 @@
126129
import com.oracle.svm.hosted.phases.InlineBeforeAnalysisPolicyUtils.AlwaysInlineScope;
127130
import com.oracle.svm.hosted.phases.StrengthenStampsPhase;
128131

132+
import jdk.vm.ci.code.BytecodeFrame;
129133
import jdk.vm.ci.code.BytecodePosition;
130134
import jdk.vm.ci.meta.JavaKind;
131135
import jdk.vm.ci.meta.ResolvedJavaMethod;
@@ -138,12 +142,8 @@
138142
public class ParseOnceRuntimeCompilationFeature extends RuntimeCompilationFeature implements Feature, RuntimeCompilationSupport {
139143

140144
public static class Options {
141-
/*
142-
* Note this phase is currently overly aggressive and can illegally remove proxies. This
143-
* will be fixed in GR-44459.
144-
*/
145145
@Option(help = "Remove Deopt(Entries,Anchors,Proxies) determined to be unneeded after the runtime compiled graphs have been finalized.")//
146-
public static final HostedOptionKey<Boolean> RemoveUnneededDeoptSupport = new HostedOptionKey<>(false);
146+
public static final HostedOptionKey<Boolean> RemoveUnneededDeoptSupport = new HostedOptionKey<>(true);
147147

148148
@Option(help = "Perform InlineBeforeAnalysis on runtime compiled methods")//
149149
public static final HostedOptionKey<Boolean> RuntimeCompilationInlineBeforeAnalysis = new HostedOptionKey<>(true);
@@ -1216,58 +1216,103 @@ public boolean insertPlaceholderParamAndReturnFlows(MultiMethod.MultiMethodKey m
12161216
}
12171217

12181218
/**
1219-
* Removes Deoptimizations Entrypoints which are deemed to be unnecessary after the runtime
1220-
* compilation methods are optimized.
1219+
* Removes {@link DeoptEntryNode}s, {@link DeoptProxyAnchorNode}s, and {@link DeoptProxyNode}s
1220+
* which are determined to be unnecessary after the runtime compilation methods are optimized.
12211221
*/
12221222
static class RemoveUnneededDeoptSupport extends Phase {
1223+
enum RemovalDecision {
1224+
KEEP,
1225+
PROXIFY,
1226+
REMOVE
1227+
}
12231228

12241229
@Override
12251230
protected void run(StructuredGraph graph) {
1226-
EconomicMap<StateSplit, Boolean> decisionCache = EconomicMap.create();
1231+
EconomicMap<StateSplit, RemovalDecision> decisionCache = EconomicMap.create();
12271232

12281233
// First go through and delete all unneeded proxies
12291234
for (DeoptProxyNode proxyNode : graph.getNodes(DeoptProxyNode.TYPE).snapshot()) {
12301235
ValueNode proxyPoint = proxyNode.getProxyPoint();
12311236
if (proxyPoint instanceof StateSplit) {
1232-
if (proxyPoint instanceof DeoptEntryNode && shouldRemove((StateSplit) proxyPoint, decisionCache)) {
1237+
if (getDecision((StateSplit) proxyPoint, decisionCache) == RemovalDecision.REMOVE) {
12331238
proxyNode.replaceAtAllUsages(proxyNode.getOriginalNode(), true);
12341239
proxyNode.safeDelete();
12351240
}
12361241
}
12371242
}
12381243

1239-
// Next remove all unneeded DeoptEntryNodes
1244+
// Next, remove all unneeded DeoptEntryNodes
12401245
for (DeoptEntryNode deoptEntry : graph.getNodes().filter(DeoptEntryNode.class).snapshot()) {
1241-
if (shouldRemove(deoptEntry, decisionCache)) {
1242-
deoptEntry.killExceptionEdge();
1243-
graph.removeSplit(deoptEntry, deoptEntry.getPrimarySuccessor());
1246+
switch (getDecision(deoptEntry, decisionCache)) {
1247+
case REMOVE -> {
1248+
deoptEntry.killExceptionEdge();
1249+
graph.removeSplit(deoptEntry, deoptEntry.getPrimarySuccessor());
1250+
}
1251+
case PROXIFY -> {
1252+
deoptEntry.killExceptionEdge();
1253+
DeoptProxyAnchorNode newAnchor = graph.add(new DeoptProxyAnchorNode(deoptEntry.getProxifiedInvokeBci()));
1254+
newAnchor.setStateAfter(deoptEntry.stateAfter());
1255+
graph.replaceSplitWithFixed(deoptEntry, newAnchor, deoptEntry.getPrimarySuccessor());
1256+
}
1257+
}
1258+
}
1259+
1260+
// Finally, remove all unneeded DeoptProxyAnchorNodes
1261+
for (DeoptProxyAnchorNode proxyAnchor : graph.getNodes().filter(DeoptProxyAnchorNode.class).snapshot()) {
1262+
if (getDecision(proxyAnchor, decisionCache) == RemovalDecision.REMOVE) {
1263+
graph.removeFixed(proxyAnchor);
12441264
}
12451265
}
12461266
}
12471267

1248-
boolean shouldRemove(StateSplit node, EconomicMap<StateSplit, Boolean> decisionCache) {
1249-
Boolean cached = decisionCache.get(node);
1268+
RemovalDecision getDecision(StateSplit node, EconomicMap<StateSplit, RemovalDecision> decisionCache) {
1269+
RemovalDecision cached = decisionCache.get(node);
12501270
if (cached != null) {
12511271
return cached;
12521272
}
12531273

1274+
DeoptEntrySupport proxyNode;
1275+
if (node instanceof ExceptionObjectNode exceptionObject) {
1276+
/*
1277+
* For the exception edge of a DeoptEntryNode, we insert the proxies on the
1278+
* exception object.
1279+
*/
1280+
proxyNode = (DeoptEntrySupport) exceptionObject.predecessor();
1281+
} else {
1282+
proxyNode = (DeoptEntrySupport) node;
1283+
}
1284+
1285+
RemovalDecision decision = RemovalDecision.REMOVE;
12541286
var directive = SubstrateCompilationDirectives.singleton();
1255-
FrameState state = node.stateAfter();
1287+
FrameState state = proxyNode.stateAfter();
12561288
HostedMethod method = (HostedMethod) state.getMethod();
1289+
if (proxyNode instanceof DeoptEntryNode) {
1290+
if (directive.isDeoptEntry(method, state.bci, state.duringCall(), state.rethrowException())) {
1291+
// must keep all deopt entries which are still guarding nodes
1292+
decision = RemovalDecision.KEEP;
1293+
}
1294+
}
12571295

1258-
boolean result = true;
1259-
if (directive.isRegisteredDeoptTarget(method)) {
1260-
result = !directive.isDeoptEntry(method, state.bci, state.duringCall(), state.rethrowException());
1296+
if (decision == RemovalDecision.REMOVE) {
1297+
// now check for any implicit deopt entry being protected against
1298+
int proxifiedInvokeBci = proxyNode.getProxifiedInvokeBci();
1299+
if (proxifiedInvokeBci != BytecodeFrame.UNKNOWN_BCI && directive.isDeoptEntry(method, proxifiedInvokeBci, true, false)) {
1300+
// must keep still keep a proxy for nodes which are "proxifying" an invoke
1301+
decision = proxyNode instanceof DeoptEntryNode ? RemovalDecision.PROXIFY : RemovalDecision.KEEP;
1302+
}
12611303
}
12621304

12631305
// cache the decision
1264-
decisionCache.put(node, result);
1265-
return result;
1306+
decisionCache.put(node, decision);
1307+
if (proxyNode != node) {
1308+
decisionCache.put(proxyNode, decision);
1309+
}
1310+
return decision;
12661311
}
12671312

12681313
@Override
12691314
public CharSequence getName() {
1270-
return "RemoveDeoptEntries";
1315+
return "RemoveUnneededDeoptSupport";
12711316
}
12721317
}
12731318
}

‎substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/heap/PodFactorySubstitutionMethod.java‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,7 @@ private static int startMethod(HostedGraphKit kit, DeoptInfoProvider deoptInfo,
174174
if (deoptInfo != null) {
175175
FrameState initialState = kit.getGraph().start().stateAfter();
176176
if (shouldInsertDeoptEntry(deoptInfo, initialState.bci, false, false)) {
177-
return appendDeoptWithExceptionUnwind(kit, initialState, initialState.bci, nextDeoptIndex);
177+
return appendDeoptWithExceptionUnwind(kit, initialState, initialState.bci, nextDeoptIndex, DeoptEntryNode.create());
178178
}
179179
}
180180
return nextDeoptIndex;
@@ -223,7 +223,7 @@ private static int invokeWithDeoptAndExceptionUnwind(HostedGraphKit kit, DeoptIn
223223
if (shouldInsertDeoptEntry(deoptInfo, bci, false, true)) {
224224
// Exception during invoke
225225

226-
var exceptionDeopt = kit.add(new DeoptEntryNode());
226+
var exceptionDeopt = kit.add(DeoptEntryNode.create(invoke.bci()));
227227
exceptionDeopt.setStateAfter(exception.stateAfter().duplicate());
228228
var exceptionDeoptBegin = kit.add(new DeoptEntryBeginNode());
229229
int exceptionDeoptIndex = nextDeoptIndex++;
@@ -246,10 +246,10 @@ private static int invokeWithDeoptAndExceptionUnwind(HostedGraphKit kit, DeoptIn
246246
kit.noExceptionPart();
247247
if (needDeoptEntry) {
248248
// Deopt entry after invoke without exception
249-
nextDeoptIndex = appendDeoptWithExceptionUnwind(kit, invoke.stateAfter(), invoke.stateAfter().bci, nextDeoptIndex);
249+
nextDeoptIndex = appendDeoptWithExceptionUnwind(kit, invoke.stateAfter(), invoke.stateAfter().bci, nextDeoptIndex, DeoptEntryNode.create(invoke.bci()));
250250
} else {
251251
// Only a proxy is needed
252-
nextDeoptIndex = appendDeoptProxyAnchorNode(kit, invoke.stateAfter(), nextDeoptIndex);
252+
nextDeoptIndex = appendDeoptProxyAnchorNode(kit, invoke.stateAfter(), nextDeoptIndex, invoke.bci());
253253
}
254254
}
255255
kit.endInvokeWithException();
@@ -258,8 +258,8 @@ private static int invokeWithDeoptAndExceptionUnwind(HostedGraphKit kit, DeoptIn
258258
}
259259

260260
/** @see com.oracle.svm.hosted.phases.SharedGraphBuilderPhase */
261-
private static int appendDeoptWithExceptionUnwind(HostedGraphKit kit, FrameState state, int exceptionBci, int nextDeoptIndex) {
262-
var entry = kit.add(new DeoptEntryNode());
261+
private static int appendDeoptWithExceptionUnwind(HostedGraphKit kit, FrameState state, int exceptionBci, int nextDeoptIndex, DeoptEntryNode deoptEntryNode) {
262+
var entry = kit.add(deoptEntryNode);
263263
entry.setStateAfter(state.duplicate());
264264
var begin = kit.append(new DeoptEntryBeginNode());
265265
((FixedWithNextNode) begin.predecessor()).setNext(entry);
@@ -280,8 +280,8 @@ private static int appendDeoptWithExceptionUnwind(HostedGraphKit kit, FrameState
280280
}
281281

282282
/** @see com.oracle.svm.hosted.phases.SharedGraphBuilderPhase */
283-
private static int appendDeoptProxyAnchorNode(HostedGraphKit kit, FrameState state, int nextDeoptIndex) {
284-
var anchor = kit.append(new DeoptProxyAnchorNode());
283+
private static int appendDeoptProxyAnchorNode(HostedGraphKit kit, FrameState state, int nextDeoptIndex, int invokeBci) {
284+
var anchor = kit.append(new DeoptProxyAnchorNode(invokeBci));
285285
anchor.setStateAfter(state.duplicate());
286286

287287
// Ensure later nodes see values from potential deoptimization

0 commit comments

Comments
 (0)