Optimize InlineMap and InlineList in SpEL

- Make InlineList#constant and InlineMap#constant final. - Add more assertions for InlineMap tests. Closes gh-30251
spring-projects · Aug 20, 2023 · 0e0c298 · 0e0c298
1 parent baf3678
commit 0e0c298
Show file tree

Hide file tree

Showing 3 changed files with 62 additions and 60 deletions.
diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/InlineList.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/InlineList.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2021 the original author or authors.
+ * Copyright 2002-2023 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.
@@ -35,55 +35,57 @@
  *
  * @author Andy Clement
  * @author Sam Brannen
+ * @author Harry Yang
  * @since 3.0.4
  */
 public class InlineList extends SpelNodeImpl {
 
 	// If the list is purely literals, it is a constant value and can be computed and cached
 	@Nullable
-	private TypedValue constant;  // TODO must be immutable list
+	private final TypedValue constant;
 
 
 	public InlineList(int startPos, int endPos, SpelNodeImpl... args) {
 		super(startPos, endPos, args);
-		checkIfConstant();
+		this.constant = computeConstantValue();
 	}
 
 
 	/**
-	 * If all the components of the list are constants, or lists that themselves contain constants, then a constant list
-	 * can be built to represent this node. This will speed up later getValue calls and reduce the amount of garbage
+	 * If all the components of the list are constants, or lists
+	 * that themselves contain constants, then a constant list
+	 * can be built to represent this node. This will speed up
+	 * later getValue calls and reduce the amount of garbage
 	 * created.
 	 */
-	private void checkIfConstant() {
-		boolean isConstant = true;
+	@Nullable
+	private TypedValue computeConstantValue() {
 		for (int c = 0, max = getChildCount(); c < max; c++) {
 			SpelNode child = getChild(c);
 			if (!(child instanceof Literal)) {
 				if (child instanceof InlineList inlineList) {
 					if (!inlineList.isConstant()) {
-						isConstant = false;
+						return null;
 					}
 				}
 				else {
-					isConstant = false;
+					return null;
 				}
 			}
 		}
-		if (isConstant) {
-			List<Object> constantList = new ArrayList<>();
-			int childcount = getChildCount();
-			for (int c = 0; c < childcount; c++) {
-				SpelNode child = getChild(c);
-				if (child instanceof Literal literal) {
-					constantList.add(literal.getLiteralValue().getValue());
-				}
-				else if (child instanceof InlineList inlineList) {
-					constantList.add(inlineList.getConstantValue());
-				}
+
+		List<Object> constantList = new ArrayList<>();
+		int childcount = getChildCount();
+		for (int c = 0; c < childcount; c++) {
+			SpelNode child = getChild(c);
+			if (child instanceof Literal literal) {
+				constantList.add(literal.getLiteralValue().getValue());
+			}
+			else if (child instanceof InlineList inlineList) {
+				constantList.add(inlineList.getConstantValue());
 			}
-			this.constant = new TypedValue(Collections.unmodifiableList(constantList));
 		}
+		return new TypedValue(Collections.unmodifiableList(constantList));
 	}
 
 	@Override

diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/InlineMap.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/InlineMap.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2021 the original author or authors.
+ * Copyright 2002-2023 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.
@@ -32,18 +32,19 @@
  *
  * @author Andy Clement
  * @author Sam Brannen
+ * @author Harry Yang
  * @since 4.1
  */
 public class InlineMap extends SpelNodeImpl {
 
 	// If the map is purely literals, it is a constant value and can be computed and cached
 	@Nullable
-	private TypedValue constant;
+	private final TypedValue constant;
 
 
 	public InlineMap(int startPos, int endPos, SpelNodeImpl... args) {
 		super(startPos, endPos, args);
-		checkIfConstant();
+		this.constant =	computeConstantValue();
 	}
 
 
@@ -52,59 +53,55 @@ public InlineMap(int startPos, int endPos, SpelNodeImpl... args) {
 	 * contain constants, then a constant list can be built to represent this node.
 	 * This will speed up later getValue calls and reduce the amount of garbage created.
 	 */
-	private void checkIfConstant() {
-		boolean isConstant = true;
+	@Nullable
+	private TypedValue computeConstantValue() {
 		for (int c = 0, max = getChildCount(); c < max; c++) {
 			SpelNode child = getChild(c);
 			if (!(child instanceof Literal)) {
 				if (child instanceof InlineList inlineList) {
 					if (!inlineList.isConstant()) {
-						isConstant = false;
-						break;
+						return null;
 					}
 				}
 				else if (child instanceof InlineMap inlineMap) {
 					if (!inlineMap.isConstant()) {
-						isConstant = false;
-						break;
+						return null;
 					}
 				}
 				else if (!(c % 2 == 0 && child instanceof PropertyOrFieldReference)) {
-					isConstant = false;
-					break;
+					return null;
 				}
 			}
 		}
-		if (isConstant) {
-			Map<Object, Object> constantMap = new LinkedHashMap<>();
-			int childCount = getChildCount();
-			for (int c = 0; c < childCount; c++) {
-				SpelNode keyChild = getChild(c++);
-				SpelNode valueChild = getChild(c);
-				Object key = null;
-				Object value = null;
-				if (keyChild instanceof Literal literal) {
-					key = literal.getLiteralValue().getValue();
-				}
-				else if (keyChild instanceof PropertyOrFieldReference propertyOrFieldReference) {
-					key = propertyOrFieldReference.getName();
-				}
-				else {
-					return;
-				}
-				if (valueChild instanceof Literal literal) {
-					value = literal.getLiteralValue().getValue();
-				}
-				else if (valueChild instanceof InlineList inlineList) {
-					value = inlineList.getConstantValue();
-				}
-				else if (valueChild instanceof InlineMap inlineMap) {
-					value = inlineMap.getConstantValue();
-				}
-				constantMap.put(key, value);
+
+		Map<Object, Object> constantMap = new LinkedHashMap<>();
+		int childCount = getChildCount();
+		for (int c = 0; c < childCount; c++) {
+			SpelNode keyChild = getChild(c++);
+			SpelNode valueChild = getChild(c);
+			Object key;
+			Object value = null;
+			if (keyChild instanceof Literal literal) {
+				key = literal.getLiteralValue().getValue();
+			}
+			else if (keyChild instanceof PropertyOrFieldReference propertyOrFieldReference) {
+				key = propertyOrFieldReference.getName();
+			}
+			else {
+				return null;
+			}
+			if (valueChild instanceof Literal literal) {
+				value = literal.getLiteralValue().getValue();
+			}
+			else if (valueChild instanceof InlineList inlineList) {
+				value = inlineList.getConstantValue();
+			}
+			else if (valueChild instanceof InlineMap inlineMap) {
+				value = inlineMap.getConstantValue();
 			}
-			this.constant = new TypedValue(Collections.unmodifiableMap(constantMap));
+			constantMap.put(key, value);
 		}
+		return new TypedValue(Collections.unmodifiableMap(constantMap));
 	}
 
 	@Override

diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/MapTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/MapTests.java
@@ -117,6 +117,9 @@ public void testConstantRepresentation1() {
 		checkConstantMap("{#root.name:true}",false);
 		checkConstantMap("{a:1,b:2,c:{d:true,e:false}}", true);
 		checkConstantMap("{a:1,b:2,c:{d:{1,2,3},e:{4,5,6},f:{'a','b','c'}}}", true);
+		// for nested InlineMap
+		checkConstantMap("{a:{k:#d}}", false);
+		checkConstantMap("{@bean:@bean}", false);
 	}
 
 	private void checkConstantMap(String expressionText, boolean expectedToBeConstant) {