Skip to content

Avoid in-process captures in UpdateReferencesAync#36161

Merged
sharwell merged 1 commit intodotnet:masterfrom
sharwell:updaterefs-no-capture
Jun 8, 2019
Merged

Avoid in-process captures in UpdateReferencesAync#36161
sharwell merged 1 commit intodotnet:masterfrom
sharwell:updaterefs-no-capture

Conversation

@sharwell
Copy link
Copy Markdown
Contributor

@sharwell sharwell commented Jun 4, 2019

This change produced a 220MB (6.7%) allocation reduction in the scenario described by #36158.

@sharwell sharwell requested a review from a team as a code owner June 4, 2019 20:41
This change produced a 220MB (6.7%) allocation reduction in the scenario
described by dotnet#36158.
@sharwell sharwell force-pushed the updaterefs-no-capture branch from a586bb4 to 2ec12e3 Compare June 4, 2019 20:41
}

private Task GetTask(Project project, Func<Task> func, CancellationToken cancellationToken)
[PerformanceSensitive("https://github.com/dotnet/roslyn/issues/36158", AllowCaptures = false, Constraint = "Avoid captures to reduce GC pressure when running in the host workspace.")]
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.

Shouldn't the attribute be applied to UpdateSymbolTreeInfoAsync above?

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.

➡️ No. UpdateSymbolTreeInfoAsync was not a significant source of allocations in the test scenario. The changes to the method are a side effect of the signature change, as opposed to a change directly intended to reduce allocations.

? GetNewTask(this, func, project, reference, cancellationToken)
: func(this, project, reference, cancellationToken);

static Task GetNewTask(IncrementalAnalyzer self, Func<IncrementalAnalyzer, Project, PortableExecutableReference, CancellationToken, Task> func, Project project, PortableExecutableReference reference, CancellationToken cancellationToken)
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.

How does this help reduce allocation?

Copy link
Copy Markdown
Contributor Author

@sharwell sharwell Jun 5, 2019

Choose a reason for hiding this comment

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

➡️ If a method contains a lambda that captures parameters, a closure instance is created for those parameters at the start of the method whether or not the lambda is created or used. By moving the creation of the lambda to a local function, the creation of the closure instance is deferred until such time as the lambda actually needs to be created. This leaves the path above that doesn't use the lambda as a non-allocating code path.

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.

Copy link
Copy Markdown
Contributor

@Therzok Therzok left a comment

Choose a reason for hiding this comment

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

LGTM!

@sharwell can you please CC me on future PRs?

@sharwell sharwell merged commit 611578d into dotnet:master Jun 8, 2019
@sharwell sharwell deleted the updaterefs-no-capture branch June 8, 2019 01:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants