Reuse previous lambda body binding if possible #43366

cston · 2020-04-15T06:26:38Z

Allow re-use of expression-bodied lambda bodies bound during return type inference. (Re-use was disabled in #37052.)

AlekseyTs · 2020-04-19T19:06:45Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

@@ -3195,6 +3193,15 @@ public BoundBlock BindLambdaExpressionAsBlock(ExpressionSyntax body, DiagnosticB
            return bodyBinder.CreateBlockFromExpression(body, bodyBinder.GetDeclaredLocalsForScope(body), refKind, expression, expressionSyntax, diagnostics);
        }

+        public BoundBlock BindLambdaExpressionAsBlockContinued(ExpressionSyntax body, BoundExpression expression, DiagnosticBag diagnostics)


BindLambdaExpressionAsBlockContinued [](start = 26, length = 36)

"Continued" doesn't really convey anything specific. Continued from where? Etc. Consider renaming with a more specific name, it feels like simply using CreateBlockFromExpression would work. #Closed

AlekseyTs · 2020-04-19T19:11:13Z

src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs

                    types.Add((returnStatement.ExpressionOpt, type));
                }
            }

            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());
+            types.Free();


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

Were we leaking this builder before? #Closed

Yes.

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

AlekseyTs · 2020-04-19T19:22:34Z

src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs

@@ -927,7 +933,7 @@ BoundLambda rebind(BoundLambda lambda)
                    returnType = DelegateReturnTypeWithAnnotations(invokeMethod, out refKind);
                    if (!returnType.HasType || returnType.Type.ContainsTypeParameter())
                    {
-                        var t = (inferredReturnType.HadExpressionlessReturn || inferredReturnType.NumExpressions == 0)
+                        var t = !inferredReturnType.HasReturnValue


!inferredReturnType.HasReturnValue [](start = 32, length = 34)

It feels like this refactoring is not semantically equivalent. It looks like before we could have HadExpressionlessReturn == true and inferredReturnType.NumExpressions > 0 at the same time. The orignal condition would be true. The new condition for the same scenario would be false. #Closed

AlekseyTs · 2020-04-19T19:31:55Z

src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs

+                if (statements.Length == 1 &&
+                    statements[0] is BoundReturnStatement { RefKind: Microsoft.CodeAnalysis.RefKind.None, ExpressionOpt: BoundExpression expr })
+                {
+                    return expr;


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

AlekseyTs · 2020-04-19T19:34:36Z

src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs

+            return null;
+        }
+
+        protected override BoundBlock CreateBlockFromExpression(LambdaSymbol lambdaSymbol, Binder lambdaBodyBinder, BoundExpression expression, DiagnosticBag diagnostics)


LambdaSymbol lambdaSymbol [](start = 64, length = 25)

It doesn't look like this parameter aver used, please remove. #Closed

AlekseyTs · 2020-04-19T19:49:52Z

src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs

+            var cacheKey = ReturnInferenceCacheKey.Create(delegateType, IsAsync);
+
+            // When binding for real (not for return inference), there is still
+            // a good chance that we could reuse a body of a lambda previous bound for


a good chance that we could reuse a body of a lambda previous bound for [](start = 15, length = 71)

It doesn't look like we are reusing the body, we are reusing some portion of it. Consider reflecting this in the comment and also explaining why it is Ok to reuse that portion. #Closed

AlekseyTs · 2020-04-19T19:50:56Z

src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs

+            {
+                var statements = body.Statements;
+                if (statements.Length == 1 &&
+                    statements[0] is BoundReturnStatement { RefKind: Microsoft.CodeAnalysis.RefKind.None, ExpressionOpt: BoundExpression expr })


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

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

AlekseyTs · 2020-04-19T19:52:44Z

src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs

+            // return type inference.
+            if (_returnInferenceCache.TryGetValue(cacheKey, out BoundLambda returnInferenceLambda) &&
+                GetReusableLambdaExpressionBody(returnInferenceLambda.Body) is BoundExpression expression &&
+                (lambdaSymbol = returnInferenceLambda.Symbol).RefKind == refKind &&


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

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

AlekseyTs · 2020-04-19T19:54:29Z

src/Compilers/CSharp/Test/Semantic/Semantics/OverloadResolutionPerfTests.cs

+
+        [ConditionalFactAttribute(typeof(IsRelease))]
+        [WorkItem(1083969, "https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1083969")]
+        public void NestedLambdas_02()


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

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

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)

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)

AlekseyTs · 2020-04-19T19:55:52Z

Done with review pass (iteration 1) #Closed

AlekseyTs · 2020-04-19T20:29:12Z

src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs

            RefKind refKind,
            TypeWithAnnotations typeWithAnnotations,
            ImmutableArray<DiagnosticInfo> useSiteDiagnostics)
        {
-            NumExpressions = numExpressions;
-            HadExpressionlessReturn = hadExpressionlessReturn;
+            HasReturnValue = hasReturnValue;


HasReturnValue = hasReturnValue; [](start = 12, length = 32)

It looks like this refactoring is not related to primary goal of this PR, please exclude it from the PR. #Closed

AlekseyTs · 2020-04-19T23:23:30Z

src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs

@@ -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.


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

Typo?

AlekseyTs

LGTM (iteration 2)

gafter

cston requested a review from a team as a code owner April 15, 2020 06:26

cston marked this pull request as draft April 15, 2020 06:27

Reuse previous lambda body binding if possible

b43120a

cston force-pushed the lambda-reuse branch from 0f1eb52 to b43120a Compare April 16, 2020 17:00

cston marked this pull request as ready for review April 16, 2020 17:04

cston added the Area-Compilers label Apr 17, 2020

AlekseyTs reviewed Apr 19, 2020

View reviewed changes

PR feedback

ad529d9

AlekseyTs reviewed Apr 19, 2020

View reviewed changes

AlekseyTs approved these changes Apr 19, 2020

View reviewed changes

gafter approved these changes Apr 20, 2020

View reviewed changes

cston merged commit 278435e into dotnet:master Apr 20, 2020

ghost added this to the Next milestone Apr 20, 2020

cston deleted the lambda-reuse branch April 20, 2020 04:11

cston modified the milestones: Next, 16.7.P1 Apr 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reuse previous lambda body binding if possible #43366

Reuse previous lambda body binding if possible #43366

cston commented Apr 15, 2020 •

edited

Loading

AlekseyTs Apr 19, 2020 •

edited

Loading

AlekseyTs Apr 19, 2020 •

edited

Loading

cston Apr 19, 2020

AlekseyTs Apr 19, 2020 •

edited

Loading

AlekseyTs Apr 19, 2020 •

edited

Loading

AlekseyTs Apr 19, 2020 •

edited

Loading

AlekseyTs Apr 19, 2020 •

edited

Loading

AlekseyTs Apr 19, 2020 •

edited

Loading

AlekseyTs Apr 19, 2020 •

edited

Loading

AlekseyTs Apr 19, 2020 •

edited

Loading

cston Apr 19, 2020

AlekseyTs Apr 19, 2020

AlekseyTs commented Apr 19, 2020 •

edited

Loading

AlekseyTs Apr 19, 2020 •

edited

Loading

AlekseyTs Apr 19, 2020

AlekseyTs left a comment

gafter left a comment

Reuse previous lambda body binding if possible #43366

Reuse previous lambda body binding if possible #43366

Conversation

cston commented Apr 15, 2020 • edited Loading

AlekseyTs Apr 19, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 19, 2020 • edited Loading

Choose a reason for hiding this comment

cston Apr 19, 2020

Choose a reason for hiding this comment

AlekseyTs Apr 19, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 19, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 19, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 19, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 19, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 19, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 19, 2020 • edited Loading

Choose a reason for hiding this comment

cston Apr 19, 2020

Choose a reason for hiding this comment

AlekseyTs Apr 19, 2020

Choose a reason for hiding this comment

AlekseyTs commented Apr 19, 2020 • edited Loading

AlekseyTs Apr 19, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 19, 2020

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

gafter left a comment

Choose a reason for hiding this comment

cston commented Apr 15, 2020 •

edited

Loading

AlekseyTs Apr 19, 2020 •

edited

Loading

AlekseyTs Apr 19, 2020 •

edited

Loading

AlekseyTs Apr 19, 2020 •

edited

Loading

AlekseyTs Apr 19, 2020 •

edited

Loading

AlekseyTs Apr 19, 2020 •

edited

Loading

AlekseyTs Apr 19, 2020 •

edited

Loading

AlekseyTs Apr 19, 2020 •

edited

Loading

AlekseyTs Apr 19, 2020 •

edited

Loading

AlekseyTs Apr 19, 2020 •

edited

Loading

AlekseyTs commented Apr 19, 2020 •

edited

Loading

AlekseyTs Apr 19, 2020 •

edited

Loading