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

Implementing C# null semantics in the new pipeline #15745

Merged
merged 1 commit into from
May 20, 2019
Merged

Implementing C# null semantics in the new pipeline #15745

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
519 changes: 519 additions & 0 deletions src/EFCore.Relational/Query/PipeLine/NullSemanticsRewritingVisitor.cs
View file
Open in desktop

Large diffs are not rendered by default.

196 changes: 196 additions & 0 deletions src/EFCore.Relational/Query/PipeLine/SqlExpressionOptimizingVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,196 @@
// 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.Expressions;
using Microsoft.EntityFrameworkCore.Relational.Query.Pipeline;
using Microsoft.EntityFrameworkCore.Relational.Query.Pipeline.SqlExpressions;

namespace Microsoft.EntityFrameworkCore.Query.Pipeline
{
public class SqlExpressionOptimizingVisitor : ExpressionVisitor
{
private readonly ISqlExpressionFactory _sqlExpressionFactory;

private readonly Dictionary<ExpressionType, ExpressionType> _expressionTypesNegationMap
Copy link
Member

Choose a reason for hiding this comment

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

C# 8 switch expression FTW (more terse, more efficient):

Suggested change
private readonly Dictionary<ExpressionType, ExpressionType> _expressionTypesNegationMap
private static ExpressionType Negate(ExpressionType expressionType)
=> expressionType switch {
ExpressionType.Equal => ExpressionType.NotEqual,
ExpressionType.NotEqual => ExpressionType.Equal,
ExpressionType.GreaterThan => ExpressionType.LessThanOrEqual,
ExpressionType.GreaterThanOrEqual => ExpressionType.LessThan,
ExpressionType.LessThan => ExpressionType.GreaterThanOrEqual,
ExpressionType.LessThanOrEqual => ExpressionType.GreaterThan
};

If you need to check whether negation is possible, we can have TryNegate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Which one would be more performant? Readonly dictionary should give O(1) access. Methodcall may need to check through all cases (rejected ones) before reaching correct one.

Copy link
Member

@roji roji May 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

A switch over an enum is a switch over its underlying type - so over an int here, which in most cases will do an efficient lookup table under the hood. This is in general much faster than any dictionary - see benchmark below. Also no allocations whatsoever for the dictionary and its internals.

Note that the difference here specifically may be negligible in the grander scheme - especially since this is query compilation only - but a switch expression also seems more terse so why not.

And because it's so easy to do, here's a benchmark comparison:

// * Summary *

BenchmarkDotNet=v0.11.5, OS=ubuntu 19.04
Intel Core i7-6700HQ CPU 2.60GHz (Skylake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=3.0.100-preview4-011223
[Host] : .NET Core 3.0.0-preview5-27626-15 (CoreCLR 4.6.27622.75, CoreFX 4.700.19.22408), 64bit RyuJIT
DefaultJob : .NET Core 3.0.0-preview5-27626-15 (CoreCLR 4.6.27622.75, CoreFX 4.700.19.22408), 64bit RyuJIT

Method Mean Error StdDev Median
SwitchExpression 2.553 ns 0.0884 ns 0.1150 ns 2.533 ns
Dictionary 7.055 ns 0.1993 ns 0.4775 ns 6.813 ns 
public class SwitchEnum
{
    [Benchmark]
    public ExpressionType SwitchExpression()
        => SwitchMethod(ExpressionType.GreaterThanOrEqual);

    [Benchmark]
    public ExpressionType Dictionary()
        => _dictionary[ExpressionType.GreaterThanOrEqual];

    static readonly Dictionary<ExpressionType, ExpressionType> _dictionary
        = new Dictionary<ExpressionType, ExpressionType>
    {
        { ExpressionType.Equal, ExpressionType.NotEqual },
        { ExpressionType.NotEqual, ExpressionType.Equal },
        { ExpressionType.GreaterThan, ExpressionType.LessThanOrEqual },
        { ExpressionType.GreaterThanOrEqual, ExpressionType.LessThan },
        { ExpressionType.LessThan, ExpressionType.GreaterThanOrEqual },
        { ExpressionType.LessThanOrEqual, ExpressionType.GreaterThan },
    };

    static ExpressionType SwitchMethod(ExpressionType expressionType)
        => expressionType switch {
            ExpressionType.Equal => ExpressionType.NotEqual,
            ExpressionType.NotEqual => ExpressionType.Equal,
            ExpressionType.GreaterThan => ExpressionType.LessThanOrEqual,
            ExpressionType.GreaterThanOrEqual => ExpressionType.LessThan,
            ExpressionType.LessThan => ExpressionType.GreaterThanOrEqual,
            ExpressionType.LessThanOrEqual => ExpressionType.GreaterThan,
            _ => throw new ArgumentException(nameof(expressionType))
        };
    }

Copy link
Contributor

Choose a reason for hiding this comment

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

If switch is faster let's use it. Is it specific for Enum case only of any type? I would guess not all pattern matching is as cheap as enums over int.

Copy link
Member

Choose a reason for hiding this comment

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

Here are the results for string keys, switch wins as well:

Method Mean Error StdDev
SwitchExpression 13.29 ns 0.2945 ns 0.4317 ns
Dictionary 23.10 ns 0.4976 ns 1.0387 ns

In general, for small datasets switch should (almost) always be faster than a dictionary - when in doubt we can always do a quick benchmark to find out.

= new Dictionary<ExpressionType, ExpressionType>
{
{ ExpressionType.AndAlso, ExpressionType.OrElse },
{ ExpressionType.OrElse, ExpressionType.AndAlso },
{ ExpressionType.Equal, ExpressionType.NotEqual },
{ ExpressionType.NotEqual, ExpressionType.Equal },
{ ExpressionType.GreaterThan, ExpressionType.LessThanOrEqual },
{ ExpressionType.GreaterThanOrEqual, ExpressionType.LessThan },
{ ExpressionType.LessThan, ExpressionType.GreaterThanOrEqual },
{ ExpressionType.LessThanOrEqual, ExpressionType.GreaterThan },
};

public SqlExpressionOptimizingVisitor(ISqlExpressionFactory sqlExpressionFactory)
{
_sqlExpressionFactory = sqlExpressionFactory;
}

protected override Expression VisitExtension(Expression extensionExpression)
{
if (extensionExpression is SqlUnaryExpression sqlUnaryExpression)
{
return VisitSqlUnaryExpression(sqlUnaryExpression);
}

if (extensionExpression is SqlBinaryExpression sqlBinaryExpression)
{
return VisitSqlBinaryExpression(sqlBinaryExpression);
}

return base.VisitExtension(extensionExpression);
}

private Expression VisitSqlUnaryExpression(SqlUnaryExpression sqlUnaryExpression)
{
// !(true) -> false
// !(false) -> true
if (sqlUnaryExpression.OperatorType == ExpressionType.Not
&& sqlUnaryExpression.Operand is SqlConstantExpression innerConstantBool
&& innerConstantBool.Value is bool value)
{
return value
? _sqlExpressionFactory.Constant(false, sqlUnaryExpression.TypeMapping)
: _sqlExpressionFactory.Constant(true, sqlUnaryExpression.TypeMapping);
}

// NULL IS NULL -> true
// non_nullablee_constant IS NULL -> false
if (sqlUnaryExpression.OperatorType == ExpressionType.Equal
&& sqlUnaryExpression.Operand is SqlConstantExpression innerConstantNull1)
{
return _sqlExpressionFactory.Constant(innerConstantNull1.Value == null, sqlUnaryExpression.TypeMapping);
}

// NULL IS NOT NULL -> false
// non_nullablee_constant IS NOT NULL -> true
if (sqlUnaryExpression.OperatorType == ExpressionType.NotEqual
&& sqlUnaryExpression.Operand is SqlConstantExpression innerConstantNull2)
{
return _sqlExpressionFactory.Constant(innerConstantNull2.Value != null, sqlUnaryExpression.TypeMapping);
}

if (sqlUnaryExpression.Operand is SqlUnaryExpression innerUnary)
{
if (sqlUnaryExpression.OperatorType == ExpressionType.Not)
{
// !(!a) -> a
if (innerUnary.OperatorType == ExpressionType.Not)
{
return Visit(innerUnary.Operand);
}

if (innerUnary.OperatorType == ExpressionType.Equal)
{
//!(a IS NULL) -> a IS NOT NULL
return Visit(_sqlExpressionFactory.IsNotNull(innerUnary.Operand));
}

//!(a IS NOT NULL) -> a IS NULL
if (innerUnary.OperatorType == ExpressionType.NotEqual)
{
return Visit(_sqlExpressionFactory.IsNull(innerUnary.Operand));
}
}

// (!a) IS NULL <==> a IS NULL
if (sqlUnaryExpression.OperatorType == ExpressionType.Equal
&& innerUnary.OperatorType == ExpressionType.Not)
{
return Visit(_sqlExpressionFactory.IsNull(innerUnary.Operand));
}

// (!a) IS NOT NULL <==> a IS NOT NULL
if (sqlUnaryExpression.OperatorType == ExpressionType.NotEqual
&& innerUnary.OperatorType == ExpressionType.Not)
{
return Visit(_sqlExpressionFactory.IsNotNull(innerUnary.Operand));
}
}

if (sqlUnaryExpression.Operand is SqlBinaryExpression innerBinary)
{
// De Morgan's
if (innerBinary.OperatorType == ExpressionType.AndAlso
|| innerBinary.OperatorType == ExpressionType.OrElse)
{
var newLeft = (SqlExpression)Visit(_sqlExpressionFactory.Not(innerBinary.Left));
var newRight = (SqlExpression)Visit(_sqlExpressionFactory.Not(innerBinary.Right));

return innerBinary.OperatorType == ExpressionType.AndAlso
? _sqlExpressionFactory.OrElse(newLeft, newRight)
: _sqlExpressionFactory.AndAlso(newLeft, newRight);
}

// note that those optimizations are only valid in 2-value logic
// they are safe to do here because null semantics removes possibility of nulls in the tree
// however if we decide to do "partial" null semantics (that doesn't distinguish between NULL and FALSE, e.g. for predicates)
// we need to be extra careful here
if (_expressionTypesNegationMap.ContainsKey(innerBinary.OperatorType))
{
return Visit(
_sqlExpressionFactory.MakeBinary(
_expressionTypesNegationMap[innerBinary.OperatorType],
innerBinary.Left,
innerBinary.Right,
innerBinary.TypeMapping));
}
}

var newOperand = (SqlExpression)Visit(sqlUnaryExpression.Operand);

return sqlUnaryExpression.Update(newOperand);
}

private Expression VisitSqlBinaryExpression(SqlBinaryExpression sqlBinaryExpression)
{
var newLeft = (SqlExpression)Visit(sqlBinaryExpression.Left);
var newRight = (SqlExpression)Visit(sqlBinaryExpression.Right);

if (sqlBinaryExpression.OperatorType == ExpressionType.AndAlso
|| sqlBinaryExpression.OperatorType == ExpressionType.OrElse)
{

var newLeftConstant = newLeft as SqlConstantExpression;
var newRightConstant = newRight as SqlConstantExpression;

// true && a -> a
// true || a -> true
// false && a -> false
// false || a -> a
if (newLeftConstant != null)
{
return sqlBinaryExpression.OperatorType == ExpressionType.AndAlso
? (bool)newLeftConstant.Value
? newRight
: newLeftConstant
: (bool)newLeftConstant.Value
? newLeftConstant
: newRight;
}
else if (newRightConstant != null)
{
// a && true -> a
// a || true -> true
// a && false -> false
// a || false -> a
return sqlBinaryExpression.OperatorType == ExpressionType.AndAlso
? (bool)newRightConstant.Value
? newLeft
: newRightConstant
: (bool)newRightConstant.Value
? newRightConstant
: newLeft;
}

return sqlBinaryExpression.Update(newLeft, newRight);
}

return sqlBinaryExpression.Update(newLeft, newRight);
}
}
}
17 changes: 15 additions & 2 deletions src/EFCore.Relational/Query/Pipeline/RelationalShapedQueryOptimizingExpressionVisitors.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,25 +2,38 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Linq.Expressions;
using Microsoft.EntityFrameworkCore.Infrastructure;
using Microsoft.EntityFrameworkCore.Query.Pipeline;

namespace Microsoft.EntityFrameworkCore.Relational.Query.Pipeline
{
public class RelationalShapedQueryOptimizer : ShapedQueryOptimizer
{
private QueryCompilationContext2 _queryCompilationContext;
private readonly QueryCompilationContext2 _queryCompilationContext;

public RelationalShapedQueryOptimizer(QueryCompilationContext2 queryCompilationContext)
public RelationalShapedQueryOptimizer(
QueryCompilationContext2 queryCompilationContext,
ISqlExpressionFactory sqlExpressionFactory)
{
_queryCompilationContext = queryCompilationContext;
SqlExpressionFactory = sqlExpressionFactory;
}

protected ISqlExpressionFactory SqlExpressionFactory { get; private set; }

public override Expression Visit(Expression query)
{
query = base.Visit(query);
query = new ShaperExpressionDedupingExpressionVisitor().Process(query);
query = new SelectExpressionProjectionApplyingExpressionVisitor().Visit(query);
query = new SelectExpressionTableAliasUniquifyingExpressionVisitor().Visit(query);

if (!RelationalOptionsExtension.Extract(_queryCompilationContext.ContextOptions).UseRelationalNulls)
{
query = new NullSemanticsRewritingVisitor(SqlExpressionFactory).Visit(query);
}

query = new SqlExpressionOptimizingVisitor(SqlExpressionFactory).Visit(query);
query = new NullComparisonTransformingExpressionVisitor().Visit(query);

return query;
Expand Down
9 changes: 8 additions & 1 deletion ...ore.Relational/Query/Pipeline/RelationalShapedQueryOptimizingExpressionVisitorsFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,16 @@ namespace Microsoft.EntityFrameworkCore.Relational.Query.Pipeline
{
public class RelationalShapedQueryOptimizerFactory : ShapedQueryOptimizerFactory
{
protected ISqlExpressionFactory SqlExpressionFactory { get; private set; }

public RelationalShapedQueryOptimizerFactory(ISqlExpressionFactory sqlExpressionFactory)
{
SqlExpressionFactory = sqlExpressionFactory;
}

public override ShapedQueryOptimizer Create(QueryCompilationContext2 queryCompilationContext)
{
return new RelationalShapedQueryOptimizer(queryCompilationContext);
return new RelationalShapedQueryOptimizer(queryCompilationContext, SqlExpressionFactory);
}
}
}
8 changes: 6 additions & 2 deletions src/EFCore.Relational/Query/Pipeline/SqlExpressions/CaseExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,13 @@ protected override Expression VisitChildren(ExpressionVisitor visitor)
}

public virtual CaseExpression Update(
SqlExpression operand, IReadOnlyList<CaseWhenClause> whenClauses, SqlExpression elseResult)
SqlExpression operand,
IReadOnlyList<CaseWhenClause> whenClauses,
SqlExpression elseResult)
{
return new CaseExpression(operand, whenClauses, elseResult);
return operand != Operand || !whenClauses.SequenceEqual(WhenClauses) || elseResult != ElseResult
roji marked this conversation as resolved.
Show resolved Hide resolved
? new CaseExpression(operand, whenClauses, elseResult)
: this;
}

#endregion
Expand Down
2 changes: 1 addition & 1 deletion src/EFCore.Relational/Query/Pipeline/SqlExpressions/SqlBinaryExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ public override void Print(ExpressionPrinter expressionPrinter)
expressionPrinter.StringBuilder.Append(")");
}

expressionPrinter.StringBuilder.Append(" " + expressionPrinter.GenerateBinaryOperator(OperatorType) + " ");
expressionPrinter.StringBuilder.Append(expressionPrinter.GenerateBinaryOperator(OperatorType));

requiresBrackets = RequiresBrackets(Right);

Expand Down
2 changes: 1 addition & 1 deletion src/EFCore.Relational/Query/Pipeline/SqlExpressions/SqlFunctionExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ public SqlFunctionExpression ApplyTypeMapping(RelationalTypeMapping typeMapping)

public SqlFunctionExpression Update(SqlExpression instance, IReadOnlyList<SqlExpression> arguments)
{
return instance != Instance || arguments != Arguments
return instance != Instance || !arguments.SequenceEqual(Arguments)
smitpatel marked this conversation as resolved.
Show resolved Hide resolved
? new SqlFunctionExpression(instance, Schema, FunctionName, IsNiladic, arguments, Type, TypeMapping)
: this;
}
Expand Down
60 changes: 4 additions & 56 deletions src/EFCore.SqlServer/Query/Pipeline/SearchConditionConvertingExpressionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@ private Expression ApplyConversion(SqlExpression sqlExpression, bool condition)
: ConvertToValue(sqlExpression, condition);

private Expression ConvertToSearchCondition(SqlExpression sqlExpression, bool condition)
=> condition
? sqlExpression
: BuildCompareToExpression(sqlExpression);
=> condition
roji marked this conversation as resolved.
Show resolved Hide resolved
? sqlExpression
: BuildCompareToExpression(sqlExpression);

private Expression ConvertToValue(SqlExpression sqlExpression, bool condition)
{
Expand Down Expand Up @@ -246,67 +246,15 @@ protected override Expression VisitSqlFunction(SqlFunctionExpression sqlFunction
{
var parentSearchCondition = _isSearchCondition;
_isSearchCondition = false;
var changed = false;
var instance = (SqlExpression)Visit(sqlFunctionExpression.Instance);
changed |= instance != sqlFunctionExpression.Instance;
var arguments = new SqlExpression[sqlFunctionExpression.Arguments.Count];
for (var i = 0; i < arguments.Length; i++)
{
arguments[i] = (SqlExpression)Visit(sqlFunctionExpression.Arguments[i]);
changed |= arguments[i] != sqlFunctionExpression.Arguments[i];
}

_isSearchCondition = parentSearchCondition;
SqlExpression newFunction;
if (changed)
{
if (sqlFunctionExpression.Instance != null)
{
if (sqlFunctionExpression.IsNiladic)
{
newFunction = _sqlExpressionFactory.Function(
instance,
sqlFunctionExpression.FunctionName,
sqlFunctionExpression.IsNiladic,
sqlFunctionExpression.Type,
sqlFunctionExpression.TypeMapping);
}
else
{
newFunction = _sqlExpressionFactory.Function(
instance,
sqlFunctionExpression.FunctionName,
arguments,
sqlFunctionExpression.Type,
sqlFunctionExpression.TypeMapping);
}
}
else
{
if (sqlFunctionExpression.IsNiladic)
{
newFunction = _sqlExpressionFactory.Function(
sqlFunctionExpression.Schema,
sqlFunctionExpression.FunctionName,
sqlFunctionExpression.IsNiladic,
sqlFunctionExpression.Type,
sqlFunctionExpression.TypeMapping);
}
else
{
newFunction = _sqlExpressionFactory.Function(
sqlFunctionExpression.Schema,
sqlFunctionExpression.FunctionName,
arguments,
sqlFunctionExpression.Type,
sqlFunctionExpression.TypeMapping);
}
}
}
else
{
newFunction = sqlFunctionExpression;
}
var newFunction = sqlFunctionExpression.Update(instance, arguments);

var condition = string.Equals(sqlFunctionExpression.FunctionName, "FREETEXT")
|| string.Equals(sqlFunctionExpression.FunctionName, "CONTAINS");
Expand Down
Loading