Skip to content

Conversation

ccastanedaucf
Copy link
Contributor

@ccastanedaucf ccastanedaucf commented May 30, 2025

Changes

  • Unroll looped uses of CommunicationsUtils.ReadAsync to avoid redundant state machine allocations.
  • Add missing ConfigureAwait(false) to awaited reads (implicit with Task.Result) to reduce allocations due to context capturing.
  • Avoid creating a Task when ReadAsync() completes synchronously and returns a ValueTask.

Context

Currently the pipe read loops on .NET Core go through a helper in CommunicationsUtilities that looks like this:

internal static async ValueTask<int> ReadAsync(Stream stream, byte[] buffer, int bytesToRead)
{
	int totalBytesRead = 0;
	while (totalBytesRead < bytesToRead)
	{
		int bytesRead = await stream.ReadAsync(buffer.AsMemory(totalBytesRead, bytesToRead - totalBytesRead), CancellationToken.None);
		if (bytesRead == 0)
		{
			return totalBytesRead;
		}
		totalBytesRead += bytesRead;
	}
	return totalBytesRead;
}

Unfortunately when used within an outer loop, this leads to a ton of extra allocations due to having to create a new AsyncStateMachineBox for every iteration.

Here's an example:

System.Runtime.CompilerServices.AsyncTaskMethodBuilder+AsyncStateMachineBox<Int32, CommunicationsUtilities+<ReadAsync>d__17>
  Objects : 552567
  Bytes   : 75149112

 100%  GetStateMachineBox • 71.67 MB / 71.67 MB • System.Runtime.CompilerServices.AsyncTaskMethodBuilder<TResult>.GetStateMachineBox<TStateMachine>(TStateMachine, Task<TResult>)
  ►  96.8%  MoveNext • 69.40 MB / - • Microsoft.Build.Internal.CommunicationsUtilities+<ReadAsync>d__17.MoveNext()
  ►  3.16%  AwaitUnsafeOnCompleted • 2.27 MB / - • System.Runtime.CompilerServices.AsyncTaskMethodBuilder<TResult>.AwaitUnsafeOnCompleted<TAwaiter, TStateMachine>(TAwaiter, TStateMachine, Task<TResult>)

Using NodeProviderOutOfProcBase.ReadPacketLoopAsync() as an example - after this change, we only see the single state machine allocations for the outer loop (1 per pipe client) as it's reused, even when the async method schedules a continuation:

System.Runtime.CompilerServices.AsyncTaskMethodBuilder+AsyncStateMachineBox<VoidTaskResult, NodeProviderOutOfProcBase+NodeContext+<RunPacketReadLoopAsync>d__20>
  Objects : 15
  Bytes   : 1920

 100%  GetStateMachineBox • 1.9 KB / 1.9 KB • System.Runtime.CompilerServices.AsyncTaskMethodBuilder<TResult>.GetStateMachineBox<TStateMachine>(TStateMachine, Task<TResult>)
 ...

Profiles

Here's two profiles comparing total before / after on a .NET Core build (see objects allocated, total allocations, GC). This is accounting for ~8% of allocations on the main node.

Pasted image 20250530154531

Pasted image 20250530154433

@Copilot Copilot AI review requested due to automatic review settings May 30, 2025 22:51
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR aims to reduce asynchronous state machine allocations during pipe read operations by replacing looped calls to CommunicationsUtilities.ReadAsync with direct stream reading and proper use of ConfigureAwait(false).

  • Replaces redundant state machine calls with direct asynchronous stream reads using Memory overloads.
  • Adds missing ConfigureAwait(false) to awaited calls to reduce context capturing.
  • Unrolls packet read loops to avoid unnecessary Task allocations when reads complete synchronously.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/Shared/NodeEndpointOutOfProcBase.cs Refactored async reading to use stream.ReadAsync with memory for .NET Core.
src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs Unrolled the packet body read loop and switched to direct stream reading with ConfigureAwait(false).
src/Build/BackEnd/Client/MSBuildClientPacketPump.cs Updated pipe read operations to use stream.ReadAsync with memory and adjusted handle usage.

@SimaTian SimaTian merged commit 78ca182 into dotnet:main Jun 5, 2025
10 checks passed
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.

3 participants