Skip to content

Commit e175b0e

Browse files
davleopodavleopo
committed
ristretto: address review comments
1 parent f10d4c4 commit e175b0e

File tree

5 files changed

+50
-63
lines changed

5 files changed

+50
-63
lines changed

‎substratevm/src/com.oracle.svm.interpreter.metadata/src/com/oracle/svm/interpreter/metadata/profile/MethodProfile.java‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -65,9 +65,6 @@ public final class MethodProfile {
6565

6666
private final ResolvedJavaMethod method;
6767

68-
/**
69-
* See {@link #isMature()}.
70-
*/
7168
private boolean isMature;
7269

7370
public MethodProfile(ResolvedJavaMethod method) {

‎substratevm/src/com.oracle.svm.interpreter/src/com/oracle/svm/interpreter/Interpreter.java‎

Lines changed: 20 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -886,17 +886,14 @@ private static Object executeBodyFromBCI(InterpreterFrame frame, InterpreterReso
886886
case IFGE: // fall through
887887
case IFGT: // fall through
888888
case IFLE:
889-
if (takeBranchPrimitive1(popInt(frame, top - 1), curOpcode)) {
890-
if (methodProfile != null) {
891-
methodProfile.profileBranch(curBCI, true);
892-
}
889+
final boolean branchTaken1 = takeBranchPrimitive1(popInt(frame, top - 1), curOpcode);
890+
if (methodProfile != null) {
891+
methodProfile.profileBranch(curBCI, branchTaken1);
892+
}
893+
if (branchTaken1) {
893894
top += ConstantBytecodes.stackEffectOf(IFLE);
894895
curBCI = beforeJumpChecks(frame, curBCI, BytecodeStream.readBranchDest2(code, curBCI), top);
895896
continue loop;
896-
} else {
897-
if (methodProfile != null) {
898-
methodProfile.profileBranch(curBCI, false);
899-
}
900897
}
901898
break;
902899

@@ -906,49 +903,40 @@ private static Object executeBodyFromBCI(InterpreterFrame frame, InterpreterReso
906903
case IF_ICMPGE: // fall through
907904
case IF_ICMPGT: // fall through
908905
case IF_ICMPLE:
909-
if (takeBranchPrimitive2(popInt(frame, top - 1), popInt(frame, top - 2), curOpcode)) {
910-
if (methodProfile != null) {
911-
methodProfile.profileBranch(curBCI, true);
912-
}
906+
final boolean branchTaken2 = takeBranchPrimitive2(popInt(frame, top - 1), popInt(frame, top - 2), curOpcode);
907+
if (methodProfile != null) {
908+
methodProfile.profileBranch(curBCI, branchTaken2);
909+
}
910+
if (branchTaken2) {
913911
top += ConstantBytecodes.stackEffectOf(IF_ICMPLE);
914912
curBCI = beforeJumpChecks(frame, curBCI, BytecodeStream.readBranchDest2(code, curBCI), top);
915913
continue loop;
916-
} else {
917-
if (methodProfile != null) {
918-
methodProfile.profileBranch(curBCI, false);
919-
}
920914
}
921915
break;
922916

923917
case IF_ACMPEQ: // fall through
924918
case IF_ACMPNE:
925-
if (takeBranchRef2(popObject(frame, top - 1), popObject(frame, top - 2), curOpcode)) {
926-
if (methodProfile != null) {
927-
methodProfile.profileBranch(curBCI, true);
928-
}
919+
final boolean branchTakenRef2 = takeBranchRef2(popObject(frame, top - 1), popObject(frame, top - 2), curOpcode);
920+
if (methodProfile != null) {
921+
methodProfile.profileBranch(curBCI, branchTakenRef2);
922+
}
923+
if (branchTakenRef2) {
929924
top += ConstantBytecodes.stackEffectOf(IF_ACMPNE);
930925
curBCI = beforeJumpChecks(frame, curBCI, BytecodeStream.readBranchDest2(code, curBCI), top);
931926
continue loop;
932-
} else {
933-
if (methodProfile != null) {
934-
methodProfile.profileBranch(curBCI, false);
935-
}
936927
}
937928
break;
938929

939930
case IFNULL: // fall through
940931
case IFNONNULL:
941-
if (takeBranchRef1(popObject(frame, top - 1), curOpcode)) {
942-
if (methodProfile != null) {
943-
methodProfile.profileBranch(curBCI, true);
944-
}
932+
final boolean branchTakenRef1 = takeBranchRef1(popObject(frame, top - 1), curOpcode);
933+
if (methodProfile != null) {
934+
methodProfile.profileBranch(curBCI, branchTakenRef1);
935+
}
936+
if (branchTakenRef1) {
945937
top += ConstantBytecodes.stackEffectOf(IFNONNULL);
946938
curBCI = beforeJumpChecks(frame, curBCI, BytecodeStream.readBranchDest2(code, curBCI), top);
947939
continue loop;
948-
} else {
949-
if (methodProfile != null) {
950-
methodProfile.profileBranch(curBCI, false);
951-
}
952940
}
953941
break;
954942

‎substratevm/src/com.oracle.svm.interpreter/src/com/oracle/svm/interpreter/ristretto/meta/RistrettoMethod.java‎

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -100,24 +100,25 @@ public static RistrettoMethod create(InterpreterResolvedJavaMethod interpreterMe
100100

101101
public MethodProfile getProfile() {
102102
if (profile == null) {
103-
/*
104-
* Allocate the profile once per method. Apart from test scenarios the profile is never
105-
* set to null again. Thus, the heavy locking code below is normally not run in a fast
106-
* path.
107-
*/
108-
synchronized (this) {
109-
if (profile == null) {
110-
MethodProfile newProfile = new MethodProfile(this);
111-
// ensure everything is allocated and initialized before we signal the barrier
112-
// for the publishing write
113-
MembarNode.memoryBarrier(MembarNode.FenceKind.STORE_STORE);
114-
profile = newProfile;
115-
}
116-
}
103+
initializedProfile();
117104
}
118105
return profile;
119106
}
120107

108+
/**
109+
* Allocate the profile once per method. Apart from test scenarios the profile is never set to
110+
* null again. Thus, the heavy locking code below is normally not run in a fast path.
111+
*/
112+
private synchronized void initializedProfile() {
113+
if (profile == null) {
114+
MethodProfile newProfile = new MethodProfile(this);
115+
// ensure everything is allocated and initialized before we signal the barrier
116+
// for the publishing write
117+
MembarNode.memoryBarrier(MembarNode.FenceKind.STORE_STORE);
118+
profile = newProfile;
119+
}
120+
}
121+
121122
public synchronized void resetProfile() {
122123
profile = null;
123124
}

‎substratevm/src/com.oracle.svm.interpreter/src/com/oracle/svm/interpreter/ristretto/profile/RistrettoProfileSupport.java‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -117,12 +117,12 @@ public static MethodProfile profileMethodEntry(InterpreterResolvedJavaMethod iMe
117117
* happens on most of the frequently executed cases here.
118118
*/
119119
switch (oldState) {
120-
/*
121-
* profile has not been initialized yet for this method, do so by switching to
122-
* INITIALIZING and then wait, if another thread went to initializing in the
123-
* meantime we are done
124-
*/
125120
case RistrettoConstants.COMPILE_STATE_INIT_VAL: {
121+
/*
122+
* The profile has not been initialized yet for this method. Do so by switching
123+
* to INITIALIZING. If another thread transitioned to initializing in the
124+
* meantime we are done.
125+
*/
126126
methodEntryInitCase(iMethod, rMethod);
127127
break;
128128
}
@@ -190,9 +190,8 @@ private static void methodEntryInitCase(InterpreterResolvedJavaMethod iMethod, R
190190
if (COMPILATION_STATE_UPDATER.compareAndSet(rMethod, RistrettoConstants.COMPILE_STATE_INIT_VAL, RistrettoConstants.COMPILE_STATE_INITIALIZING)) {
191191
trace(RistrettoRuntimeOptions.JITTraceCompilationQueuing, "[Ristretto Compile Queue]Entering state %s for %s%n",
192192
RistrettoCompileStateMachine.toString(COMPILATION_STATE_UPDATER.get(rMethod)), iMethod);
193-
while (!COMPILATION_STATE_UPDATER.compareAndSet(rMethod, RistrettoConstants.COMPILE_STATE_INITIALIZING, RistrettoConstants.COMPILE_STATE_NEVER_COMPILED)) {
194-
// spin until we are done writing
195-
PauseNode.pause();
193+
if (!COMPILATION_STATE_UPDATER.compareAndSet(rMethod, RistrettoConstants.COMPILE_STATE_INITIALIZING, RistrettoConstants.COMPILE_STATE_NEVER_COMPILED)) {
194+
throw GraalError.shouldNotReachHere("We set transition to COMPILE_STATE_INITIALIZING, we must be allowed to set it to COMPILE_STATE_NEVER_COMPILED");
196195
}
197196
// continue to the NEVER_COMPILED state
198197
trace(RistrettoRuntimeOptions.JITTraceCompilationQueuing, "[Ristretto Compile Queue]Finished setting state %s for %s%n",

‎substratevm/src/com.oracle.svm.interpreter/src/com/oracle/svm/interpreter/ristretto/profile/RistrettoProfilingInfo.java‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,11 @@
3535
import jdk.vm.ci.meta.TriState;
3636

3737
public class RistrettoProfilingInfo implements ProfilingInfo {
38-
private final MethodProfile methodProfile;
38+
3939
private static final double PROB_DELTA = 1e-6;
4040

41+
private final MethodProfile methodProfile;
42+
4143
public RistrettoProfilingInfo(MethodProfile methodProfile) {
4244
this.methodProfile = methodProfile;
4345
}
@@ -119,11 +121,6 @@ public boolean isMature() {
119121
return methodProfile.isMature() || methodProfile.getProfileEntryCount() > RistrettoRuntimeOptions.JITProfileMatureInvocationThreshold.getValue();
120122
}
121123

122-
@Override
123-
public String toString() {
124-
return "RistrettoProfilingInfo<" + this.toString(null, "; ") + ">";
125-
}
126-
127124
@Override
128125
public void setMature() {
129126
methodProfile.setMature(true);
@@ -138,4 +135,9 @@ public boolean setCompilerIRSize(Class<?> irType, int nodeCount) {
138135
public int getCompilerIRSize(Class<?> irType) {
139136
return -1;
140137
}
138+
139+
@Override
140+
public String toString() {
141+
return "RistrettoProfilingInfo<" + this.toString(null, "; ") + ">";
142+
}
141143
}

0 commit comments

Comments
 (0)