Skip to content

Remove delegate and capture allocations#61719

Merged
sharwell merged 10 commits intodotnet:mainfrom
sharwell:rm-delegate-allocs
Jun 8, 2022
Merged

Remove delegate and capture allocations#61719
sharwell merged 10 commits intodotnet:mainfrom
sharwell:rm-delegate-allocs

Conversation

@sharwell
Copy link
Copy Markdown
Contributor

@sharwell sharwell commented Jun 6, 2022

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.

⚠️ Review commit-by-commit highly recommended, as the changes are broken out by concept.

@sharwell sharwell requested review from a team as code owners June 6, 2022 17:10
@sharwell sharwell requested a review from a team June 6, 2022 17:10
@sharwell sharwell requested a review from a team as a code owner June 6, 2022 17:10
@ghost ghost added the Area-Compilers label Jun 6, 2022
@sharwell sharwell marked this pull request as draft June 6, 2022 17:11
@sharwell sharwell force-pushed the rm-delegate-allocs branch from 7d492e1 to 69b540d Compare June 6, 2022 17:15
@sharwell sharwell marked this pull request as ready for review June 6, 2022 17:16
@sharwell sharwell marked this pull request as draft June 6, 2022 17:18
// 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))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure I like having static everywhere even it it's obvious the lambda is not lifting any variables.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I left it out during my first pass, but added it during the second pass. It significantly improves auditability.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd rather prefer readability. static is mostly noise, imo.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member

@tmat tmat Jun 7, 2022

Choose a reason for hiding this comment

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

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.

@sharwell sharwell force-pushed the rm-delegate-allocs branch from 69b540d to b3abbda Compare June 6, 2022 19:58
@sharwell sharwell marked this pull request as ready for review June 6, 2022 22:03
@sharwell sharwell removed the request for review from a team June 6, 2022 22:36
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;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ngl. i hate this code :D

@sharwell sharwell enabled auto-merge June 7, 2022 19:32
Copy link
Copy Markdown
Member

@tmat tmat left a comment

Choose a reason for hiding this comment

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

OK as a short temp memory improvement but would like to see follow up.

@sharwell sharwell merged commit edc3313 into dotnet:main Jun 8, 2022
@ghost ghost added this to the Next milestone Jun 8, 2022
@RikkiGibson RikkiGibson modified the milestones: Next, 17.3 P3 Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants