Bind fully-typed lambda only once for error recovery #69695
Conversation
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
Inspired by dotnet#59733.
| comp.TestOnlyCompilationData = data; | ||
| comp.VerifyDiagnostics(); | ||
| Assert.Equal(localFunctions ? 20 : 220, data.LambdaBindingCount); | ||
| }, timeout: TimeSpan.FromSeconds(20)); |
There was a problem hiding this comment.
This runs ~45 seconds (before the fix) vs ~5 seconds (after the fix) on my machine for lambdas and ~5 seconds both before and after for local functions.
Also without the fix, the number of lambda bindings is 1930 instead of 220.
| unboundArgument.FunctionType is { } functionType && | ||
| functionType.GetInternalDelegateType() is { } delegateType) | ||
| { | ||
| _ = unboundArgument.Bind(delegateType, isExpressionTree: false); |
There was a problem hiding this comment.
nit: Consider adding the same comment as below here ("// Just assume we're not in an expression tree for the purposes of error recovery.")
|
@cston PTAL |
| for (int j = 0; j < statementsNumber; j++) | ||
| { | ||
| builder3.AppendLine($"""arg = arg + "{j}";"""); | ||
| } |
There was a problem hiding this comment.
What is the effect of lambdasNumber or statementsNumber? Are these just constant multiples for the total execution time with or without the fix? If so, would it make sense to simply use 1 lambda and 1 statement?
There was a problem hiding this comment.
You're right, that was necessary when the test was based on timeouts, but now it's not since I reused your lambda binding counter.
There was a problem hiding this comment.
With this change of the test (time doesn't change visibly anymore):
| lambda bindings | time | |
|---|---|---|
| local functions | 20 | ~1s |
| lambdas | 211 → 40 | ~0.1s |
| """; | ||
| RunInThread(() => | ||
| { | ||
| var comp = CreateCompilation(source, options: TestOptions.DebugDll.WithConcurrentBuild(false)); |
There was a problem hiding this comment.
We could, but I'm worried that then the number of lambda bindings would be non-deterministic as two threads could race to bind the same lambda so the counter would be incremented twice in such rare case, making the test flaky.
ghost
added this to the 
Fixes #69093 (as an alternative to #69505), although not exactly as suggested there - skipping lambda body binding in
HasAnonymousFunctionConversionhas no effect since the lambda is bound later for error recovery or inCreateAnonymousFunctionConversionanyway (and the result is cached) - see #69505 (comment). We could perhaps skip some unnecessary binding inCreateAnonymousFunctionConversion, but not sure if that could help since in my testing scenarios, the change in this PR already makes lambdas as fast as local functions.