Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ReplacingEV uses two arrays instead of dictionary #19858

Merged
merged 2 commits into from
Feb 10, 2020
Merged

ReplacingEV uses two arrays instead of dictionary #19858

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fb1d5c0
ReplacingEV uses two arrays instead of dictionary
roji Feb 10, 2020
f70b9cc
Added comment
roji Feb 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 8 additions & 18 deletions src/EFCore.InMemory/Query/Internal/InMemoryQueryExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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)
Expand Down
7 changes: 2 additions & 5 deletions src/EFCore.InMemory/Query/Internal/InMemoryQueryableMethodTranslatingExpressionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
3 changes: 2 additions & 1 deletion src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/EFCore/ChangeTracking/ValueComparer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
2 changes: 1 addition & 1 deletion src/EFCore/Query/Internal/EntityEqualityRewritingExpressionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
13 changes: 4 additions & 9 deletions src/EFCore/Query/Internal/InvocationExpressionRemovingExpressionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Linq;
using System.Linq.Expressions;
using Microsoft.EntityFrameworkCore.Utilities;

Expand Down Expand Up @@ -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);
}
}
23 changes: 9 additions & 14 deletions src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/EFCore/Query/QueryableMethodTranslatingExpressionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
20 changes: 13 additions & 7 deletions src/EFCore/Query/ReplacingExpressionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking change if patching.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. We could leave the previous constructor and build arrays out of the keys/values.

{
Check.NotNull(originals, nameof(originals));
Check.NotNull(replacements, nameof(replacements));

_originals = originals;
_replacements = replacements;
}

Expand All @@ -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++)
roji marked this conversation as resolved.
Show resolved Hide resolved
{
return replacement;
if (expression.Equals(_originals[i]))
{
return _replacements[i];
}
}

return base.Visit(expression);
Expand Down