Skip to content

Commit

Permalink
Remove unnecessary lock in TerminalLogger.UpdateNodeStatus (#10045)
Browse files Browse the repository at this point in the history
This is a minor perf improvement.

In particular when using the project cache with a high cache rate (thus spamming ProjectFinished events), this lock is taking ~6% of the CPU for the process due to contention. That's a lower bound though because I'm seeing other methods which do take the lock also suffer from high contention (eg `ThreadProc` also takes ~6% CPU), so removing the lock from `UpdateNodeStatus` is likely to reduce contention there as well.

This lock isn't necessary since this operation is a simple object replacement in an array. Other similar operations also don't take a lock, (See the write in `ProjectStarted` and the read in `MessageRaised`) which proves that it's not required.
  • Loading branch information
dfederm authored May 1, 2024
1 parent 4be99f8 commit 300b2a8
Showing 1 changed file with 6 additions and 5 deletions.
11 changes: 6 additions & 5 deletions src/MSBuild/TerminalLogger/TerminalLogger.cs
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,10 @@ public ProjectContext(BuildEventContext context)
/// <summary>
/// Tracks the work currently being done by build nodes. Null means the node is not doing any work worth reporting.
/// </summary>
/// <remarks>
/// There is no locking around access to this data structure despite it being accessed concurrently by multiple threads.
/// However, reads and writes to locations in an array is atomic, so locking is not required.
/// </remarks>
private NodeStatus?[] _nodes = Array.Empty<NodeStatus>();

/// <summary>
Expand Down Expand Up @@ -701,11 +705,8 @@ private void TargetStarted(object sender, TargetStartedEventArgs e)

private void UpdateNodeStatus(BuildEventContext buildEventContext, NodeStatus? nodeStatus)
{
lock (_lock)
{
int nodeIndex = NodeIndexForContext(buildEventContext);
_nodes[nodeIndex] = nodeStatus;
}
int nodeIndex = NodeIndexForContext(buildEventContext);
_nodes[nodeIndex] = nodeStatus;
}

/// <summary>
Expand Down

0 comments on commit 300b2a8

Please sign in to comment.