GitHub-Spring
/
spring-framework
mirror of https://github.com/spring-projects/spring-framework.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity
Browse Source

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
pull/1228/merge
Andy Clement 10 years ago
parent
99cacaa72d
commit
9000acd39d
5 changed files with 196 additions and 136 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Split View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 9
      spring-expression/src/main/java/org/springframework/expression/spel/CodeFlow.java
  2. 109
      spring-expression/src/main/java/org/springframework/expression/spel/ast/OpEQ.java
  3. 86
      spring-expression/src/main/java/org/springframework/expression/spel/ast/OpNE.java
  4. 8
      spring-expression/src/main/java/org/springframework/expression/spel/ast/Operator.java
  5. 120
      spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java

9
spring-expression/src/main/java/org/springframework/expression/spel/CodeFlow.java
Unescape View File

@ -99,6 +99,15 @@ public class CodeFlow implements Opcodes { @@ -99,6 +99,15 @@ public class CodeFlow implements Opcodes {
mv.visitVarInsn(ALOAD, 1);
}
/**
* Push the bytecode to load the EvaluationContext (the second parameter passed to
* the compiled expression method).
* @param mv the visitor into which the load instruction should be inserted
*/
public void loadEvaluationContext(MethodVisitor mv) {
mv.visitVarInsn(ALOAD, 2);
}
/**
* Record the descriptor for the most recently evaluated expression element.
* @param descriptor type descriptor for most recently evaluated element

109
spring-expression/src/main/java/org/springframework/expression/spel/ast/OpEQ.java
Unescape View File

@ -1,5 +1,5 @@ @@ -1,5 +1,5 @@
/*
* Copyright 2002-2014 the original author or authors.
* Copyright 2002-2016 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@ -16,8 +16,8 @@ @@ -16,8 +16,8 @@
package org.springframework.expression.spel.ast;
import org.springframework.asm.Label;
import org.springframework.asm.MethodVisitor;
import org.springframework.expression.EvaluationContext;
import org.springframework.expression.EvaluationException;
import org.springframework.expression.spel.CodeFlow;
import org.springframework.expression.spel.ExpressionState;
@ -36,112 +36,61 @@ public class OpEQ extends Operator { @@ -36,112 +36,61 @@ public class OpEQ extends Operator {
this.exitTypeDescriptor = "Z";
}
@Override
public BooleanTypedValue getValueInternal(ExpressionState state) throws EvaluationException {
public BooleanTypedValue getValueInternal(ExpressionState state)
throws EvaluationException {
Object left = getLeftOperand().getValueInternal(state).getValue();
Object right = getRightOperand().getValueInternal(state).getValue();
this.leftActualDescriptor = CodeFlow.toDescriptorFromObject(left);
this.rightActualDescriptor = CodeFlow.toDescriptorFromObject(right);
return BooleanTypedValue.forValue(equalityCheck(state, left, right));
return BooleanTypedValue.forValue(
equalityCheck(state.getEvaluationContext(), left, right));
}
// This check is different to the one in the other numeric operators (OpLt/etc)
// because it allows for simple object comparison
@Override
public boolean isCompilable() {
SpelNodeImpl left = getLeftOperand();
SpelNodeImpl right= getRightOperand();
SpelNodeImpl right = getRightOperand();
if (!left.isCompilable() || !right.isCompilable()) {
return false;
}
String leftDesc = left.exitTypeDescriptor;
String rightDesc = right.exitTypeDescriptor;
DescriptorComparison dc = DescriptorComparison.checkNumericCompatibility(
leftDesc, rightDesc, this.leftActualDescriptor, this.rightActualDescriptor);
DescriptorComparison dc = DescriptorComparison.checkNumericCompatibility(leftDesc,
rightDesc, this.leftActualDescriptor, this.rightActualDescriptor);
return (!dc.areNumbers || dc.areCompatible);
}
@Override
public void generateCode(MethodVisitor mv, CodeFlow cf) {
cf.loadEvaluationContext(mv);
String leftDesc = getLeftOperand().exitTypeDescriptor;
String rightDesc = getRightOperand().exitTypeDescriptor;
Label elseTarget = new Label();
Label endOfIf = new Label();
boolean leftPrim = CodeFlow.isPrimitive(leftDesc);
boolean rightPrim = CodeFlow.isPrimitive(rightDesc);
DescriptorComparison dc = DescriptorComparison.checkNumericCompatibility(
leftDesc, rightDesc, this.leftActualDescriptor, this.rightActualDescriptor);
if (dc.areNumbers && dc.areCompatible) {
char targetType = dc.compatibleType;
getLeftOperand().generateCode(mv, cf);
if (!leftPrim) {
CodeFlow.insertUnboxInsns(mv, targetType, leftDesc);
}
cf.enterCompilationScope();
getRightOperand().generateCode(mv, cf);
cf.exitCompilationScope();
if (!rightPrim) {
CodeFlow.insertUnboxInsns(mv, targetType, rightDesc);
}
// assert: SpelCompiler.boxingCompatible(leftDesc, rightDesc)
if (targetType == 'D') {
mv.visitInsn(DCMPL);
mv.visitJumpInsn(IFNE, elseTarget);
}
else if (targetType == 'F') {
mv.visitInsn(FCMPL);
mv.visitJumpInsn(IFNE, elseTarget);
}
else if (targetType == 'J') {
mv.visitInsn(LCMP);
mv.visitJumpInsn(IFNE, elseTarget);
}
else if (targetType == 'I' || targetType == 'Z') {
mv.visitJumpInsn(IF_ICMPNE, elseTarget);
}
else {
throw new IllegalStateException("Unexpected descriptor " + leftDesc);
}
cf.enterCompilationScope();
getLeftOperand().generateCode(mv, cf);
cf.exitCompilationScope();
if (leftPrim) {
CodeFlow.insertBoxIfNecessary(mv, leftDesc.charAt(0));
}
else {
getLeftOperand().generateCode(mv, cf);
if (leftPrim) {
CodeFlow.insertBoxIfNecessary(mv, leftDesc.charAt(0));
}
getRightOperand().generateCode(mv, cf);
if (rightPrim) {
CodeFlow.insertBoxIfNecessary(mv, rightDesc.charAt(0));
}
Label leftNotNull = new Label();
mv.visitInsn(DUP_X1); // dup right on the top of the stack
mv.visitJumpInsn(IFNONNULL, leftNotNull);
// Right is null!
mv.visitInsn(SWAP);
mv.visitInsn(POP); // remove it
Label rightNotNull = new Label();
mv.visitJumpInsn(IFNONNULL, rightNotNull);
// Left is null too
mv.visitInsn(ICONST_1);
mv.visitJumpInsn(GOTO, endOfIf);
mv.visitLabel(rightNotNull);
mv.visitInsn(ICONST_0);
mv.visitJumpInsn(GOTO, endOfIf);
mv.visitLabel(leftNotNull);
mv.visitMethodInsn(INVOKEVIRTUAL, "java/lang/Object", "equals", "(Ljava/lang/Object;)Z", false);
mv.visitLabel(endOfIf);
cf.pushDescriptor("Z");
return;
cf.enterCompilationScope();
getRightOperand().generateCode(mv, cf);
cf.exitCompilationScope();
if (rightPrim) {
CodeFlow.insertBoxIfNecessary(mv, rightDesc.charAt(0));
}
mv.visitInsn(ICONST_1);
mv.visitJumpInsn(GOTO, endOfIf);
mv.visitLabel(elseTarget);
mv.visitInsn(ICONST_0);
mv.visitLabel(endOfIf);
String operatorClassName = Operator.class.getName().replace('.', '/');
String evaluationContextClassName = EvaluationContext.class.getName().replace('.',
'/');
mv.visitMethodInsn(INVOKESTATIC, operatorClassName, "equalityCheck", "(L"
+ evaluationContextClassName + ";Ljava/lang/Object;Ljava/lang/Object;)Z",
false);
cf.pushDescriptor("Z");
}

86
spring-expression/src/main/java/org/springframework/expression/spel/ast/OpNE.java
Unescape View File

@ -18,6 +18,7 @@ package org.springframework.expression.spel.ast; @@ -18,6 +18,7 @@ package org.springframework.expression.spel.ast;
import org.springframework.asm.Label;
import org.springframework.asm.MethodVisitor;
import org.springframework.expression.EvaluationContext;
import org.springframework.expression.EvaluationException;
import org.springframework.expression.spel.CodeFlow;
import org.springframework.expression.spel.ExpressionState;
@ -36,14 +37,15 @@ public class OpNE extends Operator { @@ -36,14 +37,15 @@ public class OpNE extends Operator {
this.exitTypeDescriptor = "Z";
}
@Override
public BooleanTypedValue getValueInternal(ExpressionState state) throws EvaluationException {
public BooleanTypedValue getValueInternal(ExpressionState state)
throws EvaluationException {
Object left = getLeftOperand().getValueInternal(state).getValue();
Object right = getRightOperand().getValueInternal(state).getValue();
this.leftActualDescriptor = CodeFlow.toDescriptorFromObject(left);
this.rightActualDescriptor = CodeFlow.toDescriptorFromObject(right);
return BooleanTypedValue.forValue(!equalityCheck(state, left, right));
return BooleanTypedValue.forValue(
!equalityCheck(state.getEvaluationContext(), left, right));
}
// This check is different to the one in the other numeric operators (OpLt/etc)
@ -51,72 +53,56 @@ public class OpNE extends Operator { @@ -51,72 +53,56 @@ public class OpNE extends Operator {
@Override
public boolean isCompilable() {
SpelNodeImpl left = getLeftOperand();
SpelNodeImpl right= getRightOperand();
SpelNodeImpl right = getRightOperand();
if (!left.isCompilable() || !right.isCompilable()) {
return false;
}
String leftDesc = left.exitTypeDescriptor;
String rightDesc = right.exitTypeDescriptor;
DescriptorComparison dc = DescriptorComparison.checkNumericCompatibility(
leftDesc, rightDesc, this.leftActualDescriptor, this.rightActualDescriptor);
DescriptorComparison dc = DescriptorComparison.checkNumericCompatibility(leftDesc,
rightDesc, this.leftActualDescriptor, this.rightActualDescriptor);
return (!dc.areNumbers || dc.areCompatible);
}
@Override
public void generateCode(MethodVisitor mv, CodeFlow cf) {
cf.loadEvaluationContext(mv);
String leftDesc = getLeftOperand().exitTypeDescriptor;
String rightDesc = getRightOperand().exitTypeDescriptor;
Label elseTarget = new Label();
Label endOfIf = new Label();
boolean leftPrim = CodeFlow.isPrimitive(leftDesc);
boolean rightPrim = CodeFlow.isPrimitive(rightDesc);
DescriptorComparison dc = DescriptorComparison.checkNumericCompatibility(
leftDesc, rightDesc, this.leftActualDescriptor, this.rightActualDescriptor);
if (dc.areNumbers && dc.areCompatible) {
char targetType = dc.compatibleType;
getLeftOperand().generateCode(mv, cf);
if (!leftPrim) {
CodeFlow.insertUnboxInsns(mv, targetType, leftDesc);
}
cf.enterCompilationScope();
getRightOperand().generateCode(mv, cf);
cf.exitCompilationScope();
if (!rightPrim) {
CodeFlow.insertUnboxInsns(mv, targetType, rightDesc);
}
// assert: SpelCompiler.boxingCompatible(leftDesc, rightDesc)
if (targetType == 'D') {
mv.visitInsn(DCMPL);
mv.visitJumpInsn(IFEQ, elseTarget);
}
else if (targetType == 'F') {
mv.visitInsn(FCMPL);
mv.visitJumpInsn(IFEQ, elseTarget);
}
else if (targetType == 'J') {
mv.visitInsn(LCMP);
mv.visitJumpInsn(IFEQ, elseTarget);
}
else if (targetType == 'I' || targetType == 'Z') {
mv.visitJumpInsn(IF_ICMPEQ, elseTarget);
}
else {
throw new IllegalStateException("Unexpected descriptor " + leftDesc);
}
cf.enterCompilationScope();
getLeftOperand().generateCode(mv, cf);
cf.exitCompilationScope();
if (leftPrim) {
CodeFlow.insertBoxIfNecessary(mv, leftDesc.charAt(0));
}
else {
getLeftOperand().generateCode(mv, cf);
getRightOperand().generateCode(mv, cf);
mv.visitJumpInsn(IF_ACMPEQ, elseTarget);
cf.enterCompilationScope();
getRightOperand().generateCode(mv, cf);
cf.exitCompilationScope();
if (rightPrim) {
CodeFlow.insertBoxIfNecessary(mv, rightDesc.charAt(0));
}
String operatorClassName = Operator.class.getName().replace('.', '/');
String evaluationContextClassName = EvaluationContext.class.getName().replace('.',
'/');
mv.visitMethodInsn(INVOKESTATIC, operatorClassName, "equalityCheck", "(L"
+ evaluationContextClassName + ";Ljava/lang/Object;Ljava/lang/Object;)Z",
false);
// Invert the boolean
Label notZero = new Label();
Label end = new Label();
mv.visitJumpInsn(IFNE, notZero);
mv.visitInsn(ICONST_1);
mv.visitJumpInsn(GOTO,endOfIf);
mv.visitLabel(elseTarget);
mv.visitJumpInsn(GOTO, end);
mv.visitLabel(notZero);
mv.visitInsn(ICONST_0);
mv.visitLabel(endOfIf);
mv.visitLabel(end);
cf.pushDescriptor("Z");
}

8
spring-expression/src/main/java/org/springframework/expression/spel/ast/Operator.java
Unescape View File

@ -21,8 +21,8 @@ import java.math.BigInteger; @@ -21,8 +21,8 @@ import java.math.BigInteger;
import org.springframework.asm.Label;
import org.springframework.asm.MethodVisitor;
import org.springframework.expression.EvaluationContext;
import org.springframework.expression.spel.CodeFlow;
import org.springframework.expression.spel.ExpressionState;
import org.springframework.util.ClassUtils;
import org.springframework.util.NumberUtils;
import org.springframework.util.ObjectUtils;
@ -114,7 +114,9 @@ public abstract class Operator extends SpelNodeImpl { @@ -114,7 +114,9 @@ public abstract class Operator extends SpelNodeImpl {
leftDesc, rightDesc, this.leftActualDescriptor, this.rightActualDescriptor);
char targetType = dc.compatibleType; // CodeFlow.toPrimitiveTargetDesc(leftDesc);
cf.enterCompilationScope();
getLeftOperand().generateCode(mv, cf);
cf.exitCompilationScope();
if (unboxLeft) {
CodeFlow.insertUnboxInsns(mv, targetType, leftDesc);
}
@ -157,7 +159,7 @@ public abstract class Operator extends SpelNodeImpl { @@ -157,7 +159,7 @@ public abstract class Operator extends SpelNodeImpl {
cf.pushDescriptor("Z");
}
protected boolean equalityCheck(ExpressionState state, Object left, Object right) {
public static boolean equalityCheck(EvaluationContext context, Object left, Object right) {
if (left instanceof Number && right instanceof Number) {
Number leftNumber = (Number) left;
Number rightNumber = (Number) right;
@ -207,7 +209,7 @@ public abstract class Operator extends SpelNodeImpl { @@ -207,7 +209,7 @@ public abstract class Operator extends SpelNodeImpl {
if (left instanceof Comparable && right instanceof Comparable) {
Class<?> ancestor = ClassUtils.determineCommonAncestor(left.getClass(), right.getClass());
if (ancestor != null && Comparable.class.isAssignableFrom(ancestor)) {
return (state.getTypeComparator().compare(left, right) == 0);
return (context.getTypeComparator().compare(left, right) == 0);
}
}

120
spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java
Unescape View File

@ -1335,8 +1335,6 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { @@ -1335,8 +1335,6 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
public void opEq() throws Exception {
String tvar = "35";
expression = parse("#root == 35");
Boolean bb = (Boolean)expression.getValue(tvar);
System.out.println(bb);
assertFalse((Boolean)expression.getValue(tvar));
assertCanCompile(expression);
assertFalse((Boolean)expression.getValue(tvar));
@ -1623,6 +1621,122 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { @@ -1623,6 +1621,122 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
assertTrue((Boolean)expression.getValue());
}
@Test
public void opNe_SPR14863() throws Exception {
// First part is from the test case specified in the bug report
Map<String, String> data = new HashMap<>();
data.put("my-key", new String("my-value"));
StandardEvaluationContext context = new StandardEvaluationContext(new MyContext(data));
SpelParserConfiguration configuration = new SpelParserConfiguration(SpelCompilerMode.MIXED,
ClassLoader.getSystemClassLoader());
SpelExpressionParser parser = new SpelExpressionParser(configuration);
Expression expression = parser.parseExpression("data['my-key'] != 'my-value'");
assertFalse(expression.getValue(context, Boolean.class));
assertCanCompile(expression);
((SpelExpression) expression).compileExpression();
assertFalse(expression.getValue(context, Boolean.class));
List<String> ls = new ArrayList<String>();
ls.add(new String("foo"));
context = new StandardEvaluationContext(ls);
expression = parse("get(0) != 'foo'");
assertFalse(expression.getValue(context, Boolean.class));
assertCanCompile(expression);
assertFalse(expression.getValue(context, Boolean.class));
ls.remove(0);
ls.add("goo");
assertTrue(expression.getValue(context, Boolean.class));
}
@Test
public void opEq_SPR14863() throws Exception {
// Exercise the comparator invocation code that runs in
// equalityCheck() (called from interpreted and compiled code)
expression = parser.parseExpression("#aa==#bb");
StandardEvaluationContext sec = new StandardEvaluationContext();
Apple aa = new Apple(1);
Apple bb = new Apple(2);
sec.setVariable("aa",aa);
sec.setVariable("bb",bb);
boolean b = expression.getValue(sec,Boolean.class);
// Verify what the expression caused aa to be compared to
assertEquals(bb,aa.gotComparedTo);
assertFalse(b);
bb.setValue(1);
b = expression.getValue(sec,Boolean.class);
assertEquals(bb,aa.gotComparedTo);
assertTrue(b);
assertCanCompile(expression);
// Similar test with compiled expression
aa = new Apple(99);
bb = new Apple(100);
sec.setVariable("aa",aa);
sec.setVariable("bb",bb);
b = expression.getValue(sec,Boolean.class);
assertFalse(b);
assertEquals(bb,aa.gotComparedTo);
bb.setValue(99);
b = expression.getValue(sec,Boolean.class);
assertTrue(b);
assertEquals(bb,aa.gotComparedTo);
List<String> ls = new ArrayList<String>();
ls.add(new String("foo"));
StandardEvaluationContext context = new StandardEvaluationContext(ls);
expression = parse("get(0) == 'foo'");
assertTrue(expression.getValue(context, Boolean.class));
assertCanCompile(expression);
assertTrue(expression.getValue(context, Boolean.class));
ls.remove(0);
ls.add("goo");
assertFalse(expression.getValue(context, Boolean.class));
}
public static class Apple implements Comparable<Apple> {
public Object gotComparedTo = null;
public int i;
public Apple(int i) {
this.i = i;
}
public void setValue(int i) {
this.i = i;
}
@Override
public int compareTo(Apple that) {
this.gotComparedTo = that;
if (this.i<that.i) {
return -1;
} else if (this.i>that.i) {
return +1;
} else {
return 0;
}
}
}
// For opNe_SPR14863
public static class MyContext {
private final Map<String, String> data;
public MyContext(Map<String, String> data) {
this.data = data;
}
public Map<String, String> getData() {
return data;
}
}
@Test
public void opPlus() throws Exception {
expression = parse("2+2");
@ -3901,7 +4015,7 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { @@ -3901,7 +4015,7 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
m.put("andy","778");
expression = parse("['andy']==null?1:2");
System.out.println(expression.getValue(m));
assertEquals(2,expression.getValue(m));
assertCanCompile(expression);
assertEquals(2,expression.getValue(m));
m.remove("andy");

Write Preview
Loading…
Cancel
Save