Merge pull request #30 from Miaplaza/fix-expression-caching

Fix expression caching
Miaplaza · Apr 10, 2020 · 64481c1 · 64481c1
2 parents e3ae9a4 + ed34ac6
commit 64481c1
Show file tree

Hide file tree

Showing 9 changed files with 165 additions and 50 deletions.
diff --git a/ExpressionUtils/CompiledActivator.cs b/ExpressionUtils/CompiledActivator.cs
@@ -61,24 +61,5 @@ public static T Create(Type t) {
 				return constructor.Invoke();
 			}
 		}
-
-		public static class ForAnyType {
-			private static ConcurrentDictionary<Type, Func<object>> cachedNew = new ConcurrentDictionary<Type, Func<object>>();
-
-			/// <summary>
-			/// Create a <paramref name="t"/> by invoking its default constructor.
-			/// This is much faster than <c>(T)Activator.CreateInstance(t)</c>.
-			/// </summary>
-			public static object Create(Type t) {
-				Func<object> constructor;
-				// We do not need to lock the dictionary; another thread can only overwrite it with the same value
-				if (!cachedNew.TryGetValue(t, out constructor)) {
-
-					constructor = Expression.Lambda<Func<object>>(Expression.TypeAs(Expression.New(t), typeof(object))).Compile();
-					cachedNew[t] = constructor;
-				}
-				return constructor.Invoke();
-			}
-		}
 	}
 }
diff --git a/ExpressionUtils/Evaluating/CachedExpressionCompiler.cs b/ExpressionUtils/Evaluating/CachedExpressionCompiler.cs
@@ -3,8 +3,10 @@
 using System.Collections.Generic;
 using System.Linq;
 using System.Linq.Expressions;
-using System.Text;
-using System.Threading.Tasks;
+using System.Runtime.CompilerServices;
+
+[assembly: InternalsVisibleTo("ExpressionUtilsTest")]
+
 
 namespace MiaPlaza.ExpressionUtils.Evaluating {
 	/// <summary>
@@ -40,8 +42,8 @@ public VariadicArrayParametersDelegate CachedCompileLambda(LambdaExpression lamb
 						extractionResult.ConstantfreeExpression.Parameters.Concat(lambda.Parameters)))
 						.Compile();
 
-				var key = buildCacheKey(extractionResult, lambda.Parameters);
-				
+				var key = getClosureFreeKeyForCaching(extractionResult, lambda.Parameters);
+
 				delegates.TryAdd(key, compiled);
 				constants = extractionResult.ExtractedConstants;
 			}
@@ -55,13 +57,32 @@ public VariadicArrayParametersDelegate CachedCompileLambda(LambdaExpression lamb
 		DELEGATE IExpressionEvaluator.EvaluateTypedLambda<DELEGATE>(Expression<DELEGATE> expression) => CachedCompileTypedLambda(expression);
 		public DELEGATE CachedCompileTypedLambda<DELEGATE>(Expression<DELEGATE> expression) where DELEGATE : class => CachedCompileLambda(expression).WrapDelegate<DELEGATE>();
 
-		private LambdaExpression buildCacheKey(ConstantExtractor.ExtractionResult extractionResult, IReadOnlyCollection<ParameterExpression> parameterExpressions) {
-			var e= ParameterSubstituter.SubstituteParameter(extractionResult.ConstantfreeExpression,
+
+		/// <summary>
+		/// A closure free expression tree that can be used as a caching key. Can be used with the <see cref="ExpressionComparing.StructuralComparer" /> to compare
+		/// to the original lambda expression.
+		/// </summary>
+		private LambdaExpression getClosureFreeKeyForCaching(ConstantExtractor.ExtractionResult extractionResult, IReadOnlyCollection<ParameterExpression> parameterExpressions) {
+			var e = SimpleParameterSubstituter.SubstituteParameter(extractionResult.ConstantfreeExpression,
 				extractionResult.ConstantfreeExpression.Parameters.Select(
-						p => p.Type.IsValueType && !(p.Type.IsGenericType && p.Type.GetGenericTypeDefinition() == typeof(Nullable<>)) ? 
-							(Expression) Expression.Constant(CompiledActivator.ForAnyType.Create(p.Type)) :
-							(Expression) Expression.TypeAs(Expression.Constant(null), p.Type)));
+						p => (Expression) Expression.Constant(getDefaultValue(p.Type), p.Type)));
+
 			return Expression.Lambda(e, parameterExpressions);
 		}
+
+		private static object getDefaultValue(Type t) {
+			if (t.IsValueType) {
+				return Activator.CreateInstance(t);
+			}
+
+			return null;
+		}
+
+		/// <remarks>
+		/// Use for testing only.
+		/// </remarks>
+		internal bool IsCached(LambdaExpression lambda) {
+			return delegates.ContainsKey(lambda);
+		}
 	}
 }
diff --git a/ExpressionUtils/ExpressionStructureIdentity.cs b/ExpressionUtils/ExpressionStructureIdentity.cs
@@ -237,7 +237,7 @@ public StructuralComparer(bool ignoreConstantsValues = false, int? hashCodeExpre
 
 			public int GetHashCode(Expression tree) => GetNodeTypeStructureHashCode(tree, IgnoreConstantsValues, HashCodeExpressionDepth);
 
-			bool IEqualityComparer<Expression>.Equals(Expression x, Expression y) => StructuralIdentical(x, y);
+			bool IEqualityComparer<Expression>.Equals(Expression x, Expression y) => StructuralIdentical(x, y, IgnoreConstantsValues);
 		}
 
 		/// <summary>

diff --git a/ExpressionUtils/ExpressionUtils.csproj b/ExpressionUtils/ExpressionUtils.csproj
@@ -8,9 +8,9 @@
     <Product>MiaPlaza.ExpressionUtils.Properties</Product>
     <Description>Efficient Processing, Compilation, and Execution of Expression Trees at Runtime</Description>
     <Copyright>Copyright ©2017-2019 Miaplaza Inc.</Copyright>
-    <Version>1.1.7</Version>
-    <AssemblyVersion>1.1.7</AssemblyVersion>
-    <FileVersion>1.1.7</FileVersion>
+    <Version>1.2.0</Version>
+    <AssemblyVersion>1.2.0</AssemblyVersion>
+    <FileVersion>1.2.0</FileVersion>
     <ErrorReport>none</ErrorReport>
     <Authors>Miaplaza Inc.</Authors>
     <RepositoryUrl>https://github.com/Miaplaza/expression-utils</RepositoryUrl>

diff --git a/ExpressionUtils/ParameterSubstituter.cs b/ExpressionUtils/ParameterSubstituter.cs
@@ -21,14 +21,15 @@ namespace MiaPlaza.ExpressionUtils {
 	/// To that end, the visitor also removes unecessary casts to find the most specific override
 	/// possible (<see cref="VisitUnary"/>.)
 	/// </remarks>
-	public class ParameterSubstituter : ExpressionVisitor {
-		public static Expression SubstituteParameter(LambdaExpression expression, params Expression[] replacements)
+	public class ParameterSubstituter : SimpleParameterSubstituter {
+
+		public static new Expression SubstituteParameter(LambdaExpression expression, params Expression[] replacements)
 			=> SubstituteParameter(expression, replacements as IReadOnlyCollection<Expression>);
 
-		public static Expression SubstituteParameter(LambdaExpression expression, IEnumerable<Expression> replacements)
+		public static new Expression SubstituteParameter(LambdaExpression expression, IEnumerable<Expression> replacements)
 			=> SubstituteParameter(expression, replacements.ToList());
 
-		public static Expression SubstituteParameter(LambdaExpression expression, IReadOnlyCollection<Expression> replacements) {
+		public static new Expression SubstituteParameter(LambdaExpression expression, IReadOnlyCollection<Expression> replacements) {
 			if (expression == null) {
 				throw new ArgumentNullException(nameof(expression));
 			}
@@ -56,20 +57,7 @@ public static Expression SubstituteParameter(LambdaExpression expression, IReadO
 		public static Expression SubstituteParameter(Expression expression, IReadOnlyDictionary<ParameterExpression, Expression> replacements)
 			=> new ParameterSubstituter(replacements).Visit(expression);
 
-		readonly IReadOnlyDictionary<ParameterExpression, Expression> replacements;
-
-		ParameterSubstituter(IReadOnlyDictionary<ParameterExpression, Expression> replacements) {
-			this.replacements = replacements;
-		}
-
-		protected override Expression VisitParameter(ParameterExpression node) {
-			Expression replacement;
-			if (replacements.TryGetValue(node, out replacement)) {
-				return replacement;
-			} else {
-				return node;
-			}
-		}
+		ParameterSubstituter(IReadOnlyDictionary<ParameterExpression, Expression> replacements) : base(replacements) {	}
 
 		protected override Expression VisitMember(MemberExpression node) {
 			var baseCallResult = (MemberExpression)base.VisitMember(node);
@@ -119,6 +107,10 @@ protected override Expression VisitUnary(UnaryExpression node) {
 			return base.VisitUnary(node);
 		}
 
+		protected override Expression VisitBinary(BinaryExpression node) {
+			return base.VisitBinary(node);
+		}
+
 		private static MethodInfo getImplementationToCallOn(Type t, MethodInfo method) {
 			if (method.DeclaringType.IsInterface) {
 				return method.GetImplementationInfo(t);

diff --git a/ExpressionUtils/SimpleParameterSubstituter.cs b/ExpressionUtils/SimpleParameterSubstituter.cs
@@ -0,0 +1,67 @@
+using System;
+using System.Collections.Generic;
+using System.Collections.ObjectModel;
+using System.Linq;
+using System.Linq.Expressions;
+using System.Reflection;
+
+namespace MiaPlaza.ExpressionUtils {
+	/// <summary>
+	/// Replaces all parameters in an lambda-expression by other expressions, i.e., given a lambda
+	/// <c>(t0, …, tn) → f(t0,…,tn)</c> and expressions <c>(x0,…,xn)</c>, it returns the expression <c>f(x0,…,xn)</c>.
+	/// </summary>
+	/// <remarks>
+	/// Does nothing else than that, esepicially does not change the structure of the expression or removes closures.false
+	/// If you want that and other optimizations, use see <cref="ParameterSubstituter" />
+	/// </remarks>
+	public class SimpleParameterSubstituter : ExpressionVisitor {
+		public static Expression SubstituteParameter(LambdaExpression expression, params Expression[] replacements)
+			=> SubstituteParameter(expression, replacements as IReadOnlyCollection<Expression>);
+
+		public static Expression SubstituteParameter(LambdaExpression expression, IEnumerable<Expression> replacements)
+			=> SubstituteParameter(expression, replacements.ToList());
+
+		public static Expression SubstituteParameter(LambdaExpression expression, IReadOnlyCollection<Expression> replacements) {
+			if (expression == null) {
+				throw new ArgumentNullException(nameof(expression));
+			}
+
+			if (replacements == null) {
+				throw new ArgumentNullException(nameof(replacements));
+			}
+
+			if (expression.Parameters.Count != replacements.Count) {
+				throw new ArgumentException($"Replacement count does not match parameter count ({replacements.Count} vs {expression.Parameters.Count})");
+			}
+
+			var dict = new Dictionary<ParameterExpression, Expression>();
+
+			foreach (var tuple in expression.Parameters.Zip(replacements, (p, r) => new { parameter = p, replacement = r })) {
+				if (!tuple.parameter.Type.IsAssignableFrom(tuple.replacement.Type)) {
+					throw new ArgumentException($"The expression {tuple.replacement} cannot be used as replacement for the parameter {tuple.parameter}.");
+				}
+				dict[tuple.parameter] = tuple.replacement;
+			}
+
+			return new SimpleParameterSubstituter(dict).Visit(expression.Body);
+		}
+
+		public static Expression SubstituteParameter(Expression expression, IReadOnlyDictionary<ParameterExpression, Expression> replacements)
+			=> new SimpleParameterSubstituter(replacements).Visit(expression);
+
+		readonly IReadOnlyDictionary<ParameterExpression, Expression> replacements;
+
+		protected SimpleParameterSubstituter(IReadOnlyDictionary<ParameterExpression, Expression> replacements) {
+			this.replacements = replacements;
+		}
+
+		protected override Expression VisitParameter(ParameterExpression node) {
+			Expression replacement;
+			if (replacements.TryGetValue(node, out replacement)) {
+				return replacement;
+			} else {
+				return node;
+			}
+		}
+	}
+}
diff --git a/ExpressionUtilsTest/CachedExpressionCompilerTestEvaluator.cs b/ExpressionUtilsTest/CachedExpressionCompilerTestEvaluator.cs
@@ -0,0 +1,26 @@
+using System.Linq.Expressions;
+using MiaPlaza.ExpressionUtils;
+using MiaPlaza.ExpressionUtils.Evaluating;
+using NUnit.Framework;
+
+namespace MiaPlaza.Test.ExpressionUtilsTest {
+	public class CachedExpressionCompilerTestEvaluator : IExpressionEvaluator {
+
+		public object Evaluate(Expression unparametrizedExpression) {
+			var lambda = Expression.Lambda(unparametrizedExpression);
+			return this.EvaluateLambda(lambda)();
+		}
+
+		public VariadicArrayParametersDelegate EvaluateLambda(LambdaExpression lambdaExpression) {
+			var result = ((IExpressionEvaluator)CachedExpressionCompiler.Instance).EvaluateLambda(lambdaExpression);
+			Assert.IsTrue(CachedExpressionCompiler.Instance.IsCached(lambdaExpression));
+			return result;
+		}
+
+		public DELEGATE EvaluateTypedLambda<DELEGATE>(Expression<DELEGATE> expression) where DELEGATE : class {
+
+			var result = this.EvaluateLambda(expression);
+			return result.WrapDelegate<DELEGATE>();
+		}
+	}
+}
diff --git a/ExpressionUtilsTest/ExpressionEvaluation.cs b/ExpressionUtilsTest/ExpressionEvaluation.cs
@@ -19,6 +19,7 @@ class ExpressionEvaluation {
 			new TestFixtureData(ExpressionCompiler.Instance),
 			new TestFixtureData(CachedExpressionCompiler.Instance),
 			new TestFixtureData(ExpressionInterpreter.Instance),
+			new TestFixtureData(new CachedExpressionCompilerTestEvaluator()),
 		};
 
 		private readonly IExpressionEvaluator evaluator;
@@ -119,6 +120,14 @@ public void TestNullableEnumToIntConvertExpression() {
 			Assert.Catch(() => evaluator.Evaluate(expression.Body));
 		}
 
+		[Test]
+		public void TestNullableEnumExpression() {
+			Expression<Func<MyEnum?, bool>> expA = (MyEnum? t) => t == MyEnum.First;
+
+			Assert.IsTrue((bool)evaluator.EvaluateLambda(expA)(MyEnum.First));
+			Assert.IsFalse((bool)evaluator.EvaluateLambda(expA)(MyEnum.Second));
+		}
+
 		public static readonly IEnumerable<int> TestOffsets = new[] {
 			1,
 			100,
@@ -206,5 +215,15 @@ public void TestNullableEquality() {
 			Assert.That((bool)evaluator.EvaluateLambda(hasValue)(DateTime.Now));
 			Assert.IsFalse((bool)(evaluator.EvaluateLambda(hasValue)((DateTime?)null)));
 		}
+
+		class ParentClass {}
+		class ChildClass : ParentClass {}  
+
+		[Test]
+		public void TestConvertExpresssion() {
+			var exp = Expression.Convert(Expression.Constant(new ChildClass()), typeof(ParentClass));
+
+			evaluator.Evaluate(exp);
+		}
 	}
 }
diff --git a/ExpressionUtilsTest/StructuralIdentity.cs b/ExpressionUtilsTest/StructuralIdentity.cs
@@ -188,5 +188,14 @@ public void ComplexLambda() {
 
 			Assert.IsTrue(expA.StructuralIdentical(expB));
 		}
+
+		[Test]
+		public void TestClosureLambda() {
+			int variable = 7;
+			Expression<Func<int, bool>> expA = (int a) => a != variable;
+			Expression<Func<int, bool>> expB = (int a) => a != 8;
+
+			Assert.IsFalse(expA.StructuralIdentical(expB, true));
+		}		
 	}
 }