Remove delegate and capture allocations#61719
Conversation
7d492e1 to
69b540d
Compare
src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs
Outdated
Show resolved
Hide resolved
| // EmptyStructTypeCache.IsEmptyStructType() returned true. If there are fields, | ||
| // at least one of those fields must be cyclic, so treat the type as empty. | ||
| if (members.Any(m => m.Kind == SymbolKind.Field)) | ||
| if (members.Any(static m => m.Kind == SymbolKind.Field)) |
There was a problem hiding this comment.
Not sure I like having static everywhere even it it's obvious the lambda is not lifting any variables.
There was a problem hiding this comment.
I left it out during my first pass, but added it during the second pass. It significantly improves auditability.
There was a problem hiding this comment.
I'd rather prefer readability. static is mostly noise, imo.
There was a problem hiding this comment.
fwiw i'm flexible. But i'm aligning more with sam in terms of having hte visibility here. It feels like this is a general area that does cause us pain (i've certainly had to fix tons of cases in the IDE where this cost hurt us), and it's why i did the work to add static-lambdas to the language :)
Perhaps we can start here, but then be comfortable allowing devs in their area to judiciously decide if they do/don't want to use this modifier?
There was a problem hiding this comment.
As discussed with Sam offline, I think the ideal solution is to have an analyzer that identifies call sites of methods that take lambdas but there is also an alternative that takes a TArg argument passed thru to the lambda as well. That's what we are actually trying to do here - to use the more memory efficient, argument passing version. It might get a bit tricky to identify these methods, but it's doable. If we had such an analyzer there wouldn't be need for using static.
I don't actually find static generally that useful. Yes, it makes it more obvious that you're not creating a closure but how do you know that it's possible to not create a closure in the first place? How does the dev writing code that calls Any (or any other method that takes a func) know there is a version of it that passes thru an argument that can avoid closure creation? Especially since 1) the methods are generally not in BCL 2) our extension methods that fill in the need are scattered across multiple namespaces and often don't even show up in completion.
Adding static to Any lambdas may be fine to address an imminent issue that we observe, but doesn't seem like a comprehensive solution to this problem. I think we should follow up with 1) moving the extension methods to the same namespace as the type being extended, so that they are available at all call sites where the original method is 2) check with runtime and LDM if the compiler can automatically optimize the lambdas if both overloads are available (and maybe marked with some attribute that declares the functional equivalence of the overloads) 3) write the analyzer.
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.PatternLocalRewriter.cs
Outdated
Show resolved
Hide resolved
69b540d to
b3abbda
Compare
src/Workspaces/Core/Portable/Shared/Extensions/IMethodSymbolExtensions.cs
Outdated
Show resolved
Hide resolved
| a.AttributeClass?.IsAccessibleWithin(accessibleWithin) == false; | ||
| static bool shouldRemoveAttribute(AttributeData a, (INamedTypeSymbol[] removeAttributeTypes, ISymbol accessibleWithin) arg) => | ||
| arg.removeAttributeTypes.Any(attr => attr.Equals(a.AttributeClass)) || | ||
| a.AttributeClass?.IsAccessibleWithin(arg.accessibleWithin) == false; |
There was a problem hiding this comment.
ngl. i hate this code :D
This pull request is the result of walking down a local performance trace running code analysis on Roslyn.sln, and looking for delegate and capture allocations that were easily eliminated. For some cases where a simple pattern emerged (such as the trivial elimination of allocations for
ImmutableArray<T>.Any), the correction was applied throughout production code as a preventative measure.