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.

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

Avoid captures for AnyAsync #51506

Merged
merged 1 commit into from
Feb 26, 2021
Merged

Avoid captures for AnyAsync #51506

merged 1 commit into from
Feb 26, 2021

Conversation

sharwell
Copy link
Member

These captures showed up as a few GB during a long-running high CPU operation measured locally.

int n = array.Length;
for (int i = 0; i < n; i++)
{
var a = array[i];
Copy link
Member

Choose a reason for hiding this comment

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

Why not basic for each? It's non allocating for immutable array right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure why everything in this file is written this way, but I prefer not to change it for just one case without cause, especially as part of a performance PR.

@@ -54,8 +54,8 @@ private static bool IsAccessible(ISymbol symbol)
var namespace2Count = namespace2.ConstituentNamespaces.Length;
if (namespace1Count != namespace2Count)
{
if ((namespace1Count > 1 && await namespace1.ConstituentNamespaces.AnyAsync(n => NamespaceSymbolsMatchAsync(solution, n, namespace2, cancellationToken)).ConfigureAwait(false)) ||
(namespace2Count > 1 && await namespace2.ConstituentNamespaces.AnyAsync(n2 => NamespaceSymbolsMatchAsync(solution, namespace1, n2, cancellationToken)).ConfigureAwait(false)))
if ((namespace1Count > 1 && await namespace1.ConstituentNamespaces.AnyAsync(static (n, arg) => NamespaceSymbolsMatchAsync(arg.solution, n, arg.namespace2, arg.cancellationToken), (solution, namespace2, cancellationToken)).ConfigureAwait(false)) ||
Copy link
Member

Choose a reason for hiding this comment

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

Can the other extension method be deleted in favor of the new one?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure. The other one is perfectly valid so I didn't look outside the ones that appeared in the trace.

Copy link
Member

Choose a reason for hiding this comment

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

I gave a look and it seems this PR replaced all usages of the old extension method.

https://grep.app/search?q=.AnyAsync&filter[repo][0]=dotnet/roslyn

It had only 3 usages which you replaced in the PR. I think it should be fine either way.

@sharwell sharwell merged commit e6aabad into dotnet:master Feb 26, 2021
@ghost ghost added this to the Next milestone Feb 26, 2021
@sharwell sharwell deleted the avoid-captures branch February 26, 2021 18:53
@allisonchou allisonchou modified the milestones: Next, 16.10.P2 Mar 29, 2021
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.

7 participants