Skip to content

Use hashing for targets list comparison and enable scheduler tests #11870

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

Merged
merged 4 commits into from
May 29, 2025
Merged

Use hashing for targets list comparison and enable scheduler tests #11870

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b75dd18
Use hashing for targets list comparison and enable scheduler tests
YuliiaKovalova May 20, 2025
47e1503
use more robust aggregation
YuliiaKovalova May 20, 2025
15dee5d
fix review comment
YuliiaKovalova May 20, 2025
d79cb67
cleanup
YuliiaKovalova May 20, 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
44 changes: 22 additions & 22 deletions src/Build.UnitTests/BackEnd/Scheduler_Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ public void Dispose()
/// <summary>
/// Verify that when a single request is submitted, we get a request assigned back out.
/// </summary>
[Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
[Fact]
public void TestSimpleRequest()
{
CreateConfiguration(1, "foo.proj");
Expand All @@ -117,7 +117,7 @@ public void TestSimpleRequest()
/// <summary>
/// Verify that when we submit a request and we already have results, we get the results back.
/// </summary>
[Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
[Fact]
public void TestSimpleRequestWithCachedResultsSuccess()
{
CreateConfiguration(1, "foo.proj");
Expand All @@ -141,7 +141,7 @@ public void TestSimpleRequestWithCachedResultsSuccess()
/// <summary>
/// Verify that when we submit a request with failing results, we get the results back.
/// </summary>
[Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
[Fact]
public void TestSimpleRequestWithCachedResultsFail()
{
CreateConfiguration(1, "foo.proj");
Expand All @@ -165,7 +165,7 @@ public void TestSimpleRequestWithCachedResultsFail()
/// <summary>
/// Verify that when we submit a child request with results cached, we get those results back.
/// </summary>
[Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
[Fact]
public void TestChildRequest()
{
CreateConfiguration(1, "foo.proj");
Expand Down Expand Up @@ -195,7 +195,7 @@ public void TestChildRequest()
/// <summary>
/// Verify that when multiple requests are submitted, the first one in is the first one out.
/// </summary>
[Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
[Fact]
public void TestMultipleRequests()
{
CreateConfiguration(1, "foo.proj");
Expand All @@ -213,7 +213,7 @@ public void TestMultipleRequests()
/// <summary>
/// Verify that when multiple requests are submitted with results cached, we get the results back.
/// </summary>
[Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
[Fact]
public void TestMultipleRequestsWithSomeResults()
{
CreateConfiguration(1, "foo.proj");
Expand All @@ -235,7 +235,7 @@ public void TestMultipleRequestsWithSomeResults()
/// <summary>
/// Verify that when multiple requests are submitted with results cached, we get the results back.
/// </summary>
[Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
[Fact]
public void TestMultipleRequestsWithAllResults()
{
CreateConfiguration(1, "foo.proj");
Expand Down Expand Up @@ -266,7 +266,7 @@ public void TestMultipleRequestsWithAllResults()
/// Verify that if the affinity of one of the requests is out-of-proc, we create an out-of-proc node (but only one)
/// even if the max node count = 1.
/// </summary>
[Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
[Fact]
public void TestOutOfProcNodeCreatedWhenAffinityIsOutOfProc()
{
CreateConfiguration(1, "foo.proj");
Expand All @@ -288,7 +288,7 @@ public void TestOutOfProcNodeCreatedWhenAffinityIsOutOfProc()
/// Verify that if the affinity of our requests is out-of-proc, that many out-of-proc nodes will
/// be made (assuming it does not exceed MaxNodeCount)
/// </summary>
[Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
[Fact]
public void TestOutOfProcNodesCreatedWhenAffinityIsOutOfProc()
{
_host.BuildParameters.MaxNodeCount = 4;
Expand All @@ -313,7 +313,7 @@ public void TestOutOfProcNodesCreatedWhenAffinityIsOutOfProc()
/// we still won't create any new nodes if they're all for the same configuration --
/// they'd end up all being assigned to the same node.
/// </summary>
[Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
[Fact]
public void TestNoNewNodesCreatedForMultipleRequestsWithSameConfiguration()
{
_host.BuildParameters.MaxNodeCount = 3;
Expand All @@ -336,7 +336,7 @@ public void TestNoNewNodesCreatedForMultipleRequestsWithSameConfiguration()
/// Verify that if the affinity of our requests is "any", we will not create more than
/// MaxNodeCount nodes (1 IP node + MaxNodeCount - 1 OOP nodes)
/// </summary>
[Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
[Fact]
public void TestMaxNodeCountNotExceededWithRequestsOfAffinityAny()
{
_host.BuildParameters.MaxNodeCount = 3;
Expand Down Expand Up @@ -366,7 +366,7 @@ public void TestMaxNodeCountNotExceededWithRequestsOfAffinityAny()
/// node will service an Any request instead of an inproc request, leaving only one non-inproc request for the second round
/// of node creation.
/// </summary>
[Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
[Fact]
public void VerifyRequestOrderingDoesNotAffectNodeCreationCountWithInProcAndAnyRequests()
{
// Since we're creating our own BuildManager, we need to make sure that the default
Expand Down Expand Up @@ -414,7 +414,7 @@ public void VerifyRequestOrderingDoesNotAffectNodeCreationCountWithInProcAndAnyR
/// Verify that if the affinity of our requests is out-of-proc, we will create as many as
/// MaxNodeCount out-of-proc nodes
/// </summary>
[Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
[Fact]
public void TestMaxNodeCountOOPNodesCreatedForOOPAffinitizedRequests()
{
_host.BuildParameters.MaxNodeCount = 3;
Expand Down Expand Up @@ -444,7 +444,7 @@ public void TestMaxNodeCountOOPNodesCreatedForOOPAffinitizedRequests()
/// is less than MaxNodeCount, that we only create MaxNodeCount - 1 OOP nodes (for a total of MaxNodeCount
/// nodes, when the inproc node is included)
/// </summary>
[Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
[Fact]
public void TestMaxNodeCountNodesNotExceededWithSomeOOPRequests1()
{
_host.BuildParameters.MaxNodeCount = 3;
Expand Down Expand Up @@ -474,7 +474,7 @@ public void TestMaxNodeCountNodesNotExceededWithSomeOOPRequests1()
/// is less than MaxNodeCount, that we only create MaxNodeCount - 1 OOP nodes (for a total of MaxNodeCount
/// nodes, when the inproc node is included)
/// </summary>
[Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
[Fact]
public void TestMaxNodeCountNodesNotExceededWithSomeOOPRequests2()
{
_host.BuildParameters.MaxNodeCount = 3;
Expand Down Expand Up @@ -511,7 +511,7 @@ public void SchedulerShouldHonorDisableInprocNode()
/// Make sure that traversal projects are marked with an affinity of "InProc", which means that
/// even if multiple are available, we should still only have the single inproc node.
/// </summary>
[Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
[Fact]
public void TestTraversalAffinityIsInProc()
{
_host.BuildParameters.MaxNodeCount = 3;
Expand Down Expand Up @@ -560,7 +560,7 @@ public void TestProxyAffinityIsInProc()
/// With something approximating the BuildManager's build loop, make sure that we don't end up
/// trying to create more nodes than we can actually support.
/// </summary>
[Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
[Fact]
public void VerifyNoOverCreationOfNodesWithBuildLoop()
{
// Since we're creating our own BuildManager, we need to make sure that the default
Expand Down Expand Up @@ -615,7 +615,7 @@ public void BuildResultNotPlacedInCurrentCacheIfConfigExistsInOverrideCache()
/// <summary>
/// Verify that if we get two requests but one of them is a failure, we only get the failure result back.
/// </summary>
[Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
[Fact]
public void TestTwoRequestsWithFirstFailure()
{
CreateConfiguration(1, "foo.proj");
Expand All @@ -634,7 +634,7 @@ public void TestTwoRequestsWithFirstFailure()
/// <summary>
/// Verify that if we get two requests but one of them is a failure, we only get the failure result back.
/// </summary>
[Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
[Fact]
public void TestTwoRequestsWithSecondFailure()
{
CreateConfiguration(1, "foo.proj");
Expand All @@ -653,7 +653,7 @@ public void TestTwoRequestsWithSecondFailure()
/// <summary>
/// Verify that if we get three requests but one of them is a failure, we only get the failure result back.
/// </summary>
[Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
[Fact]
public void TestThreeRequestsWithOneFailure()
{
CreateConfiguration(1, "foo.proj");
Expand All @@ -673,7 +673,7 @@ public void TestThreeRequestsWithOneFailure()
/// <summary>
/// Verify that providing a result to the only outstanding request results in build complete.
/// </summary>
[Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
[Fact]
public void TestResult()
{
CreateConfiguration(1, "foo.proj");
Expand All @@ -697,7 +697,7 @@ public void TestResult()
/// <summary>
/// Tests that the detailed summary setting causes the summary to be produced.
/// </summary>
[Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
[Fact]
public void TestDetailedSummary()
{
string contents = ObjectModelHelpers.CleanupFileContents(@"
Expand Down
76 changes: 48 additions & 28 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 @@ -2239,39 +2239,30 @@ private bool RequestOrAnyItIsBlockedByCanBeServiced(SchedulableRequest request)
}

/// <summary>
/// Determines if we have a matching request somewhere, and if so, assigns the same request ID. Otherwise
/// assigns a new request id.
/// Determines if we have a matching request somewhere, and if so, assigns the same request ID.
/// Otherwise assigns a new request id.
/// </summary>
/// <remarks>
/// UNDONE: (Performance) This algorithm should be modified so we don't have to iterate over all of the
/// requests to find a matching one. A HashSet with proper equality semantics and a good hash code for the BuildRequest
/// would speed this considerably, especially for large numbers of projects in a build.
/// </remarks>
/// <param name="request">The request whose ID should be assigned</param>
private void AssignGlobalRequestId(BuildRequest request)
{
bool assignNewId = false;
if (request.GlobalRequestId == BuildRequest.InvalidGlobalRequestId && _schedulingData.GetRequestsAssignedToConfigurationCount(request.ConfigurationId) > 0)
// Quick exit if already assigned or if there are no requests for this configuration
if (request.GlobalRequestId != BuildRequest.InvalidGlobalRequestId
|| _schedulingData.GetRequestsAssignedToConfigurationCount(request.ConfigurationId) == 0)
{
request.GlobalRequestId = _nextGlobalRequestId++;
return;
}

// Look for matching requests in the configuration
foreach (SchedulableRequest existingRequest in _schedulingData.GetRequestsAssignedToConfiguration(request.ConfigurationId))
{
foreach (SchedulableRequest existingRequest in _schedulingData.GetRequestsAssignedToConfiguration(request.ConfigurationId))
if (existingRequest.BuildRequest.Targets.Count == request.Targets.Count)
{
if (existingRequest.BuildRequest.Targets.Count == request.Targets.Count)
// Check if the hash matches before doing expensive comparisons
if (ComputeTargetsHash(request.Targets) == ComputeTargetsHash(existingRequest.BuildRequest.Targets))
{
List<string> leftTargets = new List<string>(existingRequest.BuildRequest.Targets);
List<string> rightTargets = new List<string>(request.Targets);

leftTargets.Sort(StringComparer.OrdinalIgnoreCase);
rightTargets.Sort(StringComparer.OrdinalIgnoreCase);
for (int i = 0; i < leftTargets.Count; i++)
{
if (!leftTargets[i].Equals(rightTargets[i], StringComparison.OrdinalIgnoreCase))
{
assignNewId = true;
break;
}
}

if (!assignNewId)
// Double-check with full comparison only when hash matches
if (TargetsMatch(request.Targets, existingRequest.BuildRequest.Targets))
{
request.GlobalRequestId = existingRequest.BuildRequest.GlobalRequestId;
return;
Expand All @@ -2280,8 +2271,37 @@ private void AssignGlobalRequestId(BuildRequest request)
}
}

request.GlobalRequestId = _nextGlobalRequestId;
_nextGlobalRequestId++;
// No matching request found, assign a new ID
request.GlobalRequestId = _nextGlobalRequestId++;
}

/// <summary>
/// Computes a hash code for a collection of targets, ignoring order and case.
/// </summary>
private int ComputeTargetsHash(List<string> targets)
{
int hash = 0;
foreach (string target in targets)
{
hash ^= StringComparer.OrdinalIgnoreCase.GetHashCode(target);
}

return hash;
}

/// <summary>
/// Determines if two target collections contain the same targets, ignoring order and case.
/// </summary>
private bool TargetsMatch(List<string> firstTargetsList, List<string> secondTargetsList)
{
if (firstTargetsList.Count != secondTargetsList.Count)
{
return false;
}

HashSet<string> set = new HashSet<string>(firstTargetsList, StringComparer.OrdinalIgnoreCase);

return set.SetEquals(secondTargetsList);
}

/// <summary>
Expand Down
Loading