From aeb716be8edeafff0d0fe36775012a8fdbd7cd3e Mon Sep 17 00:00:00 2001 From: Leonard Kleinhans <lkleinhans@miaplaza.com> Date: Fri, 10 Apr 2020 04:12:57 +0200 Subject: [PATCH 1/4] Fix bug where IgnoreConstantsValues in StructuralComparer is ignored because the argument was not passed through --- ExpressionUtils/ExpressionStructureIdentity.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ExpressionUtils/ExpressionStructureIdentity.cs b/ExpressionUtils/ExpressionStructureIdentity.cs index 2484f00..847dfd9 100644 --- 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> From 8c0368541368b9bc556c24924de27040fa5290d4 Mon Sep 17 00:00:00 2001 From: Leonard Kleinhans <lkleinhans@miaplaza.com> Date: Fri, 10 Apr 2020 04:41:12 +0200 Subject: [PATCH 2/4] Fix expression compilation cache * Use a much simpler parameter substituter to not change the structure of the expression when building the cache key * Simplify default expression constant instantiation when building the cache key * Add test for structural identity * Introduce CachedExpressionCompilerTestEvaluator to test if expression that were evaluated with the CachedExpressionCompiler are cached --- ExpressionUtils/CompiledActivator.cs | 19 ------ .../Evaluating/CachedExpressionCompiler.cs | 39 ++++++++--- ExpressionUtils/ParameterSubstituter.cs | 28 +++----- ExpressionUtils/SimpleParameterSubstituter.cs | 67 +++++++++++++++++++ .../CachedExpressionCompilerTestEvaluator.cs | 26 +++++++ ExpressionUtilsTest/ExpressionEvaluation.cs | 19 ++++++ ExpressionUtilsTest/StructuralIdentity.cs | 9 +++ 7 files changed, 161 insertions(+), 46 deletions(-) create mode 100644 ExpressionUtils/SimpleParameterSubstituter.cs create mode 100644 ExpressionUtilsTest/CachedExpressionCompilerTestEvaluator.cs diff --git a/ExpressionUtils/CompiledActivator.cs b/ExpressionUtils/CompiledActivator.cs index ec3513d..3ebf031 100644 --- 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(); - } - } } } \ No newline at end of file diff --git a/ExpressionUtils/Evaluating/CachedExpressionCompiler.cs b/ExpressionUtils/Evaluating/CachedExpressionCompiler.cs index 87362aa..dafaea9 100644 --- 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 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/ParameterSubstituter.cs b/ExpressionUtils/ParameterSubstituter.cs index 5568dc6..6a0d758 100644 --- 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 new file mode 100644 index 0000000..b45364a --- /dev/null +++ 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 new file mode 100644 index 0000000..f37886a --- /dev/null +++ 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 index a38d9f5..d3dc598 100644 --- 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 index 4f31bb5..d33f9f1 100644 --- 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)); + } } } From bd1e0afa03f672d0d585dd100071497a1b65b395 Mon Sep 17 00:00:00 2001 From: Leonard Kleinhans <lkleinhans@miaplaza.com> Date: Fri, 10 Apr 2020 04:44:37 +0200 Subject: [PATCH 3/4] Bump version to 1.2.0 --- ExpressionUtils/ExpressionUtils.csproj | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ExpressionUtils/ExpressionUtils.csproj b/ExpressionUtils/ExpressionUtils.csproj index af6e82f..45e7a69 100644 --- 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> From ed34ac6fdf6b08e31584869320ab57cae3260eeb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20R=C3=BCth?= <julian.rueth@fsfe.org> Date: Fri, 10 Apr 2020 05:06:49 +0200 Subject: [PATCH 4/4] Fix casing --- ExpressionUtils/Evaluating/CachedExpressionCompiler.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ExpressionUtils/Evaluating/CachedExpressionCompiler.cs b/ExpressionUtils/Evaluating/CachedExpressionCompiler.cs index dafaea9..1233586 100644 --- a/ExpressionUtils/Evaluating/CachedExpressionCompiler.cs +++ b/ExpressionUtils/Evaluating/CachedExpressionCompiler.cs @@ -65,12 +65,12 @@ public VariadicArrayParametersDelegate CachedCompileLambda(LambdaExpression lamb private LambdaExpression getClosureFreeKeyForCaching(ConstantExtractor.ExtractionResult extractionResult, IReadOnlyCollection<ParameterExpression> parameterExpressions) { var e = SimpleParameterSubstituter.SubstituteParameter(extractionResult.ConstantfreeExpression, extractionResult.ConstantfreeExpression.Parameters.Select( - p => (Expression) Expression.Constant(GetDefaultValue(p.Type), p.Type))); + p => (Expression) Expression.Constant(getDefaultValue(p.Type), p.Type))); return Expression.Lambda(e, parameterExpressions); } - private object GetDefaultValue(Type t) { + private static object getDefaultValue(Type t) { if (t.IsValueType) { return Activator.CreateInstance(t); }