Skip to content
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

Several changes to reduce allocations #11273

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Several changes to reduce allocations #11273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3e869e9
Several changes to reduce allocations
Erarndt Jan 9, 2025
6ead7d3
Merge branch 'main' of https://github.com/dotnet/msbuild into dev/era…
Erarndt Feb 3, 2025
4d2b7fd
Move some changes to their own PRs
Erarndt Feb 3, 2025
efd8f40
Address PR feedback
Erarndt Feb 26, 2025
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
12 changes: 8 additions & 4 deletions src/Build/BackEnd/Components/BuildRequestEngine/BuildRequestEngine.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1128,8 +1128,8 @@ private void IssueBuildRequests(BuildRequestEntry issuingEntry, FullyQualifiedBu
// to the entry rather than a series of them.
lock (issuingEntry.GlobalLock)
{
var existingResultsToReport = new List<BuildResult>();
var unresolvedConfigurationsAdded = new HashSet<int>();
List<BuildResult> existingResultsToReport = null;
HashSet<int> unresolvedConfigurationsAdded = new HashSet<int>();

foreach (FullyQualifiedBuildRequest request in newRequests)
{
Expand Down Expand Up @@ -1232,6 +1232,7 @@ private void IssueBuildRequests(BuildRequestEntry issuingEntry, FullyQualifiedBu

// Can't report the result directly here, because that could cause the request to go from
// Waiting to Ready.
existingResultsToReport ??= new List<BuildResult>();
existingResultsToReport.Add(response.Results);
}
else
Expand All @@ -1243,9 +1244,12 @@ private void IssueBuildRequests(BuildRequestEntry issuingEntry, FullyQualifiedBu
}

// If we have any results we had to report, do so now.
foreach (BuildResult existingResult in existingResultsToReport)
if (existingResultsToReport is not null)
{
issuingEntry.ReportResult(existingResult);
foreach (BuildResult existingResult in existingResultsToReport)
{
issuingEntry.ReportResult(existingResult);
}
}

// Issue any configuration requests we may still need.
Expand Down
4 changes: 2 additions & 2 deletions src/Build/BackEnd/Components/RequestBuilder/BatchingEngine.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ private static Dictionary<string, ICollection<ProjectItemInstance>> GetItemLists
if (items != null)
{
// Loop through all the items in the BuildItemGroup.
foreach (ProjectItemInstance item in items)
foreach (ProjectItemInstance item in items.GetStructEnumerable())
{
ProjectErrorUtilities.VerifyThrowInvalidProject(
item.HasMetadata(consumedMetadataReference.MetadataName),
Expand Down Expand Up @@ -319,7 +319,7 @@ private static List<ItemBucket> BucketConsumedItems(

if (items != null)
{
foreach (ProjectItemInstance item in items)
foreach (ProjectItemInstance item in items.GetStructEnumerable())
{
// Get this item's values for all the metadata consumed by the batchable object.
Dictionary<string, string> itemMetadataValues = GetItemMetadataValues(item, consumedMetadataReferences, elementLocation);
Expand Down
20 changes: 12 additions & 8 deletions src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/ItemGroupIntrinsicTask.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,21 +486,25 @@ private List<ProjectItemInstance> ExpandItemIntoItems(
{
foreach (var item in items)
{
var metadataToRemove = item.MetadataNames.Where(name => !keepMetadata.Contains(name));
foreach (var metadataName in metadataToRemove)
foreach (var metadataName in item.MetadataNames)
{
item.RemoveMetadata(metadataName);
if (!keepMetadata.Contains(metadataName))
{
item.RemoveMetadata(metadataName);
}
}
}
}
else if (removeMetadata != null)
{
foreach (var item in items)
{
var metadataToRemove = item.MetadataNames.Where(name => removeMetadata.Contains(name));
foreach (var metadataName in metadataToRemove)
foreach (var metadataName in item.MetadataNames)
{
item.RemoveMetadata(metadataName);
if (removeMetadata.Contains(metadataName))
{
item.RemoveMetadata(metadataName);
}
}
}
}
Expand All @@ -517,7 +521,7 @@ private List<ProjectItemInstance> ExpandItemIntoItems(
/// <returns>A list of matching items</returns>
private HashSet<string> EvaluateExcludePaths(IReadOnlyList<string> excludes, ElementLocation excludeLocation)
{
HashSet<string> excludesUnescapedForComparison = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
HashSet<string> excludesUnescapedForComparison = new HashSet<string>(excludes.Count, StringComparer.OrdinalIgnoreCase);
foreach (string excludeSplit in excludes)
{
string[] excludeSplitFiles = EngineFileUtilities.GetFileListUnescaped(
Expand Down Expand Up @@ -595,7 +599,7 @@ private List<ProjectItemInstance> FindItemsMatchingSpecification(
// filename in the remove list.
List<ProjectItemInstance> itemsRemoved = new List<ProjectItemInstance>();

foreach (ProjectItemInstance item in items)
foreach (ProjectItemInstance item in items.GetStructEnumerable())
{
// Even if the case for the excluded files is different, they
// will still get excluded, as expected. However, if the excluded path
Expand Down
22 changes: 17 additions & 5 deletions src/Build/BackEnd/Components/RequestBuilder/Lookup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,8 +481,14 @@ public ICollection<ProjectItemInstance> GetItems(string itemType)
ICollection<ProjectItemInstance> adds = scope.Adds[itemType];
if (adds.Count != 0)
{
allAdds ??= new List<ProjectItemInstance>(adds.Count);
allAdds.AddRange(adds);
if (allAdds == null)
{
allAdds = new List<ProjectItemInstance>(adds);
}
else
{
allAdds.AddRange(adds);
}
}
}

Expand All @@ -492,8 +498,14 @@ public ICollection<ProjectItemInstance> GetItems(string itemType)
ICollection<ProjectItemInstance> removes = scope.Removes[itemType];
if (removes.Count != 0)
{
allRemoves ??= new List<ProjectItemInstance>(removes.Count);
allRemoves.AddRange(removes);
if (allRemoves == null)
{
allRemoves = new List<ProjectItemInstance>(removes);
}
else
{
allRemoves.AddRange(removes);
}
}
}

Expand Down Expand Up @@ -715,7 +727,7 @@ internal void AddNewItem(ProjectItemInstance item)
/// </summary>
internal void RemoveItems(IEnumerable<ProjectItemInstance> items)
{
foreach (ProjectItemInstance item in items)
foreach (ProjectItemInstance item in items.GetStructEnumerable())
{
RemoveItem(item);
}
Expand Down
65 changes: 46 additions & 19 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 @@ -699,7 +699,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 @@ -992,7 +992,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);
unscheduledRequests.AddRange(_schedulingData.UnscheduledRequestsWhichCanBeScheduled);
foreach (SchedulableRequest request in unscheduledRequests)
{
if (CanScheduleRequestToNode(request, InProcNodeId) && shouldBeScheduled(request))
Expand Down Expand Up @@ -1250,7 +1251,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)
{
// Don't overload the system.
if (AtSchedulingLimit())
Expand All @@ -1259,7 +1260,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 @@ -1972,21 +1973,13 @@ private bool CheckIfCacheMissOnReferencedProjectIsAllowedAndErrorIfNot(int nodeF
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 @@ -2014,7 +2007,7 @@ private bool CheckIfCacheMissOnReferencedProjectIsAllowedAndErrorIfNot(int nodeF

return false;

BuildEventContext NewBuildEventContext()
static BuildEventContext NewBuildEventContext(BuildRequest request)
{
return new BuildEventContext(
request.SubmissionId,
Expand All @@ -2025,13 +2018,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 @@ -2044,7 +2057,7 @@ BuildEventContext NewBuildEventContext()

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)
Expand Down Expand Up @@ -2080,6 +2093,20 @@ bool SkipNonexistentTargetsIfExistentTargetsHaveResults(BuildRequest buildReques
// to skip nonexistent targets.
return true;
}
static Action<ILoggingService> GetLoggingServiceAction(IConfigCache configCache, BuildRequest request, SchedulingData schedulingData)
{
Action<ILoggingService> emitNonErrorLogs;
// retrieving the configs is not quite free, so avoid computing them eagerly
var configs = GetConfigurations(configCache, request, schedulingData);

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

/// <summary>
Expand Down
30 changes: 14 additions & 16 deletions src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,12 @@ internal class SdkResolverService : ISdkResolverService
/// <summary>
/// Stores the list of manifests of specific SDK resolvers which could be loaded.
/// </summary>
protected IReadOnlyList<SdkResolverManifest> _specificResolversManifestsRegistry;
private List<SdkResolverManifest> _specificResolversManifestsRegistry;

/// <summary>
/// Stores the list of manifests of general SDK resolvers which could be loaded.
/// </summary>
protected IReadOnlyList<SdkResolverManifest> _generalResolversManifestsRegistry;
private List<SdkResolverManifest> _generalResolversManifestsRegistry;

/// <summary>
/// Stores an <see cref="SdkResolverLoader"/> which can load registered SDK resolvers.
Expand Down Expand Up @@ -226,7 +226,7 @@ private SdkResult ResolveSdkUsingResolversWithPatternsFirst(int submissionId, Sd
resolvers = GetResolvers(
_generalResolversManifestsRegistry,
loggingContext,
sdkReferenceLocation).ToList();
sdkReferenceLocation);

if (TryResolveSdkUsingSpecifiedResolvers(
resolvers,
Expand Down Expand Up @@ -259,22 +259,20 @@ private SdkResult ResolveSdkUsingResolversWithPatternsFirst(int submissionId, Sd
return new SdkResult(sdk, null, null);
}

private List<SdkResolver> GetResolvers(IReadOnlyList<SdkResolverManifest> resolversManifests, LoggingContext loggingContext, ElementLocation sdkReferenceLocation)
private List<SdkResolver> GetResolvers(List<SdkResolverManifest> resolversManifests, LoggingContext loggingContext, ElementLocation sdkReferenceLocation)
{
// Create a sorted by priority list of resolvers. Load them if needed.
List<SdkResolver> resolvers = new List<SdkResolver>();
foreach (var resolverManifest in resolversManifests)
{
if (!_manifestToResolvers.TryGetValue(resolverManifest, out IReadOnlyList<SdkResolver> newResolvers))
IReadOnlyList<SdkResolver> newResolvers;
lock (_lockObject)
{
lock (_lockObject)
if (!_manifestToResolvers.TryGetValue(resolverManifest, out newResolvers))
{
if (!_manifestToResolvers.TryGetValue(resolverManifest, out newResolvers))
{
// Loading of the needed resolvers.
newResolvers = _sdkResolverLoader.LoadResolversFromManifest(resolverManifest, sdkReferenceLocation);
_manifestToResolvers[resolverManifest] = newResolvers;
}
// Loading of the needed resolvers.
newResolvers = _sdkResolverLoader.LoadResolversFromManifest(resolverManifest, sdkReferenceLocation);
_manifestToResolvers[resolverManifest] = newResolvers;
}
}

Expand Down Expand Up @@ -423,8 +421,8 @@ internal virtual void InitializeForTests(SdkResolverLoader resolverLoader = null
SdkResolverManifest sdkResolverManifest = new SdkResolverManifest(DisplayName: "TestResolversManifest", Path: null, ResolvableSdkRegex: null);
generalResolversManifestsRegistry.Add(sdkResolverManifest);
_manifestToResolvers[sdkResolverManifest] = resolvers;
_generalResolversManifestsRegistry = generalResolversManifestsRegistry.AsReadOnly();
_specificResolversManifestsRegistry = specificResolversManifestsRegistry.AsReadOnly();
_generalResolversManifestsRegistry = generalResolversManifestsRegistry;
_specificResolversManifestsRegistry = specificResolversManifestsRegistry;
}
}

Expand Down Expand Up @@ -521,8 +519,8 @@ private void RegisterResolversManifests(ElementLocation location)
// Then it will wait at the lock and return after we release it since the collections we have filled them before releasing the lock.
// The collections are never modified after this point.
// So I've made them ReadOnly
_specificResolversManifestsRegistry = specificResolversManifestsRegistry.AsReadOnly();
_generalResolversManifestsRegistry = generalResolversManifestsRegistry.AsReadOnly();
_specificResolversManifestsRegistry = specificResolversManifestsRegistry;
_generalResolversManifestsRegistry = generalResolversManifestsRegistry;
}
}

Expand Down
Loading
Loading