ReplacingEV uses two arrays instead of dictionary (#19858)

ReplacingExpressionVisitor held original and replacement expressions in a dictionary. Unfortunately that means hash code calculation occurs exponentially, as each TryGetValue on the dictionary must traverse the entire tree. Replaced with two lists and a simple Equals check, which is much faster. Fixes #19737
dotnet · Feb 10, 2020 · 7c9fc8a · 7c9fc8a
1 parent d751472
commit 7c9fc8a
Show file tree

Hide file tree

Showing 9 changed files with 41 additions and 57 deletions.
diff --git a/src/EFCore.InMemory/Query/Internal/InMemoryQueryExpression.cs b/src/EFCore.InMemory/Query/Internal/InMemoryQueryExpression.cs
@@ -468,10 +468,8 @@ public virtual void AddInnerJoin(
             var resultValueBufferExpressions = new List<Expression>();
             var projectionMapping = new Dictionary<ProjectionMember, Expression>();
             var replacingVisitor = new ReplacingExpressionVisitor(
-                new Dictionary<Expression, Expression>
-                {
-                    { CurrentParameter, outerParameter }, { innerQueryExpression.CurrentParameter, innerParameter }
-                });
+                new Expression[] { CurrentParameter, innerQueryExpression.CurrentParameter },
+                new Expression[] { outerParameter, innerParameter });
 
             var index = 0;
             var outerMemberInfo = transparentIdentifierType.GetTypeInfo().GetDeclaredField("Outer");
@@ -583,11 +581,8 @@ public virtual void AddLeftJoin(
             var resultValueBufferExpressions = new List<Expression>();
             var projectionMapping = new Dictionary<ProjectionMember, Expression>();
             var replacingVisitor = new ReplacingExpressionVisitor(
-                new Dictionary<Expression, Expression>
-                {
-                    { CurrentParameter, MakeMemberAccess(outerParameter, outerMemberInfo) },
-                    { innerQueryExpression.CurrentParameter, innerParameter }
-                });
+                new Expression[] { CurrentParameter, innerQueryExpression.CurrentParameter },
+                new Expression[] { MakeMemberAccess(outerParameter, outerMemberInfo), innerParameter});
 
             var index = 0;
             outerMemberInfo = transparentIdentifierType.GetTypeInfo().GetDeclaredField("Outer");
@@ -684,10 +679,8 @@ public virtual void AddSelectMany(
             var resultValueBufferExpressions = new List<Expression>();
             var projectionMapping = new Dictionary<ProjectionMember, Expression>();
             var replacingVisitor = new ReplacingExpressionVisitor(
-                new Dictionary<Expression, Expression>
-                {
-                    { CurrentParameter, outerParameter }, { innerQueryExpression.CurrentParameter, innerParameter }
-                });
+                new Expression[] { CurrentParameter, innerQueryExpression.CurrentParameter },
+                new Expression[] { outerParameter, innerParameter });
 
             var index = 0;
             var outerMemberInfo = transparentIdentifierType.GetTypeInfo().GetDeclaredField("Outer");
@@ -810,11 +803,8 @@ public virtual EntityShaperExpression AddNavigationToWeakEntityType(
             var resultValueBufferExpressions = new List<Expression>();
             var projectionMapping = new Dictionary<ProjectionMember, Expression>();
             var replacingVisitor = new ReplacingExpressionVisitor(
-                new Dictionary<Expression, Expression>
-                {
-                    { CurrentParameter, MakeMemberAccess(outerParameter, outerMemberInfo) },
-                    { innerQueryExpression.CurrentParameter, innerParameter }
-                });
+                new Expression[] { CurrentParameter, innerQueryExpression.CurrentParameter },
+                new Expression[] { MakeMemberAccess(outerParameter, outerMemberInfo), innerParameter});
             var index = 0;
 
             EntityProjectionExpression copyEntityProjectionToOuter(EntityProjectionExpression entityProjection)

diff --git a/src/EFCore.InMemory/Query/Internal/InMemoryQueryableMethodTranslatingExpressionVisitor.cs b/src/EFCore.InMemory/Query/Internal/InMemoryQueryableMethodTranslatingExpressionVisitor.cs
@@ -296,11 +296,8 @@ protected override ShapedQueryExpression TranslateGroupBy(
                 var original2 = resultSelector.Parameters[1];
 
                 var newResultSelectorBody = new ReplacingExpressionVisitor(
-                    new Dictionary<Expression, Expression>
-                    {
-                        { original1, groupByShaper.KeySelector },
-                        { original2, groupByShaper }
-                    }).Visit(resultSelector.Body);
+                    new Expression[] { original1, original2 },
+                    new[] { groupByShaper.KeySelector, groupByShaper }).Visit(resultSelector.Body);
 
                 newResultSelectorBody = ExpandWeakEntities(inMemoryQueryExpression, newResultSelectorBody);
                 var newShaper = _projectionBindingExpressionVisitor.Translate(inMemoryQueryExpression, newResultSelectorBody);

diff --git a/src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs b/src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
@@ -350,7 +350,8 @@ protected override ShapedQueryExpression TranslateGroupBy(
                 var original2 = resultSelector.Parameters[1];
 
                 var newResultSelectorBody = new ReplacingExpressionVisitor(
-                        new Dictionary<Expression, Expression> { { original1, translatedKey }, { original2, groupByShaper } })
+                        new Expression[] { original1, original2 },
+                        new[] { translatedKey, groupByShaper })
                     .Visit(resultSelector.Body);
 
                 newResultSelectorBody = ExpandWeakEntities(selectExpression, newResultSelectorBody);

diff --git a/src/EFCore/ChangeTracking/ValueComparer.cs b/src/EFCore/ChangeTracking/ValueComparer.cs
@@ -146,7 +146,7 @@ public virtual Expression ExtractEqualsBody(
             var original2 = EqualsExpression.Parameters[1];
 
             return new ReplacingExpressionVisitor(
-                    new Dictionary<Expression, Expression> { { original1, leftExpression }, { original2, rightExpression } })
+                    new Expression[] { original1, original2 }, new[] { leftExpression, rightExpression })
                 .Visit(EqualsExpression.Body);
         }
 

diff --git a/src/EFCore/Query/Internal/EntityEqualityRewritingExpressionVisitor.cs b/src/EFCore/Query/Internal/EntityEqualityRewritingExpressionVisitor.cs
@@ -795,7 +795,7 @@ private LambdaExpression RewriteAndVisitLambda(
                 lambda.Type,
                 Visit(
                     new ReplacingExpressionVisitor(
-                            new Dictionary<Expression, Expression> { { original1, replacement1 }, { original2, replacement2 } })
+                            new[] { original1, original2 }, new[] { replacement1, replacement2 })
                         .Visit(lambda.Body)),
                 lambda.TailCall,
                 lambda.Parameters);

diff --git a/src/EFCore/Query/Internal/InvocationExpressionRemovingExpressionVisitor.cs b/src/EFCore/Query/Internal/InvocationExpressionRemovingExpressionVisitor.cs
@@ -3,6 +3,7 @@
 
 using System.Collections.Generic;
 using System.Collections.ObjectModel;
+using System.Linq;
 using System.Linq.Expressions;
 using Microsoft.EntityFrameworkCore.Utilities;
 
@@ -35,14 +36,8 @@ private Expression StripTrivialConversions(Expression expression)
         }
 
         private Expression InlineLambdaExpression(LambdaExpression lambdaExpression, ReadOnlyCollection<Expression> arguments)
-        {
-            var replacements = new Dictionary<Expression, Expression>(arguments.Count);
-            for (var i = 0; i < lambdaExpression.Parameters.Count; i++)
-            {
-                replacements.Add(lambdaExpression.Parameters[i], arguments[i]);
-            }
-
-            return new ReplacingExpressionVisitor(replacements).Visit(lambdaExpression.Body);
-        }
+            => new ReplacingExpressionVisitor(
+                lambdaExpression.Parameters.ToArray<Expression>(), arguments.ToArray())
+                .Visit(lambdaExpression.Body);
     }
 }
diff --git a/src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs b/src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs
@@ -860,11 +860,9 @@ private NavigationExpansionExpression ProcessJoin(
 
             var currentTree = new NavigationTreeNode(outerSource.CurrentTree, innerSource.CurrentTree);
             var pendingSelector = new ReplacingExpressionVisitor(
-                new Dictionary<Expression, Expression>
-                {
-                    { resultSelector.Parameters[0], outerSource.PendingSelector },
-                    { resultSelector.Parameters[1], innerSource.PendingSelector }
-                }).Visit(resultSelector.Body);
+                new Expression[] { resultSelector.Parameters[0], resultSelector.Parameters[1] },
+                new[] { outerSource.PendingSelector, innerSource.PendingSelector })
+                .Visit(resultSelector.Body);
             var parameterName = GetParameterName("ti");
 
             return new NavigationExpansionExpression(source, currentTree, pendingSelector, parameterName);
@@ -914,11 +912,9 @@ private NavigationExpansionExpression ProcessLeftJoin(
 
             var currentTree = new NavigationTreeNode(outerSource.CurrentTree, innerSource.CurrentTree);
             var pendingSelector = new ReplacingExpressionVisitor(
-                new Dictionary<Expression, Expression>
-                {
-                    { resultSelector.Parameters[0], outerSource.PendingSelector },
-                    { resultSelector.Parameters[1], innerPendingSelector }
-                }).Visit(resultSelector.Body);
+                new Expression[] { resultSelector.Parameters[0], resultSelector.Parameters[1] },
+                new[] { outerSource.PendingSelector, innerPendingSelector})
+                .Visit(resultSelector.Body);
             var parameterName = GetParameterName("ti");
 
             return new NavigationExpansionExpression(source, currentTree, pendingSelector, parameterName);
@@ -1039,10 +1035,9 @@ private NavigationExpansionExpression ProcessSelectMany(
                 var pendingSelector = resultSelector == null
                     ? innerTree
                     : new ReplacingExpressionVisitor(
-                        new Dictionary<Expression, Expression>
-                        {
-                            { resultSelector.Parameters[0], source.PendingSelector }, { resultSelector.Parameters[1], innerTree }
-                        }).Visit(resultSelector.Body);
+                        new Expression[] { resultSelector.Parameters[0], resultSelector.Parameters[1] },
+                        new[] { source.PendingSelector, innerTree })
+                        .Visit(resultSelector.Body);
                 var parameterName = GetParameterName("ti");
 
                 return new NavigationExpansionExpression(newSource, currentTree, pendingSelector, parameterName);

diff --git a/src/EFCore/Query/QueryableMethodTranslatingExpressionVisitor.cs b/src/EFCore/Query/QueryableMethodTranslatingExpressionVisitor.cs
@@ -486,7 +486,7 @@ protected virtual ShapedQueryExpression TranslateResultSelectorForJoin(
             var replacement2 = AccessInnerTransparentField(transparentIdentifierType, transparentIdentifierParameter);
             var newResultSelector = Expression.Lambda(
                 new ReplacingExpressionVisitor(
-                        new Dictionary<Expression, Expression> { { original1, replacement1 }, { original2, replacement2 } })
+                    new[] { original1, original2 }, new[] { replacement1, replacement2 })
                     .Visit(resultSelector.Body),
                 transparentIdentifierParameter);
 

diff --git a/src/EFCore/Query/ReplacingExpressionVisitor.cs b/src/EFCore/Query/ReplacingExpressionVisitor.cs
@@ -1,7 +1,6 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
-using System.Collections.Generic;
 using System.Linq;
 using System.Linq.Expressions;
 using System.Reflection;
@@ -14,22 +13,24 @@ namespace Microsoft.EntityFrameworkCore.Query
 {
     public class ReplacingExpressionVisitor : ExpressionVisitor
     {
-        private readonly IDictionary<Expression, Expression> _replacements;
+        private readonly Expression[] _originals;
+        private readonly Expression[] _replacements;
 
         public static Expression Replace([NotNull] Expression original, [NotNull] Expression replacement, [NotNull] Expression tree)
         {
             Check.NotNull(original, nameof(original));
             Check.NotNull(replacement, nameof(replacement));
             Check.NotNull(tree, nameof(tree));
 
-            return new ReplacingExpressionVisitor(
-                new Dictionary<Expression, Expression> { { original, replacement } }).Visit(tree);
+            return new ReplacingExpressionVisitor(new[] { original }, new[] { replacement }).Visit(tree);
         }
 
-        public ReplacingExpressionVisitor([NotNull] IDictionary<Expression, Expression> replacements)
+        public ReplacingExpressionVisitor([NotNull] Expression[] originals, [NotNull] Expression[] replacements)
         {
+            Check.NotNull(originals, nameof(originals));
             Check.NotNull(replacements, nameof(replacements));
 
+            _originals = originals;
             _replacements = replacements;
         }
 
@@ -40,9 +41,14 @@ public override Expression Visit(Expression expression)
                 return expression;
             }
 
-            if (_replacements.TryGetValue(expression, out var replacement))
+            // We use two arrays rather than a dictionary because hash calculation here can be prohibitively expensive
+            // for deep trees. Locality of reference makes arrays better for the small number of replacements anyway.
+            for (var i = 0; i < _originals.Length; i++)
             {
-                return replacement;
+                if (expression.Equals(_originals[i]))
+                {
+                    return _replacements[i];
+                }
             }
 
             return base.Visit(expression);