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

Reuse previous lambda body binding if possible #43366

Merged
merged 2 commits into from
Apr 20, 2020
Merged

Reuse previous lambda body binding if possible #43366

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
b43120a
Reuse previous lambda body binding if possible
cston Mar 30, 2020
ad529d9
PR feedback
cston Apr 19, 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
11 changes: 11 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/Binder.QueryUnboundLambdaState.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System;
using System.Collections.Immutable;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.CSharp
{
Expand Down Expand Up @@ -54,6 +55,16 @@ protected override UnboundLambdaState WithCachingCore(bool includeCache)
return new QueryUnboundLambdaState(Binder, _rangeVariableMap, _parameters, _bodyFactory, includeCache);
}

protected override BoundExpression GetLambdaExpressionBody(BoundBlock body)
{
return null;
}

protected override BoundBlock CreateBlockFromLambdaExpressionBody(Binder lambdaBodyBinder, BoundExpression expression, DiagnosticBag diagnostics)
{
throw ExceptionUtilities.Unreachable;
}

protected override BoundBlock BindLambdaBody(LambdaSymbol lambdaSymbol, Binder lambdaBodyBinder, DiagnosticBag diagnostics)
{
return _bodyFactory(lambdaSymbol, lambdaBodyBinder, diagnostics);
Expand Down
15 changes: 11 additions & 4 deletions src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@
using System.Collections.Immutable;
using System.Diagnostics;
using System.Linq;
using System.Threading.Tasks;
using System.Transactions;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.PooledObjects;
Expand Down Expand Up @@ -3155,7 +3153,7 @@ private static bool IsValidExpressionBody(SyntaxNode expressionSyntax, BoundExpr
}

/// <summary>
/// Binds an expression-bodied member with expression e as either { return e;} or { e; }.
/// Binds an expression-bodied member with expression e as either { return e; } or { e; }.
/// </summary>
internal virtual BoundBlock BindExpressionBodyAsBlock(ArrowExpressionClauseSyntax expressionBody,
DiagnosticBag diagnostics)
Expand All @@ -3179,7 +3177,7 @@ static BoundBlock bindExpressionBodyAsBlockInternal(ArrowExpressionClauseSyntax
}

/// <summary>
/// Binds a lambda with expression e as either { return e;} or { e; }.
/// Binds a lambda with expression e as either { return e; } or { e; }.
/// </summary>
public BoundBlock BindLambdaExpressionAsBlock(ExpressionSyntax body, DiagnosticBag diagnostics)
{
Expand All @@ -3195,6 +3193,15 @@ public BoundBlock BindLambdaExpressionAsBlock(ExpressionSyntax body, DiagnosticB
return bodyBinder.CreateBlockFromExpression(body, bodyBinder.GetDeclaredLocalsForScope(body), refKind, expression, expressionSyntax, diagnostics);
}

public BoundBlock CreateBlockFromExpression(ExpressionSyntax body, BoundExpression expression, DiagnosticBag diagnostics)
{
Binder bodyBinder = this.GetBinder(body);
Debug.Assert(bodyBinder != null);

Debug.Assert(body.Kind() != SyntaxKind.RefExpression);
return bodyBinder.CreateBlockFromExpression(body, bodyBinder.GetDeclaredLocalsForScope(body), RefKind.None, expression, body, diagnostics);
}

private BindValueKind GetRequiredReturnValueKind(RefKind refKind)
{
BindValueKind requiredValueKind = BindValueKind.RValue;
Expand Down
98 changes: 71 additions & 27 deletions src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ internal sealed partial class BoundLocalFunctionStatement : IBoundLambdaOrFuncti
BoundBlock IBoundLambdaOrFunction.Body { get => this.Body; }
}

internal struct InferredLambdaReturnType
internal readonly struct InferredLambdaReturnType
{
internal readonly int NumExpressions;
internal readonly bool HadExpressionlessReturn;
Expand Down Expand Up @@ -185,7 +185,9 @@ internal static InferredLambdaReturnType InferReturnType(ArrayBuilder<(BoundRetu

HashSet<DiagnosticInfo> useSiteDiagnostics = null;
var bestType = CalculateReturnType(compilation, conversions, delegateType, types, isAsync, node, ref useSiteDiagnostics);
return new InferredLambdaReturnType(types.Count, hasReturnWithoutArgument, refKind, bestType, useSiteDiagnostics.AsImmutableOrEmpty());
int numExpressions = types.Count;
types.Free();
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

types.Free(); [](start = 12, length = 13)

Were we leaking this builder before? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.

In reply to: 410970357 [](ancestors = 410970357)

return new InferredLambdaReturnType(numExpressions, hasReturnWithoutArgument, refKind, bestType, useSiteDiagnostics.AsImmutableOrEmpty());
}

private static TypeWithAnnotations CalculateReturnType(
Expand Down Expand Up @@ -467,6 +469,17 @@ internal UnboundLambdaState WithCaching(bool includeCache)
public abstract RefKind RefKind(int index);
protected abstract BoundBlock BindLambdaBody(LambdaSymbol lambdaSymbol, Binder lambdaBodyBinder, DiagnosticBag diagnostics);

/// <summary>
/// Return the bound expression if the lambda is has an expression body and can be reused easily.
Copy link
Contributor

Choose a reason for hiding this comment

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

is has [](start = 54, length = 6)

Typo?

/// This is an optimization only. Implementations can return null to skip reuse.
/// </summary>
protected abstract BoundExpression GetLambdaExpressionBody(BoundBlock body);

/// <summary>
/// Produce a bound block for the expression returned from GetLambdaExpressionBody.
/// </summary>
protected abstract BoundBlock CreateBlockFromLambdaExpressionBody(Binder lambdaBodyBinder, BoundExpression expression, DiagnosticBag diagnostics);

public virtual void GenerateAnonymousFunctionConversionError(DiagnosticBag diagnostics, TypeSymbol targetType)
{
this.Binder.GenerateAnonymousFunctionConversionError(diagnostics, _unboundLambda.Syntax, _unboundLambda, targetType);
Expand Down Expand Up @@ -541,20 +554,39 @@ private bool DelegateNeedsReturn(MethodSymbol invokeMethod)

private BoundLambda ReallyBind(NamedTypeSymbol delegateType)
{
// When binding for real (not for return inference), we cannot reuse a body of a lambda
// previously bound for return type inference because we have not converted the returned
// expression(s) to the return type.
var invokeMethod = DelegateInvokeMethod(delegateType);
var returnType = DelegateReturnTypeWithAnnotations(invokeMethod, out RefKind refKind);

LambdaSymbol lambdaSymbol = CreateLambdaSymbol(
delegateType,
Binder.ContainingMemberOrLambda,
out MethodSymbol invokeMethod,
out TypeWithAnnotations returnType,
out DiagnosticBag diagnostics,
out ReturnInferenceCacheKey cacheKey);
LambdaSymbol lambdaSymbol;
Binder lambdaBodyBinder;
BoundBlock block;

var diagnostics = DiagnosticBag.GetInstance();
var compilation = Binder.Compilation;
var cacheKey = ReturnInferenceCacheKey.Create(delegateType, IsAsync);

CSharpCompilation compilation = Binder.Compilation;
Binder lambdaBodyBinder = new ExecutableCodeBinder(_unboundLambda.Syntax, lambdaSymbol, ParameterBinder(lambdaSymbol, Binder));
// When binding for real (not for return inference), there is still a good chance
// we could reuse a body of a lambda previous bound for return type inference.
// For simplicity, reuse is limited to expression-bodied lambdas. In those cases,
// we reuse the bound expression and apply any conversion to the return value
// since the inferred return type was not used when binding for return inference.
if (refKind == CodeAnalysis.RefKind.None &&
_returnInferenceCache.TryGetValue(cacheKey, out BoundLambda returnInferenceLambda) &&
GetLambdaExpressionBody(returnInferenceLambda.Body) is BoundExpression expression &&
(lambdaSymbol = returnInferenceLambda.Symbol).RefKind == refKind &&
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

(lambdaSymbol = returnInferenceLambda.Symbol).RefKind == refKind [](start = 16, length = 64)

Should we also check that both are None as GetReusableLambdaExpressionBody checks below? #Closed

(object)LambdaSymbol.InferenceFailureReturnType != lambdaSymbol.ReturnType &&
lambdaSymbol.ReturnTypeWithAnnotations.Equals(returnType, TypeCompareKind.ConsiderEverything))
{
lambdaBodyBinder = returnInferenceLambda.Binder;
block = CreateBlockFromLambdaExpressionBody(lambdaBodyBinder, expression, diagnostics);
diagnostics.AddRange(returnInferenceLambda.Diagnostics);
}
else
{
lambdaSymbol = CreateLambdaSymbol(Binder.ContainingMemberOrLambda, returnType, diagnostics, cacheKey.ParameterTypes, cacheKey.ParameterRefKinds, refKind);
lambdaBodyBinder = new ExecutableCodeBinder(_unboundLambda.Syntax, lambdaSymbol, ParameterBinder(lambdaSymbol, Binder));
block = BindLambdaBody(lambdaSymbol, lambdaBodyBinder, diagnostics);
}

if (lambdaSymbol.RefKind == CodeAnalysis.RefKind.RefReadOnly)
{
Expand Down Expand Up @@ -583,8 +615,6 @@ private BoundLambda ReallyBind(NamedTypeSymbol delegateType)
ParameterHelpers.EnsureNullableAttributeExists(compilation, lambdaSymbol, lambdaParameters, diagnostics, modifyCompilation: false);
// Note: we don't need to warn on annotations used in #nullable disable context for lambdas, as this is handled in binding already

BoundBlock block = BindLambdaBody(lambdaSymbol, lambdaBodyBinder, diagnostics);

((ExecutableCodeBinder)lambdaBodyBinder).ValidateIteratorMethods(diagnostics);
ValidateUnsafeParameters(diagnostics, cacheKey.ParameterTypes);

Expand Down Expand Up @@ -626,16 +656,6 @@ private BoundLambda ReallyBind(NamedTypeSymbol delegateType)
return result;
}

private LambdaSymbol CreateLambdaSymbol(NamedTypeSymbol delegateType, Symbol containingSymbol, out MethodSymbol invokeMethod, out TypeWithAnnotations returnType, out DiagnosticBag diagnostics, out ReturnInferenceCacheKey cacheKey)
{
invokeMethod = DelegateInvokeMethod(delegateType);
returnType = DelegateReturnTypeWithAnnotations(invokeMethod, out RefKind refKind);
diagnostics = DiagnosticBag.GetInstance();
cacheKey = ReturnInferenceCacheKey.Create(delegateType, IsAsync);

return CreateLambdaSymbol(containingSymbol, returnType, diagnostics, cacheKey.ParameterTypes, cacheKey.ParameterRefKinds, refKind);
}

internal LambdaSymbol CreateLambdaSymbol(
Symbol containingSymbol,
TypeWithAnnotations returnType,
Expand All @@ -655,7 +675,10 @@ internal LambdaSymbol CreateLambdaSymbol(

internal LambdaSymbol CreateLambdaSymbol(NamedTypeSymbol delegateType, Symbol containingSymbol)
{
return CreateLambdaSymbol(delegateType, containingSymbol, out _, out _, out _, out _);
var invokeMethod = DelegateInvokeMethod(delegateType);
var returnType = DelegateReturnTypeWithAnnotations(invokeMethod, out RefKind refKind);
var cacheKey = ReturnInferenceCacheKey.Create(delegateType, IsAsync);
return CreateLambdaSymbol(containingSymbol, returnType, new DiagnosticBag(), cacheKey.ParameterTypes, cacheKey.ParameterRefKinds, refKind);
}

private void ValidateUnsafeParameters(DiagnosticBag diagnostics, ImmutableArray<TypeWithAnnotations> targetParameterTypes)
Expand Down Expand Up @@ -741,6 +764,7 @@ private BoundLambda ReallyInferReturnType(
var block = BindLambdaBody(lambdaSymbol, lambdaBodyBinder, diagnostics);
return (lambdaSymbol, block, lambdaBodyBinder, diagnostics);
}

public BoundLambda BindForReturnTypeInference(NamedTypeSymbol delegateType)
{
var cacheKey = ReturnInferenceCacheKey.Create(delegateType, IsAsync);
Expand Down Expand Up @@ -1216,6 +1240,26 @@ protected override UnboundLambdaState WithCachingCore(bool includeCache)
return new PlainUnboundLambdaState(unboundLambda: null, Binder, _parameterNames, _parameterIsDiscardOpt, _parameterTypesWithAnnotations, _parameterRefKinds, _isAsync, includeCache);
}

protected override BoundExpression GetLambdaExpressionBody(BoundBlock body)
{
if (IsExpressionLambda)
{
var statements = body.Statements;
if (statements.Length == 1 &&
// To simplify Binder.CreateBlockFromExpression (used below), we only reuse by-value return values.
statements[0] is BoundReturnStatement { RefKind: Microsoft.CodeAnalysis.RefKind.None, ExpressionOpt: BoundExpression expr })
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

RefKind: Microsoft.CodeAnalysis.RefKind.None [](start = 60, length = 44)

It is not obvious why this condition is significant, consider adding a clarifying comment. #Closed

{
return expr;
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

return expr; [](start = 20, length = 12)

It is not clear why it is reusable? The name doesn't imply any restrictions on the input. I can imagine a situation when some conversions are have already been applied based on some context, therefore making the expression un-reusable. If the method is not responsible for enforcing reusability, then it should not be part of the name. This is just a helper that extracts return expression from a block with a single ref-none return. #Closed

}
}
return null;
}

protected override BoundBlock CreateBlockFromLambdaExpressionBody(Binder lambdaBodyBinder, BoundExpression expression, DiagnosticBag diagnostics)
{
return lambdaBodyBinder.CreateBlockFromExpression((ExpressionSyntax)this.Body, expression, diagnostics);
}

protected override BoundBlock BindLambdaBody(LambdaSymbol lambdaSymbol, Binder lambdaBodyBinder, DiagnosticBag diagnostics)
{
if (this.IsExpressionLambda)
Expand Down
41 changes: 40 additions & 1 deletion src/Compilers/CSharp/Test/Semantic/Semantics/OverloadResolutionPerfTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ static class Ext

[ConditionalFactAttribute(typeof(IsRelease))]
[WorkItem(40495, "https://github.com/dotnet/roslyn/issues/40495")]
public void NestedLambdas()
public void NestedLambdas_01()
{
var source =
@"#nullable enable
Expand All @@ -302,6 +302,45 @@ static void Main()
Enumerable.Range(0, 1).Sum(f =>
Enumerable.Range(0, 1).Count(g => true)))))));
}
}";
var comp = CreateCompilation(source);
comp.VerifyDiagnostics();
}

// Test should complete in several seconds if UnboundLambda.ReallyBind
// uses results from _returnInferenceCache.
[ConditionalFactAttribute(typeof(IsRelease))]
[WorkItem(1083969, "https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1083969")]
public void NestedLambdas_02()
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

NestedLambdas_02 [](start = 20, length = 16)

What exactly are we testing here? Were we unable to compile the code before? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

We're testing performance. On my machine, this test completes in a couple of seconds with this change and 15+ mins without.

In reply to: 410978595 [](ancestors = 410978595)

Copy link
Contributor

Choose a reason for hiding this comment

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

We're testing performance. On my machine, this test completes in a couple of seconds with this change and 15+ mins without.

Consider adding a comment to make it easy to analyze failures.

In reply to: 411004957 [](ancestors = 411004957,410978595)

{
var source =
@"using System.Collections.Generic;
using System.Linq;
class Program
{
static void F(IEnumerable<int[]> x)
{
x.GroupBy(y => y[1]).SelectMany(x =>
x.GroupBy(y => y[2]).SelectMany(x =>
x.GroupBy(y => y[3]).SelectMany(x =>
x.GroupBy(y => y[4]).SelectMany(x =>
x.GroupBy(y => y[5]).SelectMany(x =>
x.GroupBy(y => y[6]).SelectMany(x =>
x.GroupBy(y => y[7]).SelectMany(x =>
x.GroupBy(y => y[8]).SelectMany(x =>
x.GroupBy(y => y[9]).SelectMany(x =>
x.GroupBy(y => y[0]).SelectMany(x =>
x.GroupBy(y => y[1]).SelectMany(x =>
x.GroupBy(y => y[2]).SelectMany(x =>
x.GroupBy(y => y[3]).SelectMany(x =>
x.GroupBy(y => y[4]).SelectMany(x =>
x.GroupBy(y => y[5]).SelectMany(x =>
x.GroupBy(y => y[6]).SelectMany(x =>
x.GroupBy(y => y[7]).SelectMany(x =>
x.GroupBy(y => y[8]).SelectMany(x =>
x.GroupBy(y => y[9]).SelectMany(x =>
x.GroupBy(y => y[0]).Select(x => x.Average(z => z[0])))))))))))))))))))));
}
}";
var comp = CreateCompilation(source);
comp.VerifyDiagnostics();
Expand Down