Skip to content

Several allocation fixes in Scheduler.cs #11802

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+54 −26
Open

Several allocation fixes in Scheduler.cs #11802

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 54 additions & 26 deletions src/Build/BackEnd/Components/Scheduler/Scheduler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -698,7 +698,7 @@ private void ScheduleUnassignedRequests(List<ScheduleResponse> responses)

// Resume any work available which has already been assigned to specific nodes.
ResumeRequiredWork(responses);
HashSet<int> idleNodes = new HashSet<int>();
HashSet<int> idleNodes = new HashSet<int>(_availableNodes.Count);
foreach (int availableNodeId in _availableNodes.Keys)
{
if (!_schedulingData.IsNodeWorking(availableNodeId))
Expand Down Expand Up @@ -991,7 +991,8 @@ private void AssignUnscheduledRequestsToInProcNode(List<ScheduleResponse> respon
{
if (idleNodes.Contains(InProcNodeId))
{
List<SchedulableRequest> unscheduledRequests = new List<SchedulableRequest>(_schedulingData.UnscheduledRequestsWhichCanBeScheduled);
List<SchedulableRequest> unscheduledRequests = new List<SchedulableRequest>(_schedulingData.UnscheduledRequestsCount);
Copy link
Preview

Copilot AI May 7, 2025

Choose a reason for hiding this comment

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

Using _schedulingData.UnscheduledRequestsCount as the list capacity assumes it matches the actual number of unscheduled requests. Please ensure that this value is always in sync with _schedulingData.UnscheduledRequestsWhichCanBeScheduled to avoid potential resizing during list population.

Suggested change
List<SchedulableRequest> unscheduledRequests = new List<SchedulableRequest>(_schedulingData.UnscheduledRequestsCount);
List<SchedulableRequest> unscheduledRequests = new List<SchedulableRequest>(_schedulingData.UnscheduledRequestsWhichCanBeScheduled.Count);

Copilot uses AI. Check for mistakes.

unscheduledRequests.AddRange(_schedulingData.UnscheduledRequestsWhichCanBeScheduled);
foreach (SchedulableRequest request in unscheduledRequests)
{
if (CanScheduleRequestToNode(request, InProcNodeId) && shouldBeScheduled(request))
Expand Down Expand Up @@ -1246,7 +1247,7 @@ private void AssignUnscheduledRequestsWithMaxWaitingRequests2(List<ScheduleRespo
private void AssignUnscheduledRequestsFIFO(List<ScheduleResponse> responses, HashSet<int> idleNodes)
{
// Assign requests on a first-come/first-serve basis
foreach (int nodeId in idleNodes)
foreach (SchedulableRequest unscheduledRequest in _schedulingData.UnscheduledRequestsWhichCanBeScheduled)
Copy link
Preview

Copilot AI May 7, 2025

Choose a reason for hiding this comment

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

The loop order in AssignUnscheduledRequestsFIFO has been reversed, which may affect the intended FIFO scheduling behavior. Please verify that iterating unscheduled requests first and then idle nodes produces the expected request assignment order.

Suggested change
foreach (SchedulableRequest unscheduledRequest in _schedulingData.UnscheduledRequestsWhichCanBeScheduled)
foreach (int nodeId in idleNodes)

Copilot uses AI. Check for mistakes.

Copy link
Member

Choose a reason for hiding this comment

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

What's the motivation for the switch? Document in a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enumerating over UnscheduledRequestsWhichCanBeScheduled is more expensive than enumerating over idleNodes (HashSet<int>). To enumerate UnscheduledRequestsWhichCanBeScheduled, we allocate a new enumerator each time (from the yield return) and the linked list enumeration is slightly slower too. I can add a comment to clarify if we want to keep this change.

{
// Don't overload the system.
if (AtSchedulingLimit())
Expand All @@ -1255,7 +1256,7 @@ private void AssignUnscheduledRequestsFIFO(List<ScheduleResponse> responses, Has
return;
}

foreach (SchedulableRequest unscheduledRequest in _schedulingData.UnscheduledRequestsWhichCanBeScheduled)
foreach (int nodeId in idleNodes)
{
if (CanScheduleRequestToNode(unscheduledRequest, nodeId))
{
Expand Down Expand Up @@ -1956,33 +1957,25 @@ private ScheduleResponse TrySatisfyRequestFromCache(int nodeForResults, BuildReq
/// <returns>True if caches misses are allowed, false otherwise</returns>
private bool CheckIfCacheMissOnReferencedProjectIsAllowedAndErrorIfNot(int nodeForResults, BuildRequest request, List<ScheduleResponse> responses, out Action<ILoggingService> emitNonErrorLogs)
{
emitNonErrorLogs = _ => { };
emitNonErrorLogs = static _ => { };

ProjectIsolationMode isolateProjects = _componentHost.BuildParameters.ProjectIsolationMode;
var configCache = (IConfigCache)_componentHost.GetComponent(BuildComponentType.ConfigCache);

// do not check root requests as nothing depends on them
if (isolateProjects == ProjectIsolationMode.False || request.IsRootRequest || request.SkipStaticGraphIsolationConstraints
|| SkipNonexistentTargetsIfExistentTargetsHaveResults(request))
|| SkipNonexistentTargetsIfExistentTargetsHaveResults(request, _configCache, _resultsCache))
{
bool logComment = ((isolateProjects == ProjectIsolationMode.True || isolateProjects == ProjectIsolationMode.MessageUponIsolationViolation) && request.SkipStaticGraphIsolationConstraints);
if (logComment)
{
// retrieving the configs is not quite free, so avoid computing them eagerly
var configs = GetConfigurations();

emitNonErrorLogs = ls => ls.LogComment(
NewBuildEventContext(),
MessageImportance.Normal,
"SkippedConstraintsOnRequest",
configs.ParentConfig.ProjectFullPath,
configs.RequestConfig.ProjectFullPath);
emitNonErrorLogs = GetLoggingServiceAction(configCache, request, _schedulingData);
}

return true;
}

(BuildRequestConfiguration requestConfig, BuildRequestConfiguration parentConfig) = GetConfigurations();
(BuildRequestConfiguration requestConfig, BuildRequestConfiguration parentConfig) = GetConfigurations(configCache, request, _schedulingData);

// allow self references (project calling the msbuild task on itself, potentially with different global properties)
if (parentConfig.ProjectFullPath.Equals(requestConfig.ProjectFullPath, StringComparison.OrdinalIgnoreCase))
Expand Down Expand Up @@ -2010,7 +2003,7 @@ private bool CheckIfCacheMissOnReferencedProjectIsAllowedAndErrorIfNot(int nodeF

return false;

BuildEventContext NewBuildEventContext()
static BuildEventContext NewBuildEventContext(BuildRequest request)
{
return new BuildEventContext(
request.SubmissionId,
Expand All @@ -2021,13 +2014,33 @@ BuildEventContext NewBuildEventContext()
BuildEventContext.InvalidTaskId);
}

(BuildRequestConfiguration RequestConfig, BuildRequestConfiguration ParentConfig) GetConfigurations()
static (BuildRequestConfiguration RequestConfig, BuildRequestConfiguration ParentConfig) GetConfigurations(IConfigCache configCache, BuildRequest request, SchedulingData schedulingData)
{
BuildRequestConfiguration buildRequestConfiguration = configCache[request.ConfigurationId];

// Need the parent request. It might be blocked or executing; check both.
SchedulableRequest parentRequest = _schedulingData.BlockedRequests.FirstOrDefault(r => r.BuildRequest.GlobalRequestId == request.ParentGlobalRequestId)
?? _schedulingData.ExecutingRequests.FirstOrDefault(r => r.BuildRequest.GlobalRequestId == request.ParentGlobalRequestId);
SchedulableRequest parentRequest = null;

foreach (var r in schedulingData.BlockedRequests)
{
if (r.BuildRequest.GlobalRequestId == request.ParentGlobalRequestId)
{
parentRequest = r;
break;
}
}

if (parentRequest is null)
{
foreach (var r in schedulingData.ExecutingRequests)
{
if (r.BuildRequest.GlobalRequestId == request.ParentGlobalRequestId)
{
parentRequest = r;
break;
}
}
}

ErrorUtilities.VerifyThrowInternalNull(parentRequest);
ErrorUtilities.VerifyThrow(
Expand All @@ -2038,20 +2051,20 @@ BuildEventContext NewBuildEventContext()
return (buildRequestConfiguration, parentConfiguration);
}

string ConcatenateGlobalProperties(BuildRequestConfiguration configuration)
static string ConcatenateGlobalProperties(BuildRequestConfiguration configuration)
{
return string.Join("; ", configuration.GlobalProperties.Select<ProjectPropertyInstance, string>(p => $"{p.Name}={p.EvaluatedValue}"));
return string.Join("; ", configuration.GlobalProperties.Select<ProjectPropertyInstance, string>(static p => $"{p.Name}={p.EvaluatedValue}"));
}

bool SkipNonexistentTargetsIfExistentTargetsHaveResults(BuildRequest buildRequest)
static bool SkipNonexistentTargetsIfExistentTargetsHaveResults(BuildRequest buildRequest, IConfigCache configCache, IResultsCache resultsCache)
{
// Return early if the top-level target(s) of this build request weren't requested to be skipped if nonexistent.
if ((buildRequest.BuildRequestDataFlags & BuildRequestDataFlags.SkipNonexistentTargets) != BuildRequestDataFlags.SkipNonexistentTargets)
{
return false;
}

BuildResult requestResults = _resultsCache.GetResultsForConfiguration(buildRequest.ConfigurationId);
BuildResult requestResults = resultsCache.GetResultsForConfiguration(buildRequest.ConfigurationId);

// On a self-referenced build, cache misses are allowed.
if (requestResults == null)
Expand All @@ -2061,9 +2074,9 @@ bool SkipNonexistentTargetsIfExistentTargetsHaveResults(BuildRequest buildReques

// A cache miss on at least one existing target without results is disallowed,
// as it violates isolation constraints.
foreach (string target in request.Targets)
foreach (string target in buildRequest.Targets)
{
if (_configCache[buildRequest.ConfigurationId]
if (configCache[buildRequest.ConfigurationId]
.ProjectTargets
.Contains(target) &&
!requestResults.HasResultsForTarget(target))
Expand All @@ -2076,6 +2089,21 @@ bool SkipNonexistentTargetsIfExistentTargetsHaveResults(BuildRequest buildReques
// to skip nonexistent targets.
return true;
}

static Action<ILoggingService> GetLoggingServiceAction(IConfigCache configCache, BuildRequest request, SchedulingData schedulingData)
{
// retrieving the configs is not quite free, so avoid computing them eagerly
Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate on this? Doesn't the next line realize the configs?

Copy link
Member

Choose a reason for hiding this comment

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

Oh this is copy/pasted from the original. Not sure I see the benefit of this extraction, does it cause some subtle allocation thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The goal here is to avoid the allocation of a closure object, and an unfortunate consequence is that if there's a capture anywhere in the body of the method, the closure object is created. This happens regardless of if the code that would use the closure is called. For example this:

    public static Action Test(int input)
    {
        Action action;

        if (input < int.MinValue)
        {
            action = () => Console.WriteLine(input);
        }
        else
        {
            action = () => { };
        }

        return action;
    }

Compiles to this:

public static Action Test(int input)
{
	<>c__DisplayClass1_0 <>c__DisplayClass1_ = new <>c__DisplayClass1_0();
	<>c__DisplayClass1_.input = input;
	if (<>c__DisplayClass1_.input < int.MinValue)
	{
		return <>c__DisplayClass1_.<Test>b__0;
	}
	return <>c.<>9__1_1 ?? (<>c.<>9__1_1 = <>c.<>9.<Test>b__1_1);
}

c__DisplayClass1_0 is unconditionally allocated.

By pulling the offending code into a local function, we avoid the unconditional allocation:

    public static Action Test(int input)
    {
        Action action;

        if (input < int.MinValue)
        {
            action = WriteConsoleAction();
        }
        else
        {
            action = () => { };
        }

        return action;

        Action WriteConsoleAction()
        {
            return () => Console.WriteLine(input);
        }
    }

Instead, we only allocate when we call the local function:

public static Action Test(int input)
{
	if (input < int.MinValue)
	{
		return WriteConsoleAction(input);
	}
	return <>c.<>9__1_0 ?? (<>c.<>9__1_0 = <>c.<>9.<Test>b__1_0);
	static Action WriteConsoleAction(int input)
	{
		<>c__DisplayClass1_0 <>c__DisplayClass1_ = new <>c__DisplayClass1_0();
		<>c__DisplayClass1_.input = input;
		return <>c__DisplayClass1_.<Test>b__2;
	}
}

Creating the Action requires capturing some state objects, and we can avoid this by moving to a static local function. I can add comments or clarify this somehow. What would help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One note, this statement return <>c.<>9__1_0 ?? (<>c.<>9__1_0 = <>c.<>9.<Test>b__1_0); assigns the non-capturing delegate to a static field that gets reused. No additional allocations here.

Copy link
Member

Choose a reason for hiding this comment

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

if there's a capture anywhere in the body of the method, the closure object is created.

TIL. This makes sense then. I'd remove the comment since it's meaningless in this context--the idea was "don't create the context if you won't need it" but if this method is called you need it.

(BuildRequestConfiguration requestConfig, BuildRequestConfiguration parentConfig) = GetConfigurations(configCache, request, schedulingData);

Action<ILoggingService> emitNonErrorLogs = ls => ls.LogComment(
NewBuildEventContext(request),
MessageImportance.Normal,
"SkippedConstraintsOnRequest",
parentConfig.ProjectFullPath,
requestConfig.ProjectFullPath);

return emitNonErrorLogs;
}
}

/// <summary>
Expand Down
Loading