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 (cherry picked from commit 7c9fc8a)
dotnet · Feb 14, 2020 · 0344ad7 · 0344ad7
1 parent 0fbd787
commit 0344ad7
Show file tree

Hide file tree

Showing 9 changed files with 84 additions and 56 deletions.
diff --git a/src/EFCore.InMemory/Query/Internal/InMemoryQueryExpression.cs b/src/EFCore.InMemory/Query/Internal/InMemoryQueryExpression.cs
@@ -474,10 +474,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");
@@ -589,11 +587,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");
@@ -689,10 +684,8 @@ public virtual void AddSelectMany(InMemoryQueryExpression innerQueryExpression,
             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");
@@ -815,11 +808,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
@@ -258,11 +258,9 @@ protected override ShapedQueryExpression TranslateGroupBy(
                 var original2 = resultSelector.Parameters[1];
 
                 var newResultSelectorBody = new ReplacingExpressionVisitor(
-                    new Dictionary<Expression, Expression>
-                    {
-                        { original1, ((GroupByShaperExpression)source.ShaperExpression).KeySelector },
-                        { original2, source.ShaperExpression }
-                    }).Visit(resultSelector.Body);
+                        new Expression[] { original1, original2 },
+                        new[] { ((GroupByShaperExpression)source.ShaperExpression).KeySelector, source.ShaperExpression })
+                    .Visit(resultSelector.Body);
 
                 newResultSelectorBody = ExpandWeakEntities(inMemoryQueryExpression, newResultSelectorBody);
 

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

diff --git a/src/EFCore/ChangeTracking/ValueComparer.cs b/src/EFCore/ChangeTracking/ValueComparer.cs
@@ -137,7 +137,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
@@ -757,7 +757,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;
 
 namespace Microsoft.EntityFrameworkCore.Query.Internal
@@ -32,14 +33,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
@@ -880,11 +880,9 @@ private Expression 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);
@@ -937,11 +935,9 @@ private Expression 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);
@@ -1060,10 +1056,9 @@ private Expression 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
@@ -470,7 +470,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,6 +1,7 @@
 // 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;
 using System.Collections.Generic;
 using System.Linq;
 using System.Linq.Expressions;
@@ -11,17 +12,50 @@ namespace Microsoft.EntityFrameworkCore.Query
 {
     public class ReplacingExpressionVisitor : ExpressionVisitor
     {
-        private readonly IDictionary<Expression, Expression> _replacements;
+        private readonly bool _quirkMode;
+
+        private readonly Expression[] _originals;
+        private readonly Expression[] _replacements;
+
+        private readonly IDictionary<Expression, Expression> _quirkReplacements;
 
         public static Expression Replace(Expression original, Expression replacement, Expression tree)
         {
-            return new ReplacingExpressionVisitor(
-                new Dictionary<Expression, Expression> { { original, replacement } }).Visit(tree);
+            return new ReplacingExpressionVisitor(new[] { original }, new[] { replacement }).Visit(tree);
+        }
+
+        public ReplacingExpressionVisitor(Expression[] originals, Expression[] replacements)
+        {
+            _quirkMode = AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue19737", out var enabled) && enabled;
+
+            if (_quirkMode)
+            {
+                _quirkReplacements = new Dictionary<Expression, Expression>();
+                for (var i = 0; i < originals.Length; i++)
+                {
+                    _quirkReplacements[originals[i]] = replacements[i];
+                }
+            }
+            else
+            {
+                _originals = originals;
+                _replacements = replacements;
+            }
         }
 
         public ReplacingExpressionVisitor(IDictionary<Expression, Expression> replacements)
         {
-            _replacements = replacements;
+            _quirkMode = AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue19737", out var enabled) && enabled;
+
+            if (_quirkMode)
+            {
+                _quirkReplacements = replacements;
+            }
+            else
+            {
+                _originals = replacements.Keys.ToArray();
+                _replacements = replacements.Values.ToArray();
+            }
         }
 
         public override Expression Visit(Expression expression)
@@ -31,9 +65,24 @@ public override Expression Visit(Expression expression)
                 return expression;
             }
 
-            if (_replacements.TryGetValue(expression, out var replacement))
+            if (_quirkMode)
+            {
+                if (_quirkReplacements.TryGetValue(expression, out var replacement))
+                {
+                    return replacement;
+                }
+            }
+            else
             {
-                return 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++)
+                {
+                    if (expression.Equals(_originals[i]))
+                    {
+                        return _replacements[i];
+                    }
+                }
             }
 
             return base.Visit(expression);