-
Notifications
You must be signed in to change notification settings - Fork 1.4k
RAR node: Buffer log events to client #12558
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
base: main
Are you sure you want to change the base?
RAR node: Buffer log events to client #12558
Conversation
There was a problem hiding this 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 implements buffered logging for the out-of-process RAR (ResolveAssemblyReference) node to improve performance by reducing IPC overhead. The implementation uses channels to batch log events asynchronously before sending them to the client.
- Adds a buffering mechanism using channels to batch log events before IPC transmission
- Refactors
LogMessagePacketBase
from abstract to concrete to enable reuse across different assemblies - Updates project references to consolidate logging packet handling
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/Tasks/Microsoft.Build.Tasks.csproj |
Adds new log event buffering class and moves Threading.Channels dependency to common section |
src/Tasks/AssemblyDependency/Node/RarNodeBuildEngine.cs |
Implements channel-based event buffering with async processing |
src/Tasks/AssemblyDependency/Node/RarNodeBufferedLogEvents.cs |
New packet type for batched log events |
src/Tasks/AssemblyDependency/Node/OutOfProcRarNodeEndpoint.cs |
Integrates async log processing into the RAR endpoint lifecycle |
src/Tasks/AssemblyDependency/Node/OutOfProcRarClient.cs |
Handles replay of buffered log events to the real build engine |
src/Tasks.UnitTests/AssemblyDependency/Node/RarNodeExecuteRequest_Tests.cs |
Updates tests to accommodate new constructor requirements |
src/Shared/LogMessagePacketBase.cs |
Refactors from abstract to concrete class with virtual methods for customization |
src/MSBuildTaskHost/MSBuildTaskHost.csproj |
Removes redundant LogMessagePacket reference |
src/MSBuild/OutOfProcTaskHostNode.cs |
Updates to use concrete LogMessagePacketBase |
src/MSBuild/MSBuild.csproj |
Removes LogMessagePacket.cs from project |
src/MSBuild/LogMessagePacket.cs |
File deleted as functionality moved to shared base |
src/Build/BackEnd/Components/Communications/LogMessagePacket.cs |
Moves specialized serialization logic from base class to concrete implementation |
// TODO: Buffer to a channel for the endpoint to consume. | ||
try | ||
{ | ||
while (cancellationToken.IsCancellationRequested) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loop condition is inverted. It should be while (!cancellationToken.IsCancellationRequested)
to continue processing until cancellation is requested.
while (cancellationToken.IsCancellationRequested) | |
while (!cancellationToken.IsCancellationRequested) |
Copilot uses AI. Check for mistakes.
rarTask.BuildEngine.LogMessageEvent((BuildMessageEventArgs)buildEvent); | ||
break; | ||
default: | ||
ErrorUtilities.ThrowInternalError($"Received unexpected log event type {logMessagePacket.Type}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message references logMessagePacket.Type
but should reference logMessagePacket.EventType
based on the switch statement above that uses logMessagePacket.EventType
.
ErrorUtilities.ThrowInternalError($"Received unexpected log event type {logMessagePacket.Type}"); | |
ErrorUtilities.ThrowInternalError($"Received unexpected log event type {logMessagePacket.EventType}"); |
Copilot uses AI. Check for mistakes.
Drafted as this depends on another PR #12526 - ignore the first two duplicated commits for now.
Implements logging for out-of-proc RAR node.
Context
Since RAR is often the largest source of events in MSBuild, some complexity is needed to avoid introducing
overhead from excessive allocations and task context-switching. The main goals here are:
the final response packet.
will immediately return a struct ValueTask.
As such, two channels are used: one for events, and one for the batch size.
When an event is queued, a pending count is incremented. When the count reaches the batch threshold, or the queue
is explicitly flushed, the count is written to the count channel and reset.
Once RAR completes, events are flushed with the remaining count.