Skip to content

Commit 4f61e16

Browse files
committed
[JDK-8344807] [GR-59023] Only duplicate MonitorIdNode for closed lock regions
PullRequest: graal/20024
2 parents b068e8a + 1fa877e commit 4f61e16

File tree

7 files changed

+244
-26
lines changed

7 files changed

+244
-26
lines changed

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/hotspot/HotSpotGraalCompiler.java

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@
4949
import jdk.graal.compiler.hotspot.HotSpotGraalRuntime.HotSpotGC;
5050
import jdk.graal.compiler.hotspot.meta.HotSpotProviders;
5151
import jdk.graal.compiler.hotspot.phases.OnStackReplacementPhase;
52+
import jdk.graal.compiler.hotspot.phases.VerifyLockDepthPhase;
5253
import jdk.graal.compiler.java.GraphBuilderPhase;
5354
import jdk.graal.compiler.lir.asm.CompilationResultBuilderFactory;
5455
import jdk.graal.compiler.lir.phases.LIRSuites;
@@ -380,6 +381,9 @@ protected PhaseSuite<HighTierContext> configGraphBuilderSuite(PhaseSuite<HighTie
380381
}
381382
GraphBuilderPhase newGraphBuilderPhase = new HotSpotGraphBuilderPhase(graphBuilderConfig);
382383
newGbs.findPhase(GraphBuilderPhase.class).set(newGraphBuilderPhase);
384+
if (Assertions.assertionsEnabled()) {
385+
newGbs.appendPhase(new VerifyLockDepthPhase());
386+
}
383387
if (isOSR) {
384388
newGbs.appendPhase(new OnStackReplacementPhase());
385389
}

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/hotspot/phases/OnStackReplacementPhase.java

Lines changed: 4 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@
3838
import jdk.graal.compiler.debug.DebugCloseable;
3939
import jdk.graal.compiler.debug.DebugContext;
4040
import jdk.graal.compiler.debug.GraalError;
41-
import jdk.graal.compiler.graph.Node;
4241
import jdk.graal.compiler.graph.iterators.NodeIterable;
4342
import jdk.graal.compiler.loop.phases.LoopTransformations;
4443
import jdk.graal.compiler.nodeinfo.InputType;
@@ -67,10 +66,7 @@
6766
import jdk.graal.compiler.nodes.extended.OSRMonitorEnterNode;
6867
import jdk.graal.compiler.nodes.extended.OSRStartNode;
6968
import jdk.graal.compiler.nodes.extended.ObjectIsArrayNode;
70-
import jdk.graal.compiler.nodes.java.AccessMonitorNode;
7169
import jdk.graal.compiler.nodes.java.InstanceOfNode;
72-
import jdk.graal.compiler.nodes.java.MonitorEnterNode;
73-
import jdk.graal.compiler.nodes.java.MonitorExitNode;
7470
import jdk.graal.compiler.nodes.java.MonitorIdNode;
7571
import jdk.graal.compiler.nodes.loop.Loop;
7672
import jdk.graal.compiler.nodes.loop.LoopsData;
@@ -253,12 +249,6 @@ protected void run(StructuredGraph graph, CoreProviders providers) {
253249
ValueNode lockedObject = osrState.lockAt(i);
254250
OSRMonitorEnterNode osrMonitorEnter = graph.add(new OSRMonitorEnterNode(lockedObject, id));
255251
osrMonitorEnter.setStateAfter(osrStart.stateAfter());
256-
for (Node usage : id.usages()) {
257-
if (usage instanceof AccessMonitorNode) {
258-
AccessMonitorNode access = (AccessMonitorNode) usage;
259-
access.setObject(lockedObject);
260-
}
261-
}
262252
FixedNode oldNext = osrStart.next();
263253
oldNext.replaceAtPredecessor(null);
264254
osrMonitorEnter.setNext(oldNext);
@@ -267,19 +257,10 @@ protected void run(StructuredGraph graph, CoreProviders providers) {
267257
}
268258

269259
debug.dump(DebugContext.DETAILED_LEVEL, graph, "After inserting OSR monitor enters");
270-
/*
271-
* Ensure balanced monitorenter - monitorexit
272-
*
273-
* Ensure that there is no monitor exit without a monitor enter in the graph. If there
274-
* is one this can only be done by bytecode as we have the monitor enter before the OSR
275-
* loop but the exit in a path of the loop that must be under a condition, else it will
276-
* throw an IllegalStateException anyway in the 2.iteration
277-
*/
278-
for (MonitorExitNode exit : graph.getNodes(MonitorExitNode.TYPE)) {
279-
MonitorIdNode id = exit.getMonitorId();
280-
if (id.usages().filter(MonitorEnterNode.class).count() != 1) {
281-
throw new PermanentBailoutException("Unbalanced monitor enter-exit in OSR compilation with locks. Object is locked before the loop but released inside the loop.");
282-
}
260+
try {
261+
new VerifyLockDepthPhase().run(graph);
262+
} catch (VerifyLockDepthPhase.LockStructureError e) {
263+
throw new PermanentBailoutException("Unbalanced monitor enter-exit in OSR compilation with locks: " + e.getMessage());
283264
}
284265
}
285266
debug.dump(DebugContext.DETAILED_LEVEL, graph, "OnStackReplacement result");
Lines changed: 200 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,200 @@
1+
/*
2+
* Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.
3+
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
4+
*
5+
* This code is free software; you can redistribute it and/or modify it
6+
* under the terms of the GNU General Public License version 2 only, as
7+
* published by the Free Software Foundation. Oracle designates this
8+
* particular file as subject to the "Classpath" exception as provided
9+
* by Oracle in the LICENSE file that accompanied this code.
10+
*
11+
* This code is distributed in the hope that it will be useful, but WITHOUT
12+
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
13+
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
14+
* version 2 for more details (a copy is included in the LICENSE file that
15+
* accompanied this code).
16+
*
17+
* You should have received a copy of the GNU General Public License version
18+
* 2 along with this work; if not, write to the Free Software Foundation,
19+
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
20+
*
21+
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
22+
* or visit www.oracle.com if you need additional information or have any
23+
* questions.
24+
*/
25+
package jdk.graal.compiler.hotspot.phases;
26+
27+
import java.util.ArrayList;
28+
import java.util.List;
29+
import java.util.Optional;
30+
31+
import jdk.graal.compiler.debug.GraalError;
32+
import jdk.graal.compiler.nodes.AbstractMergeNode;
33+
import jdk.graal.compiler.nodes.DeoptimizingNode;
34+
import jdk.graal.compiler.nodes.FixedNode;
35+
import jdk.graal.compiler.nodes.FrameState;
36+
import jdk.graal.compiler.nodes.GraphState;
37+
import jdk.graal.compiler.nodes.StructuredGraph;
38+
import jdk.graal.compiler.nodes.extended.OSRMonitorEnterNode;
39+
import jdk.graal.compiler.nodes.extended.OSRStartNode;
40+
import jdk.graal.compiler.nodes.java.AccessMonitorNode;
41+
import jdk.graal.compiler.nodes.java.MonitorEnterNode;
42+
import jdk.graal.compiler.nodes.java.MonitorExitNode;
43+
import jdk.graal.compiler.nodes.java.MonitorIdNode;
44+
import jdk.graal.compiler.phases.Phase;
45+
import jdk.graal.compiler.phases.graph.MergeableState;
46+
import jdk.graal.compiler.phases.graph.PostOrderNodeIterator;
47+
48+
/**
49+
* Ensure that the lock depths and {@link MonitorIdNode ids} agree with the enter and exits.
50+
*/
51+
public class VerifyLockDepthPhase extends Phase {
52+
53+
@Override
54+
public Optional<NotApplicable> notApplicableTo(GraphState graphState) {
55+
/*
56+
* The current algorithm is only correct before PEA because it creates virtual locks, which
57+
* complicates checking for the correct correspondence because the FrameState can be out of
58+
* sync with the actual locking operations.
59+
*/
60+
return NotApplicable.unlessRunBefore(this, GraphState.StageFlag.PARTIAL_ESCAPE, graphState);
61+
}
62+
63+
@Override
64+
public boolean shouldApply(StructuredGraph graph) {
65+
return graph.getNodes(MonitorIdNode.TYPE).isNotEmpty();
66+
}
67+
68+
@Override
69+
protected void run(StructuredGraph graph) {
70+
// The current algorithm is only correct before PEA because it might virtual locks, which
71+
// complicates the matching.
72+
VerifyLockDepths verify = new VerifyLockDepths(graph.start());
73+
verify.apply();
74+
}
75+
76+
public static class LockStructureError extends RuntimeException {
77+
private static final long serialVersionUID = 1L;
78+
79+
LockStructureError(String message, Object... args) {
80+
super(String.format(message, args));
81+
}
82+
}
83+
84+
static class VerifyLockDepths extends PostOrderNodeIterator<VerifyLockDepths.LockingState> {
85+
VerifyLockDepths(FixedNode start) {
86+
super(start, null);
87+
state = new LockingState();
88+
}
89+
90+
/**
91+
* The current state of locks which are held.
92+
*/
93+
static class LockingState extends MergeableState<LockingState> implements Cloneable {
94+
final ArrayList<MonitorIdNode> locks;
95+
96+
LockingState() {
97+
locks = new ArrayList<>();
98+
}
99+
100+
LockingState(LockingState state) {
101+
this.locks = new ArrayList<>(state.locks);
102+
}
103+
104+
public void pop(MonitorExitNode exit) {
105+
MonitorIdNode id = exit.getMonitorId();
106+
if (locks.isEmpty()) {
107+
throw new LockStructureError("%s: lock stack is empty at", exit);
108+
}
109+
MonitorIdNode top = locks.removeLast();
110+
if (top != id) {
111+
throw new LockStructureError(top + " != " + id);
112+
}
113+
}
114+
115+
public void push(MonitorEnterNode enter) {
116+
MonitorIdNode id = enter.getMonitorId();
117+
if (locks.size() != id.getLockDepth()) {
118+
throw new LockStructureError("%s: lock depth mismatch %d != %d", enter, locks.size(), id.getLockDepth());
119+
}
120+
locks.add(id);
121+
}
122+
123+
@Override
124+
public LockingState clone() {
125+
return new LockingState(this);
126+
}
127+
128+
@Override
129+
public boolean merge(AbstractMergeNode merge, List<LockingState> withStates) {
130+
for (LockingState other : withStates) {
131+
if (!locks.equals(other.locks)) {
132+
throw new LockStructureError("%s: lock depth mismatch %d != %d", merge, locks, other.locks);
133+
}
134+
}
135+
return true;
136+
}
137+
138+
@Override
139+
public String toString() {
140+
return "LockingState{" + locks + '}';
141+
}
142+
143+
public void verifyState(FixedNode forNode, FrameState frameState) {
144+
if (frameState == null) {
145+
return;
146+
}
147+
if (forNode instanceof OSRStartNode || forNode instanceof OSRMonitorEnterNode) {
148+
/*
149+
* Ignore the FrameState on these nodes as the FrameState on the OSRStartNode is
150+
* used to construct the OSRMonitorEnterNodes which come after the start so
151+
* there's always an apparent mismatch. These nodes all the have same state and
152+
* it's only truly valid for the very last OSRMonitorEnterNode.
153+
*/
154+
return;
155+
}
156+
if (locks.size() != frameState.nestedLockDepth()) {
157+
throw new LockStructureError("Wrong number of locks held at %s: %d != %d", forNode, locks.size(), frameState.nestedLockDepth());
158+
}
159+
FrameState current = frameState;
160+
int depth = locks.size();
161+
while (current != null) {
162+
depth -= current.locksSize();
163+
for (int i = 0; i < current.locksSize(); i++) {
164+
MonitorIdNode frameId = locks.get(depth + i);
165+
MonitorIdNode stackId = current.monitorIdAt(i);
166+
if (frameId != stackId) {
167+
throw new LockStructureError("%s: mismatched lock at depth % in %s: %s != %s", forNode, i, current, frameId, stackId);
168+
}
169+
}
170+
current = current.outerFrameState();
171+
}
172+
}
173+
}
174+
175+
@Override
176+
protected void node(FixedNode node) {
177+
if (node instanceof DeoptimizingNode.DeoptBefore) {
178+
DeoptimizingNode.DeoptBefore before = (DeoptimizingNode.DeoptBefore) node;
179+
state.verifyState(before.asFixedNode(), before.stateBefore());
180+
}
181+
if (node instanceof DeoptimizingNode.DeoptDuring) {
182+
DeoptimizingNode.DeoptDuring during = (DeoptimizingNode.DeoptDuring) node;
183+
state.verifyState(during.asFixedNode(), during.stateDuring());
184+
}
185+
if (node instanceof MonitorEnterNode) {
186+
MonitorEnterNode enter = (MonitorEnterNode) node;
187+
state.push(enter);
188+
state.verifyState(node, enter.stateAfter());
189+
} else if (node instanceof MonitorExitNode) {
190+
state.pop((MonitorExitNode) node);
191+
} else if (node instanceof AccessMonitorNode) {
192+
throw GraalError.shouldNotReachHere(node.getClass().toString());
193+
}
194+
if (node instanceof DeoptimizingNode.DeoptAfter) {
195+
DeoptimizingNode.DeoptAfter after = (DeoptimizingNode.DeoptAfter) node;
196+
state.verifyState(after.asFixedNode(), after.stateAfter());
197+
}
198+
}
199+
}
200+
}

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/GraphState.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -608,6 +608,7 @@ public enum StageFlag {
608608
CANONICALIZATION,
609609
/* Stages applied by high tier. */
610610
LOOP_OVERFLOWS_CHECKED,
611+
PARTIAL_ESCAPE,
611612
FINAL_PARTIAL_ESCAPE,
612613
HIGH_TIER_LOWERING,
613614
/* Stages applied by mid tier. */

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/loop/LoopFragment.java

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@
6262
import jdk.graal.compiler.nodes.cfg.ControlFlowGraph;
6363
import jdk.graal.compiler.nodes.cfg.HIRBlock;
6464
import jdk.graal.compiler.nodes.java.MonitorEnterNode;
65+
import jdk.graal.compiler.nodes.java.MonitorIdNode;
6566
import jdk.graal.compiler.nodes.spi.NodeWithState;
6667
import jdk.graal.compiler.nodes.util.GraphUtil;
6768
import jdk.graal.compiler.nodes.virtual.CommitAllocationNode;
@@ -296,6 +297,7 @@ protected static void computeNodes(NodeBitMap nodes, Graph graph, Loop loop, Ite
296297

297298
final NodeBitMap nonLoopNodes = graph.createNodeBitMap();
298299
WorkQueue worklist = new WorkQueue(graph);
300+
ArrayList<MonitorIdNode> ids = new ArrayList<>();
299301
for (AbstractBeginNode b : blocks) {
300302
if (b.isDeleted()) {
301303
continue;
@@ -308,7 +310,7 @@ protected static void computeNodes(NodeBitMap nodes, Graph graph, Loop loop, Ite
308310
}
309311
}
310312
if (n instanceof MonitorEnterNode) {
311-
markFloating(worklist, loop, ((MonitorEnterNode) n).getMonitorId(), nodes, nonLoopNodes);
313+
ids.add(((MonitorEnterNode) n).getMonitorId());
312314
}
313315
if (n instanceof AbstractMergeNode) {
314316
/*
@@ -326,6 +328,26 @@ protected static void computeNodes(NodeBitMap nodes, Graph graph, Loop loop, Ite
326328
}
327329
}
328330
}
331+
/*
332+
* Mark the MonitorIdNodes as part of the loop if all uses are within the loop. This will
333+
* cause them to be duplicated when the body is cloned. This makes it possible for lock
334+
* optimizations to identify independent lock regions since it relies on visiting the users
335+
* of a MonitorIdNode to find the monitor nodes for a lock region.
336+
*/
337+
boolean mark = true;
338+
outer: for (MonitorIdNode id : ids) {
339+
for (Node use : id.usages()) {
340+
if (!nodes.contains(use)) {
341+
mark = false;
342+
break outer;
343+
}
344+
}
345+
}
346+
if (mark) {
347+
for (MonitorIdNode id : ids) {
348+
markFloating(worklist, loop, id, nodes, nonLoopNodes);
349+
}
350+
}
329351
}
330352

331353
static class WorkListEntry {

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/common/LockEliminationPhase.java

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626

2727
import java.util.Optional;
2828

29-
import jdk.graal.compiler.debug.Assertions;
3029
import jdk.graal.compiler.debug.GraalError;
3130
import jdk.graal.compiler.graph.Node;
3231
import jdk.graal.compiler.nodes.FixedNode;
@@ -69,7 +68,7 @@ protected void run(StructuredGraph graph) {
6968
if ((next instanceof MonitorEnterNode)) {
7069
// should never happen, osr monitor enters are always direct successors of the graph
7170
// start
72-
assert !(next instanceof OSRMonitorEnterNode) : Assertions.errorMessageContext("next", next);
71+
GraalError.guarantee(!(next instanceof OSRMonitorEnterNode), "OSRMonitorEnterNode can't be seen here: %s", next);
7372
AccessMonitorNode monitorEnterNode = (AccessMonitorNode) next;
7473
if (isCompatibleLock(monitorEnterNode, monitorExitNode, true, cfg)) {
7574
/*

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/virtual/phases/ea/PartialEscapePhase.java

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
import java.util.Optional;
3131

3232
import org.graalvm.collections.EconomicSet;
33+
3334
import jdk.graal.compiler.debug.DebugCloseable;
3435
import jdk.graal.compiler.graph.Node;
3536
import jdk.graal.compiler.nodes.GraphState;
@@ -132,6 +133,16 @@ public Optional<NotApplicable> notApplicableTo(GraphState graphState) {
132133
cleanupPhase != null ? cleanupPhase.notApplicableTo(graphState) : ALWAYS_APPLICABLE);
133134
}
134135

136+
@Override
137+
public void updateGraphState(GraphState graphState) {
138+
super.updateGraphState(graphState);
139+
// This may be set more than once but the goal is to record whether PartialEscapePhase has
140+
// even been run.
141+
if (!graphState.isAfterStage(StageFlag.PARTIAL_ESCAPE)) {
142+
graphState.setAfterStage(StageFlag.PARTIAL_ESCAPE);
143+
}
144+
}
145+
135146
@Override
136147
@SuppressWarnings("try")
137148
protected void run(StructuredGraph graph, CoreProviders context) {

0 commit comments

Comments
 (0)