Avoid lock when setting project entry point

[Erarndt]

Context

There's a good amount of contention inside of BuildManager.PendBuildRequest() coming from the lock in the body:

    private BuildSubmissionBase<TRequestData, TResultData> PendBuildRequest<TRequestData, TResultData>(
        TRequestData requestData)
        where TRequestData : BuildRequestData<TRequestData, TResultData>
        where TResultData : BuildResultBase
    {
        lock (_syncLock)
        {
            ErrorUtilities.VerifyThrowArgumentNull(requestData);
            ErrorIfState(BuildManagerState.WaitingForBuildToComplete, "WaitingForEndOfBuild");
            ErrorIfState(BuildManagerState.Idle, "NoBuildInProgress");
            VerifyStateInternal(BuildManagerState.Building);

            var newSubmission = requestData.CreateSubmission(this, GetNextSubmissionId(), requestData,
                _buildParameters!.LegacyThreadingSemantics);

            if (_buildTelemetry != null)
            {
                // Project graph can have multiple entry points, for purposes of identifying event for same build project,
                // we believe that including only one entry point will provide enough precision.
                _buildTelemetry.ProjectPath ??= requestData.EntryProjectsFullPath.FirstOrDefault();
                _buildTelemetry.BuildTarget ??= string.Join(",", requestData.TargetNames);
            }

            _buildSubmissions.Add(newSubmission.SubmissionId, newSubmission);
            _noActiveSubmissionsEvent!.Reset();
            return newSubmission;
        }
    }

There's a small amount of CPU time caused by hitting the lock too.

Looks like this can be switched over to use some low-lock patterns to mitigate this. The results after the changes show the contention is essentially eliminated

Changes Made

Testing

Notes

[Copilot]

Pull Request Overview

This PR refactors the build submission flow to reduce lock contention and implements an atomic setter for telemetry entry points.

• Migrate _buildSubmissions from Dictionary to ConcurrentDictionary and update all related operations (add, remove, count, enumeration).
• Remove the lock in PendBuildRequest by using atomic operations (Interlocked, Thread.MemoryBarrier) and TryAdd.
• Extract ProjectPath/BuildTarget assignment into BuildTelemetry.SetProjectEntryPoint with Interlocked.CompareExchange and update tests accordingly.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/Framework/Telemetry/BuildTelemetry.cs	Added _projectEntryPointSet flag, made ProjectPath/BuildTarget setters private, and introduced SetProjectEntryPoint.
src/Build/BackEnd/BuildManager/BuildManager.cs	Replaced submission tracking with ConcurrentDictionary, removed the lock in PendBuildRequest, updated loops and ID generation.
src/Build.UnitTests/BackEnd/KnownTelemetry_Tests.cs	Updated telemetry unit test to call SetProjectEntryPoint instead of setting properties directly.

Comments suppressed due to low confidence (2)

src/Framework/Telemetry/BuildTelemetry.cs:234

• The comment refers to an entryPointSet field but the actual backing field is named _projectEntryPointSet. Updating the comment to match the field name will improve clarity.

// this check is to ensure that we only set the entry point once. The entryPointSet field is set to 0

src/Framework/Telemetry/BuildTelemetry.cs:232

• Introduce unit tests to verify that calling SetProjectEntryPoint multiple times does not override the initially set ProjectPath and BuildTarget, ensuring idempotent behavior is covered.

public void SetProjectEntryPoint(IEnumerable<string> entryProjectsFullPath, IEnumerable<string> targetNames)

[Copilot]

Using Interlocked.Increment changes the first submission ID from 0 to 1, altering existing behavior. Consider initializing _nextBuildSubmissionId to -1 or adjusting the logic to preserve the original ID sequence starting at 0.

[rainersigwald]

Suggested change

	_buildSubmissions.TryAdd(newSubmission.SubmissionId, newSubmission);
	bool added = _buildSubmissions.TryAdd(newSubmission.SubmissionId, newSubmission);
	Debug.Assert(added == true, "We expect adding a new submission to always succeed since it should have received a unique ID");

?

[rainersigwald]

I'm not sure I understand this change--if we want a local iterable don't we want ConcurrentDictionary<>.Values?

Also is there value in building the list then iterating over it, versus combining the foreaches?

[Erarndt]

The motivation for creating the list is that CheckSubmissionCompletenessAndRemove() mutates _buildSubmissions so we need a copy.

    private void CheckSubmissionCompletenessAndRemove(BuildSubmissionBase submission)
    {
        lock (_syncLock)
        {
            // If the submission has completed or never started, remove it.
            if (submission.IsCompleted || !submission.IsStarted)
            {
                _overallBuildSuccess &= (submission.BuildResultBase?.OverallResult == BuildResultCode.Success);
                _buildSubmissions.TryRemove(submission.SubmissionId, out _);

                // Clear all cached SDKs for the submission
                SdkResolverService.ClearCache(submission.SubmissionId);
            }

            CheckAllSubmissionsComplete(submission.BuildRequestDataBase.Flags);
        }
    }

The switch for enumerating the KVP vs just the Values collection is because of the difference between how ConcurrentDictionary and Dictionary expose the Values property. Using Dictionary is nice since the concrete type exposes struct-based enumerators for Keys, Values, and the KVP. In contrast, ConcurrentDictionary creates a list of all the values and returns that:

public ICollection<TValue> Values
{
    [__DynamicallyInvokable]
    get
    {
        return GetValues();
    }
}

private ReadOnlyCollection<TValue> GetValues()
{
    int locksAcquired = 0;
    try
    {
        AcquireAllLocks(ref locksAcquired);
        int countInternal = GetCountInternal();
        if (countInternal < 0)
        {
            throw new OutOfMemoryException();
        }

        List<TValue> list = new List<TValue>(countInternal);
        for (int i = 0; i < m_tables.m_buckets.Length; i++)
        {
            for (Node node = m_tables.m_buckets[i]; node != null; node = node.m_next)
            {
                list.Add(node.m_value);
            }
        }

        return new ReadOnlyCollection<TValue>(list);
    }
    finally
    {
        ReleaseLocks(0, locksAcquired);
    }
}

Just enumerating the ConcurrentDictionary enumerates the KVPs via the yield return pattern

public IEnumerator<KeyValuePair<TKey, TValue>> GetEnumerator()
{
    Node[] buckets = m_tables.m_buckets;
    for (int i = 0; i < buckets.Length; i++)
    {
        for (Node current = Volatile.Read(ref buckets[i]); current != null; current = current.m_next)
        {
            yield return new KeyValuePair<TKey, TValue>(current.m_key, current.m_value);
        }
    }
}

We still get an allocation from the generated state machine for the enumerator, but it will be better than cloning all the values unless we consistently put almost nothing in the dictionary.

[rainersigwald]

Sorry, still not seeing it. Here you walk all the KVPs and add their value to a list, then walk the list doing work. If you called .Values, wouldn't that GetValues() do almost exactly that same list construction?

[Erarndt]

Ah, I think it just clicked. You're proposing using the list returned from Values directly since it's a copy rather than manually making our own copy? If so, I agree with what you're suggesting. I can update this to just operate on the Values collection and add a comment explaining why it's safe to do so even with CheckSubmissionCompletenessAndRemove() mutating the original dictionary.

[rainersigwald]

Yeah, that's what I was thinking for this one.