Skip to content

Commit 9000acd

Browse files
aclementaclement
committed
Rework compilation of OpNE/OpEQ SpEL operators
For SPR-14863 we need to adjust the code generation for OpNE to use !x.equals(y) rather than x!=y. There are also further cases in the equalityCheck() code in Operator that were not being handled in the compilation case (when comparators are used for example). This latter issue also affects OpEQ. Rather than add yet more bytecode generation, both OpNE and OpEQ generateCode() methods have been simplified. The generated code now delegates to equalityCheck() in Operator which is exactly what the interpreted case does. This ensures that the compiled code continues to behave just like the interpreted case. It ensures changes to the interpreted case are automatically picked up for the compiled case. It makes the bytecode generation simpler. The benefit of compilation of SpEL expressions is to avoid slow reflective calls - that doesn't apply for a basic (in)equality test so there is no need to go crazy in bytecode gen. Issue: SPR-14863
1 parent 99cacaa commit 9000acd

File tree

5 files changed

+196
-136
lines changed

5 files changed

+196
-136
lines changed

‎spring-expression/src/main/java/org/springframework/expression/spel/CodeFlow.java‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,15 @@ public void loadTarget(MethodVisitor mv) {
9999
mv.visitVarInsn(ALOAD, 1);
100100
}
101101

102+
/**
103+
* Push the bytecode to load the EvaluationContext (the second parameter passed to
104+
* the compiled expression method).
105+
* @param mv the visitor into which the load instruction should be inserted
106+
*/
107+
public void loadEvaluationContext(MethodVisitor mv) {
108+
mv.visitVarInsn(ALOAD, 2);
109+
}
110+
102111
/**
103112
* Record the descriptor for the most recently evaluated expression element.
104113
* @param descriptor type descriptor for most recently evaluated element

‎spring-expression/src/main/java/org/springframework/expression/spel/ast/OpEQ.java‎

Lines changed: 29 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2014 the original author or authors.
2+
* Copyright 2002-2016 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -16,8 +16,8 @@
1616

1717
package org.springframework.expression.spel.ast;
1818

19-
import org.springframework.asm.Label;
2019
import org.springframework.asm.MethodVisitor;
20+
import org.springframework.expression.EvaluationContext;
2121
import org.springframework.expression.EvaluationException;
2222
import org.springframework.expression.spel.CodeFlow;
2323
import org.springframework.expression.spel.ExpressionState;
@@ -36,112 +36,61 @@ public OpEQ(int pos, SpelNodeImpl... operands) {
3636
this.exitTypeDescriptor = "Z";
3737
}
3838

39-
4039
@Override
41-
public BooleanTypedValue getValueInternal(ExpressionState state) throws EvaluationException {
40+
public BooleanTypedValue getValueInternal(ExpressionState state)
41+
throws EvaluationException {
4242
Object left = getLeftOperand().getValueInternal(state).getValue();
4343
Object right = getRightOperand().getValueInternal(state).getValue();
4444
this.leftActualDescriptor = CodeFlow.toDescriptorFromObject(left);
4545
this.rightActualDescriptor = CodeFlow.toDescriptorFromObject(right);
46-
return BooleanTypedValue.forValue(equalityCheck(state, left, right));
46+
return BooleanTypedValue.forValue(
47+
equalityCheck(state.getEvaluationContext(), left, right));
4748
}
48-
49+
4950
// This check is different to the one in the other numeric operators (OpLt/etc)
5051
// because it allows for simple object comparison
5152
@Override
5253
public boolean isCompilable() {
5354
SpelNodeImpl left = getLeftOperand();
54-
SpelNodeImpl right= getRightOperand();
55+
SpelNodeImpl right = getRightOperand();
5556
if (!left.isCompilable() || !right.isCompilable()) {
5657
return false;
5758
}
5859

5960
String leftDesc = left.exitTypeDescriptor;
6061
String rightDesc = right.exitTypeDescriptor;
61-
DescriptorComparison dc = DescriptorComparison.checkNumericCompatibility(
62-
leftDesc, rightDesc, this.leftActualDescriptor, this.rightActualDescriptor);
62+
DescriptorComparison dc = DescriptorComparison.checkNumericCompatibility(leftDesc,
63+
rightDesc, this.leftActualDescriptor, this.rightActualDescriptor);
6364
return (!dc.areNumbers || dc.areCompatible);
6465
}
65-
66-
66+
6767
@Override
6868
public void generateCode(MethodVisitor mv, CodeFlow cf) {
69+
cf.loadEvaluationContext(mv);
6970
String leftDesc = getLeftOperand().exitTypeDescriptor;
7071
String rightDesc = getRightOperand().exitTypeDescriptor;
71-
Label elseTarget = new Label();
72-
Label endOfIf = new Label();
7372
boolean leftPrim = CodeFlow.isPrimitive(leftDesc);
7473
boolean rightPrim = CodeFlow.isPrimitive(rightDesc);
7574

76-
DescriptorComparison dc = DescriptorComparison.checkNumericCompatibility(
77-
leftDesc, rightDesc, this.leftActualDescriptor, this.rightActualDescriptor);
78-
79-
if (dc.areNumbers && dc.areCompatible) {
80-
char targetType = dc.compatibleType;
81-
getLeftOperand().generateCode(mv, cf);
82-
if (!leftPrim) {
83-
CodeFlow.insertUnboxInsns(mv, targetType, leftDesc);
84-
}
85-
cf.enterCompilationScope();
86-
getRightOperand().generateCode(mv, cf);
87-
cf.exitCompilationScope();
88-
if (!rightPrim) {
89-
CodeFlow.insertUnboxInsns(mv, targetType, rightDesc);
90-
}
91-
// assert: SpelCompiler.boxingCompatible(leftDesc, rightDesc)
92-
if (targetType == 'D') {
93-
mv.visitInsn(DCMPL);
94-
mv.visitJumpInsn(IFNE, elseTarget);
95-
}
96-
else if (targetType == 'F') {
97-
mv.visitInsn(FCMPL);
98-
mv.visitJumpInsn(IFNE, elseTarget);
99-
}
100-
else if (targetType == 'J') {
101-
mv.visitInsn(LCMP);
102-
mv.visitJumpInsn(IFNE, elseTarget);
103-
}
104-
else if (targetType == 'I' || targetType == 'Z') {
105-
mv.visitJumpInsn(IF_ICMPNE, elseTarget);
106-
}
107-
else {
108-
throw new IllegalStateException("Unexpected descriptor " + leftDesc);
109-
}
75+
cf.enterCompilationScope();
76+
getLeftOperand().generateCode(mv, cf);
77+
cf.exitCompilationScope();
78+
if (leftPrim) {
79+
CodeFlow.insertBoxIfNecessary(mv, leftDesc.charAt(0));
11080
}
111-
else {
112-
getLeftOperand().generateCode(mv, cf);
113-
if (leftPrim) {
114-
CodeFlow.insertBoxIfNecessary(mv, leftDesc.charAt(0));
115-
}
116-
getRightOperand().generateCode(mv, cf);
117-
if (rightPrim) {
118-
CodeFlow.insertBoxIfNecessary(mv, rightDesc.charAt(0));
119-
}
120-
Label leftNotNull = new Label();
121-
mv.visitInsn(DUP_X1); // dup right on the top of the stack
122-
mv.visitJumpInsn(IFNONNULL, leftNotNull);
123-
// Right is null!
124-
mv.visitInsn(SWAP);
125-
mv.visitInsn(POP); // remove it
126-
Label rightNotNull = new Label();
127-
mv.visitJumpInsn(IFNONNULL, rightNotNull);
128-
// Left is null too
129-
mv.visitInsn(ICONST_1);
130-
mv.visitJumpInsn(GOTO, endOfIf);
131-
mv.visitLabel(rightNotNull);
132-
mv.visitInsn(ICONST_0);
133-
mv.visitJumpInsn(GOTO, endOfIf);
134-
mv.visitLabel(leftNotNull);
135-
mv.visitMethodInsn(INVOKEVIRTUAL, "java/lang/Object", "equals", "(Ljava/lang/Object;)Z", false);
136-
mv.visitLabel(endOfIf);
137-
cf.pushDescriptor("Z");
138-
return;
81+
cf.enterCompilationScope();
82+
getRightOperand().generateCode(mv, cf);
83+
cf.exitCompilationScope();
84+
if (rightPrim) {
85+
CodeFlow.insertBoxIfNecessary(mv, rightDesc.charAt(0));
13986
}
140-
mv.visitInsn(ICONST_1);
141-
mv.visitJumpInsn(GOTO, endOfIf);
142-
mv.visitLabel(elseTarget);
143-
mv.visitInsn(ICONST_0);
144-
mv.visitLabel(endOfIf);
87+
88+
String operatorClassName = Operator.class.getName().replace('.', '/');
89+
String evaluationContextClassName = EvaluationContext.class.getName().replace('.',
90+
'/');
91+
mv.visitMethodInsn(INVOKESTATIC, operatorClassName, "equalityCheck", "(L"
92+
+ evaluationContextClassName + ";Ljava/lang/Object;Ljava/lang/Object;)Z",
93+
false);
14594
cf.pushDescriptor("Z");
14695
}
14796

‎spring-expression/src/main/java/org/springframework/expression/spel/ast/OpNE.java‎

Lines changed: 36 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
import org.springframework.asm.Label;
2020
import org.springframework.asm.MethodVisitor;
21+
import org.springframework.expression.EvaluationContext;
2122
import org.springframework.expression.EvaluationException;
2223
import org.springframework.expression.spel.CodeFlow;
2324
import org.springframework.expression.spel.ExpressionState;
@@ -36,87 +37,72 @@ public OpNE(int pos, SpelNodeImpl... operands) {
3637
this.exitTypeDescriptor = "Z";
3738
}
3839

39-
4040
@Override
41-
public BooleanTypedValue getValueInternal(ExpressionState state) throws EvaluationException {
41+
public BooleanTypedValue getValueInternal(ExpressionState state)
42+
throws EvaluationException {
4243
Object left = getLeftOperand().getValueInternal(state).getValue();
4344
Object right = getRightOperand().getValueInternal(state).getValue();
4445
this.leftActualDescriptor = CodeFlow.toDescriptorFromObject(left);
4546
this.rightActualDescriptor = CodeFlow.toDescriptorFromObject(right);
46-
return BooleanTypedValue.forValue(!equalityCheck(state, left, right));
47+
return BooleanTypedValue.forValue(
48+
!equalityCheck(state.getEvaluationContext(), left, right));
4749
}
4850

4951
// This check is different to the one in the other numeric operators (OpLt/etc)
5052
// because we allow simple object comparison
5153
@Override
5254
public boolean isCompilable() {
5355
SpelNodeImpl left = getLeftOperand();
54-
SpelNodeImpl right= getRightOperand();
56+
SpelNodeImpl right = getRightOperand();
5557
if (!left.isCompilable() || !right.isCompilable()) {
5658
return false;
5759
}
5860

5961
String leftDesc = left.exitTypeDescriptor;
6062
String rightDesc = right.exitTypeDescriptor;
61-
DescriptorComparison dc = DescriptorComparison.checkNumericCompatibility(
62-
leftDesc, rightDesc, this.leftActualDescriptor, this.rightActualDescriptor);
63+
DescriptorComparison dc = DescriptorComparison.checkNumericCompatibility(leftDesc,
64+
rightDesc, this.leftActualDescriptor, this.rightActualDescriptor);
6365
return (!dc.areNumbers || dc.areCompatible);
6466
}
65-
67+
6668
@Override
6769
public void generateCode(MethodVisitor mv, CodeFlow cf) {
70+
cf.loadEvaluationContext(mv);
6871
String leftDesc = getLeftOperand().exitTypeDescriptor;
6972
String rightDesc = getRightOperand().exitTypeDescriptor;
70-
Label elseTarget = new Label();
71-
Label endOfIf = new Label();
7273
boolean leftPrim = CodeFlow.isPrimitive(leftDesc);
7374
boolean rightPrim = CodeFlow.isPrimitive(rightDesc);
7475

75-
DescriptorComparison dc = DescriptorComparison.checkNumericCompatibility(
76-
leftDesc, rightDesc, this.leftActualDescriptor, this.rightActualDescriptor);
77-
78-
if (dc.areNumbers && dc.areCompatible) {
79-
char targetType = dc.compatibleType;
80-
getLeftOperand().generateCode(mv, cf);
81-
if (!leftPrim) {
82-
CodeFlow.insertUnboxInsns(mv, targetType, leftDesc);
83-
}
84-
cf.enterCompilationScope();
85-
getRightOperand().generateCode(mv, cf);
86-
cf.exitCompilationScope();
87-
if (!rightPrim) {
88-
CodeFlow.insertUnboxInsns(mv, targetType, rightDesc);
89-
}
90-
// assert: SpelCompiler.boxingCompatible(leftDesc, rightDesc)
91-
if (targetType == 'D') {
92-
mv.visitInsn(DCMPL);
93-
mv.visitJumpInsn(IFEQ, elseTarget);
94-
}
95-
else if (targetType == 'F') {
96-
mv.visitInsn(FCMPL);
97-
mv.visitJumpInsn(IFEQ, elseTarget);
98-
}
99-
else if (targetType == 'J') {
100-
mv.visitInsn(LCMP);
101-
mv.visitJumpInsn(IFEQ, elseTarget);
102-
}
103-
else if (targetType == 'I' || targetType == 'Z') {
104-
mv.visitJumpInsn(IF_ICMPEQ, elseTarget);
105-
}
106-
else {
107-
throw new IllegalStateException("Unexpected descriptor " + leftDesc);
108-
}
76+
cf.enterCompilationScope();
77+
getLeftOperand().generateCode(mv, cf);
78+
cf.exitCompilationScope();
79+
if (leftPrim) {
80+
CodeFlow.insertBoxIfNecessary(mv, leftDesc.charAt(0));
10981
}
110-
else {
111-
getLeftOperand().generateCode(mv, cf);
112-
getRightOperand().generateCode(mv, cf);
113-
mv.visitJumpInsn(IF_ACMPEQ, elseTarget);
82+
cf.enterCompilationScope();
83+
getRightOperand().generateCode(mv, cf);
84+
cf.exitCompilationScope();
85+
if (rightPrim) {
86+
CodeFlow.insertBoxIfNecessary(mv, rightDesc.charAt(0));
11487
}
88+
89+
String operatorClassName = Operator.class.getName().replace('.', '/');
90+
String evaluationContextClassName = EvaluationContext.class.getName().replace('.',
91+
'/');
92+
mv.visitMethodInsn(INVOKESTATIC, operatorClassName, "equalityCheck", "(L"
93+
+ evaluationContextClassName + ";Ljava/lang/Object;Ljava/lang/Object;)Z",
94+
false);
95+
96+
// Invert the boolean
97+
Label notZero = new Label();
98+
Label end = new Label();
99+
mv.visitJumpInsn(IFNE, notZero);
115100
mv.visitInsn(ICONST_1);
116-
mv.visitJumpInsn(GOTO,endOfIf);
117-
mv.visitLabel(elseTarget);
101+
mv.visitJumpInsn(GOTO, end);
102+
mv.visitLabel(notZero);
118103
mv.visitInsn(ICONST_0);
119-
mv.visitLabel(endOfIf);
104+
mv.visitLabel(end);
105+
120106
cf.pushDescriptor("Z");
121107
}
122108

‎spring-expression/src/main/java/org/springframework/expression/spel/ast/Operator.java‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,8 @@
2121

2222
import org.springframework.asm.Label;
2323
import org.springframework.asm.MethodVisitor;
24+
import org.springframework.expression.EvaluationContext;
2425
import org.springframework.expression.spel.CodeFlow;
25-
import org.springframework.expression.spel.ExpressionState;
2626
import org.springframework.util.ClassUtils;
2727
import org.springframework.util.NumberUtils;
2828
import org.springframework.util.ObjectUtils;
@@ -114,7 +114,9 @@ protected void generateComparisonCode(MethodVisitor mv, CodeFlow cf, int compIns
114114
leftDesc, rightDesc, this.leftActualDescriptor, this.rightActualDescriptor);
115115
char targetType = dc.compatibleType; // CodeFlow.toPrimitiveTargetDesc(leftDesc);
116116

117+
cf.enterCompilationScope();
117118
getLeftOperand().generateCode(mv, cf);
119+
cf.exitCompilationScope();
118120
if (unboxLeft) {
119121
CodeFlow.insertUnboxInsns(mv, targetType, leftDesc);
120122
}
@@ -157,7 +159,7 @@ else if (targetType == 'I') {
157159
cf.pushDescriptor("Z");
158160
}
159161

160-
protected boolean equalityCheck(ExpressionState state, Object left, Object right) {
162+
public static boolean equalityCheck(EvaluationContext context, Object left, Object right) {
161163
if (left instanceof Number && right instanceof Number) {
162164
Number leftNumber = (Number) left;
163165
Number rightNumber = (Number) right;
@@ -207,7 +209,7 @@ else if (leftNumber instanceof Byte || rightNumber instanceof Byte) {
207209
if (left instanceof Comparable && right instanceof Comparable) {
208210
Class<?> ancestor = ClassUtils.determineCommonAncestor(left.getClass(), right.getClass());
209211
if (ancestor != null && Comparable.class.isAssignableFrom(ancestor)) {
210-
return (state.getTypeComparator().compare(left, right) == 0);
212+
return (context.getTypeComparator().compare(left, right) == 0);
211213
}
212214
}
213215

0 commit comments

Comments
 (0)